diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e18609b7d7..2b528788265 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,12 @@ * Fix missing target features in module when enabling reference types or multi-value transformation. [#3967](https://github.com/rustwasm/wasm-bindgen/pull/3967) +* Fixed Rust values getting GC'd while still borrowed. + [#3940](https://github.com/rustwasm/wasm-bindgen/pull/3940) + +* Fixed Rust values not getting GC'd if they were created via. a constructor. + [#3940](https://github.com/rustwasm/wasm-bindgen/pull/3940) + -------------------------------------------------------------------------------- ## [0.2.92](https://github.com/rustwasm/wasm-bindgen/compare/0.2.91...0.2.92) diff --git a/crates/backend/src/ast.rs b/crates/backend/src/ast.rs index 123c4ea9036..91aa23040e6 100644 --- a/crates/backend/src/ast.rs +++ b/crates/backend/src/ast.rs @@ -108,7 +108,7 @@ pub struct Export { /// The 3 types variations of `self`. #[cfg_attr(feature = "extra-traits", derive(Debug, PartialEq, Eq))] -#[derive(Clone)] +#[derive(Copy, Clone)] pub enum MethodSelf { /// `self` ByValue, diff --git a/crates/backend/src/codegen.rs b/crates/backend/src/codegen.rs index 330ffba6626..268d74fc42c 100644 --- a/crates/backend/src/codegen.rs +++ b/crates/backend/src/codegen.rs @@ -239,9 +239,9 @@ impl ToTokens for ast::Struct { type Abi = u32; fn into_abi(self) -> u32 { - use #wasm_bindgen::__rt::std::boxed::Box; + use #wasm_bindgen::__rt::std::rc::Rc; use #wasm_bindgen::__rt::WasmRefCell; - Box::into_raw(Box::new(WasmRefCell::new(self))) as u32 + Rc::into_raw(Rc::new(WasmRefCell::new(self))) as u32 } } @@ -250,14 +250,19 @@ impl ToTokens for ast::Struct { type Abi = u32; unsafe fn from_abi(js: u32) -> Self { - use #wasm_bindgen::__rt::std::boxed::Box; + use #wasm_bindgen::__rt::std::rc::Rc; + use #wasm_bindgen::__rt::std::result::Result::{Ok, Err}; use #wasm_bindgen::__rt::{assert_not_null, WasmRefCell}; let ptr = js as *mut WasmRefCell<#name>; assert_not_null(ptr); - let js = Box::from_raw(ptr); - (*js).borrow_mut(); // make sure no one's borrowing - js.into_inner() + let rc = Rc::from_raw(ptr); + match Rc::try_unwrap(rc) { + Ok(cell) => cell.into_inner(), + Err(_) => #wasm_bindgen::throw_str( + "attempted to take ownership of Rust value while it was borrowed" + ), + } } } @@ -291,39 +296,62 @@ impl ToTokens for ast::Struct { const _: () = { #[no_mangle] #[doc(hidden)] - pub unsafe extern "C" fn #free_fn(ptr: u32) { - let _ = <#name as #wasm_bindgen::convert::FromWasmAbi>::from_abi(ptr); //implicit `drop()` + // `allow_delayed` is whether it's ok to not actually free the `ptr` immediately + // if it's still borrowed. + pub unsafe extern "C" fn #free_fn(ptr: u32, allow_delayed: u32) { + use #wasm_bindgen::__rt::std::rc::Rc; + + if allow_delayed != 0 { + // Just drop the implicit `Rc` owned by JS, and then if the value is still + // referenced it'll be kept alive by its other `Rc`s. + let ptr = ptr as *mut #wasm_bindgen::__rt::WasmRefCell<#name>; + #wasm_bindgen::__rt::assert_not_null(ptr); + drop(Rc::from_raw(ptr)); + } else { + // Claim ownership of the value, which will panic if it's borrowed. + let _ = <#name as #wasm_bindgen::convert::FromWasmAbi>::from_abi(ptr); + } } }; #[automatically_derived] impl #wasm_bindgen::convert::RefFromWasmAbi for #name { type Abi = u32; - type Anchor = #wasm_bindgen::__rt::Ref<'static, #name>; + type Anchor = #wasm_bindgen::__rt::RcRef<#name>; unsafe fn ref_from_abi(js: Self::Abi) -> Self::Anchor { + use #wasm_bindgen::__rt::std::rc::Rc; + let js = js as *mut #wasm_bindgen::__rt::WasmRefCell<#name>; #wasm_bindgen::__rt::assert_not_null(js); - (*js).borrow() + + Rc::increment_strong_count(js); + let rc = Rc::from_raw(js); + #wasm_bindgen::__rt::RcRef::new(rc) } } #[automatically_derived] impl #wasm_bindgen::convert::RefMutFromWasmAbi for #name { type Abi = u32; - type Anchor = #wasm_bindgen::__rt::RefMut<'static, #name>; + type Anchor = #wasm_bindgen::__rt::RcRefMut<#name>; unsafe fn ref_mut_from_abi(js: Self::Abi) -> Self::Anchor { + use #wasm_bindgen::__rt::std::rc::Rc; + let js = js as *mut #wasm_bindgen::__rt::WasmRefCell<#name>; #wasm_bindgen::__rt::assert_not_null(js); - (*js).borrow_mut() + + Rc::increment_strong_count(js); + let rc = Rc::from_raw(js); + #wasm_bindgen::__rt::RcRefMut::new(rc) } } #[automatically_derived] impl #wasm_bindgen::convert::LongRefFromWasmAbi for #name { type Abi = u32; - type Anchor = #wasm_bindgen::__rt::Ref<'static, #name>; + type Anchor = #wasm_bindgen::__rt::RcRef<#name>; unsafe fn long_ref_from_abi(js: Self::Abi) -> Self::Anchor { ::ref_from_abi(js) @@ -562,12 +590,24 @@ impl TryToTokens for ast::Export { } Some(ast::MethodSelf::RefShared) => { let class = self.rust_class.as_ref().unwrap(); + let (trait_, func, borrow) = if self.function.r#async { + ( + quote!(LongRefFromWasmAbi), + quote!(long_ref_from_abi), + quote!( + <<#class as #wasm_bindgen::convert::LongRefFromWasmAbi> + ::Anchor as #wasm_bindgen::__rt::std::borrow::Borrow<#class>> + ::borrow(&me) + ), + ) + } else { + (quote!(RefFromWasmAbi), quote!(ref_from_abi), quote!(&*me)) + }; arg_conversions.push(quote! { let me = unsafe { - <#class as #wasm_bindgen::convert::RefFromWasmAbi> - ::ref_from_abi(me) + <#class as #wasm_bindgen::convert::#trait_>::#func(me) }; - let me = &*me; + let me = #borrow; }); quote! { me.#name } } diff --git a/crates/cli-support/src/js/binding.rs b/crates/cli-support/src/js/binding.rs index 31863da50ab..dfbd28b8b2f 100644 --- a/crates/cli-support/src/js/binding.rs +++ b/crates/cli-support/src/js/binding.rs @@ -1129,7 +1129,12 @@ fn instruction( let val = js.pop(); match constructor { Some(name) if name == class => { - js.prelude(&format!("this.__wbg_ptr = {} >>> 0;", val)); + js.prelude(&format!( + " + this.__wbg_ptr = {val} >>> 0; + {name}Finalization.register(this, this.__wbg_ptr, this); + " + )); js.push(String::from("this")); } Some(_) | None => { diff --git a/crates/cli-support/src/js/mod.rs b/crates/cli-support/src/js/mod.rs index 8c270186bf7..97a7cbd8044 100644 --- a/crates/cli-support/src/js/mod.rs +++ b/crates/cli-support/src/js/mod.rs @@ -948,7 +948,7 @@ impl<'a> Context<'a> { " const {name}Finalization = (typeof FinalizationRegistry === 'undefined') ? {{ register: () => {{}}, unregister: () => {{}} }} - : new FinalizationRegistry(ptr => wasm.{}(ptr >>> 0));", + : new FinalizationRegistry(ptr => wasm.{}(ptr >>> 0, 1));", wasm_bindgen_shared::free_function(name), )); @@ -1021,7 +1021,7 @@ impl<'a> Context<'a> { free() {{ const ptr = this.__destroy_into_raw(); - wasm.{}(ptr); + wasm.{}(ptr, 0); }} ", wasm_bindgen_shared::free_function(name), diff --git a/crates/cli/tests/reference/builder.js b/crates/cli/tests/reference/builder.js index 6db7eb3fbf0..f31735af193 100644 --- a/crates/cli/tests/reference/builder.js +++ b/crates/cli/tests/reference/builder.js @@ -26,7 +26,7 @@ function getStringFromWasm0(ptr, len) { const ClassBuilderFinalization = (typeof FinalizationRegistry === 'undefined') ? { register: () => {}, unregister: () => {} } - : new FinalizationRegistry(ptr => wasm.__wbg_classbuilder_free(ptr >>> 0)); + : new FinalizationRegistry(ptr => wasm.__wbg_classbuilder_free(ptr >>> 0, 1)); /** */ export class ClassBuilder { @@ -48,7 +48,7 @@ export class ClassBuilder { free() { const ptr = this.__destroy_into_raw(); - wasm.__wbg_classbuilder_free(ptr); + wasm.__wbg_classbuilder_free(ptr, 0); } /** * @returns {ClassBuilder} diff --git a/crates/cli/tests/reference/builder.wat b/crates/cli/tests/reference/builder.wat index 0e9d738dcda..16a39fa1d49 100644 --- a/crates/cli/tests/reference/builder.wat +++ b/crates/cli/tests/reference/builder.wat @@ -1,8 +1,8 @@ (module $reference_test.wasm (type (;0;) (func (result i32))) - (type (;1;) (func (param i32))) - (func $classbuilder_builder (;0;) (type 0) (result i32)) - (func $__wbg_classbuilder_free (;1;) (type 1) (param i32)) + (type (;1;) (func (param i32 i32))) + (func $__wbg_classbuilder_free (;0;) (type 1) (param i32 i32)) + (func $classbuilder_builder (;1;) (type 0) (result i32)) (memory (;0;) 17) (export "memory" (memory 0)) (export "__wbg_classbuilder_free" (func $__wbg_classbuilder_free)) diff --git a/crates/cli/tests/reference/constructor.js b/crates/cli/tests/reference/constructor.js index a485efbe747..3f096f76657 100644 --- a/crates/cli/tests/reference/constructor.js +++ b/crates/cli/tests/reference/constructor.js @@ -26,7 +26,7 @@ function getStringFromWasm0(ptr, len) { const ClassConstructorFinalization = (typeof FinalizationRegistry === 'undefined') ? { register: () => {}, unregister: () => {} } - : new FinalizationRegistry(ptr => wasm.__wbg_classconstructor_free(ptr >>> 0)); + : new FinalizationRegistry(ptr => wasm.__wbg_classconstructor_free(ptr >>> 0, 1)); /** */ export class ClassConstructor { @@ -40,13 +40,14 @@ export class ClassConstructor { free() { const ptr = this.__destroy_into_raw(); - wasm.__wbg_classconstructor_free(ptr); + wasm.__wbg_classconstructor_free(ptr, 0); } /** */ constructor() { const ret = wasm.classconstructor_new(); this.__wbg_ptr = ret >>> 0; + ClassConstructorFinalization.register(this, this.__wbg_ptr, this); return this; } } diff --git a/crates/cli/tests/reference/constructor.wat b/crates/cli/tests/reference/constructor.wat index f6d7e3badc2..8bc0fdcb4f6 100644 --- a/crates/cli/tests/reference/constructor.wat +++ b/crates/cli/tests/reference/constructor.wat @@ -1,8 +1,8 @@ (module $reference_test.wasm (type (;0;) (func (result i32))) - (type (;1;) (func (param i32))) - (func $classconstructor_new (;0;) (type 0) (result i32)) - (func $__wbg_classconstructor_free (;1;) (type 1) (param i32)) + (type (;1;) (func (param i32 i32))) + (func $__wbg_classconstructor_free (;0;) (type 1) (param i32 i32)) + (func $classconstructor_new (;1;) (type 0) (result i32)) (memory (;0;) 17) (export "memory" (memory 0)) (export "__wbg_classconstructor_free" (func $__wbg_classconstructor_free)) diff --git a/src/lib.rs b/src/lib.rs index f09bec2d213..d0cee09c6b0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1610,6 +1610,98 @@ pub mod __rt { ); } + if_std! { + use std::rc::Rc; + + /// A type that encapsulates an `Rc>` as well as a `Ref` + /// to the contents of that `WasmRefCell`. + /// + /// The `'static` requirement is an unfortunate consequence of how this + /// is implemented. + pub struct RcRef { + // The 'static is a lie. + // + // We could get away without storing this, since we're in the same module as + // `WasmRefCell` and can directly manipulate its `borrow`, but I'm considering + // turning it into a wrapper around `std`'s `RefCell` to reduce `unsafe` in + // which case that would stop working. This also requires less `unsafe` as is. + // + // It's important that this goes before `Rc` so that it gets dropped first. + ref_: Ref<'static, T>, + _rc: Rc>, + } + + impl RcRef { + pub fn new(rc: Rc>) -> Self { + let ref_ = unsafe { (*Rc::as_ptr(&rc)).borrow() }; + Self { _rc: rc, ref_ } + } + } + + impl Deref for RcRef { + type Target = T; + + #[inline] + fn deref(&self) -> &T { + &self.ref_ + } + } + + impl Borrow for RcRef { + #[inline] + fn borrow(&self) -> &T { + &self.ref_ + } + } + + /// A type that encapsulates an `Rc>` as well as a + /// `RefMut` to the contents of that `WasmRefCell`. + /// + /// The `'static` requirement is an unfortunate consequence of how this + /// is implemented. + pub struct RcRefMut { + ref_: RefMut<'static, T>, + _rc: Rc>, + } + + impl RcRefMut { + pub fn new(rc: Rc>) -> Self { + let ref_ = unsafe { (*Rc::as_ptr(&rc)).borrow_mut() }; + Self { _rc: rc, ref_ } + } + } + + impl Deref for RcRefMut { + type Target = T; + + #[inline] + fn deref(&self) -> &T { + &self.ref_ + } + } + + impl DerefMut for RcRefMut { + #[inline] + fn deref_mut(&mut self) -> &mut T { + &mut self.ref_ + } + } + + impl Borrow for RcRefMut { + #[inline] + fn borrow(&self) -> &T { + &self.ref_ + } + } + + impl BorrowMut for RcRefMut { + #[inline] + fn borrow_mut(&mut self) -> &mut T { + &mut self.ref_ + } + } + } + if_std! { use std::alloc::{alloc, dealloc, realloc, Layout}; diff --git a/tests/wasm/classes.js b/tests/wasm/classes.js index 49bd4472210..5317dbb0137 100644 --- a/tests/wasm/classes.js +++ b/tests/wasm/classes.js @@ -46,7 +46,7 @@ exports.js_exceptions = () => { // TODO: throws because it tries to borrow_mut, but the throw_str from the previous line doesn't clean up the // RefMut so the object is left in a broken state. // We still try to call free here so the object is removed from the FinalizationRegistry when weak refs are enabled. - assert.throws(() => b.free(), /recursive use of an object/); + assert.throws(() => b.free(), /attempted to take ownership/); let c = wasm.ClassesExceptions1.new(); let d = wasm.ClassesExceptions2.new(); diff --git a/tests/wasm/gc.js b/tests/wasm/gc.js new file mode 100644 index 00000000000..8a143e7c581 --- /dev/null +++ b/tests/wasm/gc.js @@ -0,0 +1,88 @@ +const wasm = require("wasm-bindgen-test.js"); +const assert = require("assert"); + +async function gc() { + if ("gc" in global) { + global.gc(); + // Give the `FinalizationRegistry` callbacks a chance to run. + await new Promise((resolve) => setTimeout(resolve, 0)); + } else { + console.warn("test runner doesn't expose GC function"); + } +} + +let dropCount = 0; +exports.drop_callback = () => dropCount += 1; + +exports.owned_methods = async () => { + dropCount = 0; + new wasm.OwnedValue(1).add(new wasm.OwnedValue(2)).n(); + + // The `OwnedValue`s should have been dropped as a result of `add` and `n` + // taking ownership of `self`... + assert.strictEqual(dropCount, 3); + await gc(); + // ... and GCing shouldn't double-free them. + assert.strictEqual(dropCount, 3); +}; + +// Make sure that objects created via. builders get GC'd properly. +exports.gc_builder = async () => { + dropCount = 0; + wasm.OwnedValue.build(1); + + await gc(); + assert.strictEqual(dropCount, 1); +}; + +// Make sure that objects created via. constructors get GC'd properly. +exports.gc_constructor = async () => { + dropCount = 0; + new wasm.OwnedValue(1); + + await gc(); + assert.strictEqual(dropCount, 1); +}; + +// Make sure that exported Rust types don't get GC'd while they're still in use +// by an async function. +exports.no_gc_fn_argument = async () => { + dropCount = 0; + let resolve; + + // It seems like we have to create the `OwnedValue` inside another function in + // order for the GC to see it as unused. + const createPromise = () => + wasm.borrow_and_wait( + new wasm.OwnedValue(1), + new Promise((x) => resolve = x), + ); + const promise = createPromise(); + + await gc(); + assert.strictEqual(dropCount, 0); + + resolve(); + await promise; + await gc(); + assert.strictEqual(dropCount, 1); +}; + +exports.no_gc_method_receiver = async () => { + dropCount = 0; + let resolve; + + const createPromise = () => + new wasm.OwnedValue(1).borrow_and_wait( + new Promise((x) => resolve = x), + ); + const promise = createPromise(); + + await gc(); + assert.strictEqual(dropCount, 0); + + resolve(); + await promise; + await gc(); + assert.strictEqual(dropCount, 1); +}; diff --git a/tests/wasm/gc.rs b/tests/wasm/gc.rs new file mode 100644 index 00000000000..bda2065c5c3 --- /dev/null +++ b/tests/wasm/gc.rs @@ -0,0 +1,68 @@ +use js_sys::Promise; +use wasm_bindgen::prelude::*; +use wasm_bindgen_futures::JsFuture; +use wasm_bindgen_test::*; + +#[wasm_bindgen(module = "tests/wasm/gc.js")] +extern "C" { + fn drop_callback(); + + async fn owned_methods(); + async fn gc_builder(); + async fn gc_constructor(); + async fn no_gc_fn_argument(); + async fn no_gc_method_receiver(); +} + +#[wasm_bindgen] +pub struct OwnedValue { + pub n: f64, +} + +#[wasm_bindgen] +impl OwnedValue { + #[wasm_bindgen(constructor)] + pub fn new(n: f64) -> Self { + Self { n } + } + + pub fn build(n: f64) -> Self { + Self::new(n) + } + + #[allow(clippy::should_implement_trait)] // traits unsupported by wasm_bindgen + pub fn add(self, other: OwnedValue) -> Self { + Self { + n: self.n + other.n, + } + } + + pub fn n(self) -> f64 { + self.n + } + + pub async fn borrow_and_wait(&self, promise: Promise) { + let _ = JsFuture::from(promise).await; + } +} + +impl Drop for OwnedValue { + fn drop(&mut self) { + drop_callback(); + } +} + +#[wasm_bindgen] +pub async fn borrow_and_wait(_: &OwnedValue, promise: Promise) { + let _ = JsFuture::from(promise).await; +} + +#[wasm_bindgen_test] +#[ignore = "flaky"] +async fn test_all() { + owned_methods().await; + gc_builder().await; + gc_constructor().await; + no_gc_fn_argument().await; + no_gc_method_receiver().await; +} diff --git a/tests/wasm/main.rs b/tests/wasm/main.rs index 00e58de56a6..6272352c2e9 100644 --- a/tests/wasm/main.rs +++ b/tests/wasm/main.rs @@ -29,6 +29,7 @@ pub mod enums; #[path = "final.rs"] pub mod final_; pub mod futures; +pub mod gc; pub mod getters_and_setters; pub mod import_class; pub mod imports; @@ -43,7 +44,6 @@ pub mod no_shims; pub mod node; pub mod option; pub mod optional_primitives; -pub mod owned; pub mod result; pub mod result_jserror; pub mod rethrow; diff --git a/tests/wasm/owned.js b/tests/wasm/owned.js deleted file mode 100644 index e1d5dfbe1a5..00000000000 --- a/tests/wasm/owned.js +++ /dev/null @@ -1,13 +0,0 @@ -const wasm = require("wasm-bindgen-test.js"); - -exports.create_garbage = async function () { - for (let i = 0; i < 100; i++) { - new wasm.OwnedValue(1).add(new wasm.OwnedValue(2)).n(); - } - - if ("gc" in global) { - global.gc(); - } else { - console.warn("test runner doesn't expose GC function"); - } -}; diff --git a/tests/wasm/owned.rs b/tests/wasm/owned.rs deleted file mode 100644 index 97e7d6fda3c..00000000000 --- a/tests/wasm/owned.rs +++ /dev/null @@ -1,36 +0,0 @@ -use wasm_bindgen::prelude::*; -use wasm_bindgen_test::*; - -#[wasm_bindgen] -pub struct OwnedValue { - pub n: f64, -} - -#[wasm_bindgen] -impl OwnedValue { - #[wasm_bindgen(constructor)] - pub fn new(n: f64) -> Self { - Self { n } - } - - #[allow(clippy::should_implement_trait)] // traits unsupported by wasm_bindgen - pub fn add(self, other: OwnedValue) -> Self { - Self { - n: self.n + other.n, - } - } - - pub fn n(self) -> f64 { - self.n - } -} - -#[wasm_bindgen(module = "tests/wasm/owned.js")] -extern "C" { - fn create_garbage(); -} - -#[wasm_bindgen_test] -fn test_create_garbage() { - create_garbage() -}