Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate --weak-refs in favor of run-time detection #3822

Merged
merged 4 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,7 @@ jobs:
- run: cargo test --target wasm32-unknown-unknown -p wasm-bindgen-futures
- run: cargo test --target wasm32-unknown-unknown --test wasm
env:
WASM_BINDGEN_WEAKREF: 1
- run: cargo test --target wasm32-unknown-unknown --test wasm
env:
WASM_BINDGEN_WEAKREF: 1
WASM_BINDGEN_NO_DEBUG: 1
- run: cargo test --target wasm32-unknown-unknown --test wasm --features serde-serialize
env:
WASM_BINDGEN_WEAKREF: 1
- run: cargo test --target wasm32-unknown-unknown
env:
WASM_BINDGEN_EXTERNREF: 1
Expand Down
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@
* Updated the WebGPU WebIDL to the current draft as of 2024-01-30. Note that this retains the previous update's workaround for `GPUPipelineError`, and holds back an update to the `buffer` argument of the `GPUQueue.{writeBuffer,writeTexture}` methods.
[#3816](https://github.com/rustwasm/wasm-bindgen/pull/3816)

* Depreate `--weak-refs` and `WASM_BINDGEN_WEAKREF` in favor of automatic run-time detection.
[#3822](https://github.com/rustwasm/wasm-bindgen/pull/3822)

### Fixed

* Fixed UB when freeing strings received from JS if not using the default allocator.
Expand Down
79 changes: 23 additions & 56 deletions crates/cli-support/src/js/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -921,18 +921,12 @@ impl<'a> Context<'a> {
"
static __wrap(ptr) {{
ptr = ptr >>> 0;
const obj = Object.create({}.prototype);
const obj = Object.create({name}.prototype);
obj.__wbg_ptr = ptr;
{}
{name}Finalization.register(obj, obj.__wbg_ptr, obj);
return obj;
}}
",
name,
if self.config.weak_refs {
format!("{}Finalization.register(obj, obj.__wbg_ptr, obj);", name)
} else {
String::new()
},
"
));
}

Expand All @@ -950,13 +944,13 @@ impl<'a> Context<'a> {
));
}

if self.config.weak_refs {
self.global(&format!(
"const {}Finalization = new FinalizationRegistry(ptr => wasm.{}(ptr >>> 0));",
name,
wasm_bindgen_shared::free_function(name),
));
}
self.global(&format!(
"
const {name}Finalization = (typeof FinalizationRegistry === 'undefined')
? {{ register: () => {{}}, unregister: () => {{}} }}
: new FinalizationRegistry(ptr => wasm.{}(ptr >>> 0));",
wasm_bindgen_shared::free_function(name),
));

// If the class is inspectable, generate `toJSON` and `toString`
// to expose all readable properties of the class. Otherwise,
Expand Down Expand Up @@ -1021,7 +1015,7 @@ impl<'a> Context<'a> {
__destroy_into_raw() {{
const ptr = this.__wbg_ptr;
this.__wbg_ptr = 0;
{}
{name}Finalization.unregister(this);
return ptr;
}}

Expand All @@ -1030,11 +1024,6 @@ impl<'a> Context<'a> {
wasm.{}(ptr);
}}
",
if self.config.weak_refs {
format!("{}Finalization.unregister(this);", name)
} else {
String::new()
},
wasm_bindgen_shared::free_function(name),
));
ts_dst.push_str(" free(): void;\n");
Expand Down Expand Up @@ -2121,15 +2110,7 @@ impl<'a> Context<'a> {

let table = self.export_function_table()?;

let (register, unregister) = if self.config.weak_refs {
self.expose_closure_finalization()?;
(
"CLOSURE_DTORS.register(real, state, state);",
"CLOSURE_DTORS.unregister(state)",
)
} else {
("", "")
};
self.expose_closure_finalization()?;

// For mutable closures they can't be invoked recursively.
// To handle that we swap out the `this.a` pointer with zero
Expand All @@ -2152,20 +2133,17 @@ impl<'a> Context<'a> {
}} finally {{
if (--state.cnt === 0) {{
wasm.{table}.get(state.dtor)(a, state.b);
{unregister}
CLOSURE_DTORS.unregister(state);
}} else {{
state.a = a;
}}
}}
}};
real.original = state;
{register}
CLOSURE_DTORS.register(real, state, state);
return real;
}}
",
table = table,
register = register,
unregister = unregister,
));

Ok(())
Expand All @@ -2178,15 +2156,7 @@ impl<'a> Context<'a> {

let table = self.export_function_table()?;

let (register, unregister) = if self.config.weak_refs {
self.expose_closure_finalization()?;
(
"CLOSURE_DTORS.register(real, state, state);",
"CLOSURE_DTORS.unregister(state)",
)
} else {
("", "")
};
self.expose_closure_finalization()?;

// For shared closures they can be invoked recursively so we
// just immediately pass through `this.a`. If we end up
Expand All @@ -2208,18 +2178,15 @@ impl<'a> Context<'a> {
if (--state.cnt === 0) {{
wasm.{table}.get(state.dtor)(state.a, state.b);
state.a = 0;
{unregister}
CLOSURE_DTORS.unregister(state);
}}
}}
}};
real.original = state;
{register}
CLOSURE_DTORS.register(real, state, state);
return real;
}}
",
table = table,
register = register,
unregister = unregister,
));

Ok(())
Expand All @@ -2229,15 +2196,15 @@ impl<'a> Context<'a> {
if !self.should_write_global("closure_finalization") {
return Ok(());
}
assert!(self.config.weak_refs);
let table = self.export_function_table()?;
self.global(&format!(
"
const CLOSURE_DTORS = new FinalizationRegistry(state => {{
wasm.{}.get(state.dtor)(state.a, state.b)
}});
",
table
const CLOSURE_DTORS = (typeof FinalizationRegistry === 'undefined')
? {{ register: () => {{}}, unregister: () => {{}} }}
: new FinalizationRegistry(state => {{
wasm.{table}.get(state.dtor)(state.a, state.b)
}});
"
));

Ok(())
Expand Down
9 changes: 0 additions & 9 deletions crates/cli-support/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ pub struct Bindgen {
remove_producers_section: bool,
omit_default_module_path: bool,
emit_start: bool,
// Experimental support for weakrefs, an upcoming ECMAScript feature.
// Currently only enable-able through an env var.
weak_refs: bool,
// Support for the wasm threads proposal, transforms the wasm module to be
// "ready to be instantiated on any thread"
threads: wasm_bindgen_threads_xform::Config,
Expand Down Expand Up @@ -106,7 +103,6 @@ impl Bindgen {
remove_name_section: false,
remove_producers_section: false,
emit_start: true,
weak_refs: env::var("WASM_BINDGEN_WEAKREF").is_ok(),
threads: threads_config(),
externref,
multi_value,
Expand All @@ -126,11 +122,6 @@ impl Bindgen {
self
}

pub fn weak_refs(&mut self, enable: bool) -> &mut Bindgen {
self.weak_refs = enable;
self
}

pub fn reference_types(&mut self, enable: bool) -> &mut Bindgen {
self.externref = enable;
self
Expand Down
5 changes: 1 addition & 4 deletions crates/cli/src/bin/wasm-bindgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ Options:
--nodejs Deprecated, use `--target nodejs`
--web Deprecated, use `--target web`
--no-modules Deprecated, use `--target no-modules`
--weak-refs Enable usage of the JS weak references proposal
--weak-refs Deprecated, is runtime-detected
--reference-types Enable usage of WebAssembly reference types
-V --version Print the version number of wasm-bindgen

Expand Down Expand Up @@ -126,9 +126,6 @@ fn rmain(args: &Args) -> Result<(), Error> {
.omit_imports(args.flag_omit_imports)
.omit_default_module_path(args.flag_omit_default_module_path)
.split_linked_modules(args.flag_split_linked_modules);
if let Some(true) = args.flag_weak_refs {
b.weak_refs(true);
}
if let Some(true) = args.flag_reference_types {
b.reference_types(true);
}
Expand Down
8 changes: 6 additions & 2 deletions crates/cli/tests/reference/builder.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ function getStringFromWasm0(ptr, len) {
ptr = ptr >>> 0;
return cachedTextDecoder.decode(getUint8Memory0().subarray(ptr, ptr + len));
}

const ClassBuilderFinalization = (typeof FinalizationRegistry === 'undefined')
? { register: () => {}, unregister: () => {} }
: new FinalizationRegistry(ptr => wasm.__wbg_classbuilder_free(ptr >>> 0));
/**
*/
export class ClassBuilder {
Expand All @@ -31,14 +35,14 @@ export class ClassBuilder {
ptr = ptr >>> 0;
const obj = Object.create(ClassBuilder.prototype);
obj.__wbg_ptr = ptr;

ClassBuilderFinalization.register(obj, obj.__wbg_ptr, obj);
return obj;
}

__destroy_into_raw() {
const ptr = this.__wbg_ptr;
this.__wbg_ptr = 0;

ClassBuilderFinalization.unregister(this);
return ptr;
}

Expand Down
6 changes: 5 additions & 1 deletion crates/cli/tests/reference/constructor.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,18 @@ function getStringFromWasm0(ptr, len) {
ptr = ptr >>> 0;
return cachedTextDecoder.decode(getUint8Memory0().subarray(ptr, ptr + len));
}

const ClassConstructorFinalization = (typeof FinalizationRegistry === 'undefined')
? { register: () => {}, unregister: () => {} }
: new FinalizationRegistry(ptr => wasm.__wbg_classconstructor_free(ptr >>> 0));
/**
*/
export class ClassConstructor {

__destroy_into_raw() {
const ptr = this.__wbg_ptr;
this.__wbg_ptr = 0;

ClassConstructorFinalization.unregister(this);
return ptr;
}

Expand Down
9 changes: 0 additions & 9 deletions guide/src/reference/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,6 @@ When generating bundler-compatible code (see the section on [deployment]) this
indicates that the bundled code is always intended to go into a browser so a few
checks for Node.js can be elided.

### `--weak-refs`

Enables usage of the [TC39 Weak References
proposal](https://github.com/tc39/proposal-weakrefs), ensuring that all Rust
memory is eventually deallocated regardless of whether you're calling `free` or
not. This is off-by-default while we're waiting for support to percolate into
all major browsers. For more information see the [documentation about weak
references](./weak-references.md).

### `--reference-types`

Enables usage of the [WebAssembly References Types
Expand Down
14 changes: 6 additions & 8 deletions guide/src/reference/weak-references.md
daxpedda marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# Support for Weak References

By default wasm-bindgen does not use the [TC39 weak references
proposal](https://github.com/tc39/proposal-weakrefs). This proposal just
advanced to stage 4 at the time of this writing, but it will still stake some
time for support to percolate into all the major browsers.
By default wasm-bindgen does use the [TC39 weak references
proposal](https://github.com/tc39/proposal-weakrefs) if support is detected.
At the time of this writing all major browsers do support it.

Without weak references your JS integration may be susceptible to memory leaks
in Rust, for example:
Expand All @@ -15,9 +14,8 @@ in Rust, for example:
* Rust closures have `Closure::{into_js_value,forget}` methods which explicitly
do not free the underlying memory.

These issues are all solved with the weak references proposal in JS. The
`--weak-refs` flag to the `wasm-bindgen` CLI will enable usage of
`FinalizationRegistry` to ensure that all memory is cleaned up, regardless of
These issues are all solved with the weak references proposal in JS.
`FinalizationRegistry` will ensure that all memory is cleaned up, regardless of
whether it's explicitly deallocated or not. Note that explicit deallocation
is always a possibility and supported, but if it's not called then memory will
still be automatically deallocated with the `--weak-refs` flag.
still be automatically deallocated if `FinalizationRegistry` support is detected.
16 changes: 6 additions & 10 deletions src/closure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,16 +356,12 @@ where
/// lifetime dynamically managed by the JS GC. This function can be used
/// to drop this `Closure` while keeping the associated JS function still
/// valid.
///
/// By default this function will leak memory. This can be dangerous if this
/// function is called many times in an application because the memory leak
/// will overwhelm the page quickly and crash the wasm.
///
/// If the browser, however, supports weak references, then this function
/// will not leak memory. Instead the Rust memory will be reclaimed when the
/// JS closure is GC'd. Weak references are not enabled by default since
/// they're still a proposal for the JS standard. They can be enabled with
/// `WASM_BINDGEN_WEAKREF=1` when running `wasm-bindgen`, however.
///
/// If the platform supports weak references, the Rust memory will be
/// reclaimed when the JS closure is GC'd. If weak references is not
/// supported, this can be dangerous if this function is called many times
/// in an application because the memory leak will overwhelm the page
/// quickly and crash the wasm.
pub fn into_js_value(self) -> JsValue {
let idx = self.js.idx;
mem::forget(self);
Expand Down
Loading