Fix Rust values getting GC'd while still borrowed and not getting GC'…

…d if created via. constructor (#3940)

* Add a failing test

* Force tests to run sequentially

At first I tried fixing them so that they didn't need to (giving each of them
their own `dropCount`), but running multiple GCs in parallel seems to be flaky.

* Add objects created via. constructors to the FinalizationRegistry

* Add a failing test

* Fix exported Rust types being GC'd while still borrowed

I also discovered and fixed an extra bug while working on this, which
was that `LongRefFromWasmAbi` wasn't getting used for `self`: this bug
didn't cause any problems before, because the only type that had a
different `LongRefFromWasmAbi` impl than its `RefFromWasmAbi` impl was
`JsValue` which can't be the type of `self`.

It became a problem here because I made the new `LongRefFromWasmAbi`
impl for exported Rust types clone the `Rc`, whereas the
`RefFromWasmAbi` impl doesn't because garbage collection can't occur
during the synchronous call that the value has to live for.

I might actually change it so that both of the impls behave like the
current `LongRefFromWasmAbi` impl, though: cloning an `Rc` isn't
expensive and so having the second different impl just makes things more
complicated for no good reason. I just left it in this commit as
explanation for how I discovered the `LongRefFromWasmAbi` issue.

* Unify RefFromWasmAbi and LongRefFromWasmAbi impls

* Get rid of needless looping

* Get rid of outdated `borrow_mut`

Now that borrowing a Rust value always clones its `Rc`, `Rc::into_inner`
is a sufficient check that the value isn't borrowed.

* Get rid of needless `mut`

For some reason I was getting errors before without it, but that seems
to be fixed now. (It's probably something to do with having removed the
`borrow_mut`, but that only takes `&self`, so I still don't get it.)

* Update reference tests

* Add changelog entry

* Update schema hash

* Use Rc::try_unwrap instead of Rc::into_inner

* Skip GC tests

They seem to be far flakier in CI than locally for some reason, and I
don't see any way to solve it; so just turn them off. :(

I also got rid of the weird GC warmup hack because it doesn't do
anything anymore; I could've sworn it was a reproducible effect before,
but it seems to make no difference now.

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)

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,

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 {
                     <Self as #wasm_bindgen::convert::RefFromWasmAbi>::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 }
             }

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 => {

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),

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}

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))

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;
     }
 }

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))

src/lib.rs

@@ -1610,6 +1610,98 @@ pub mod __rt {
         );
     }
 
+    if_std! {
+        use std::rc::Rc;
+
+        /// A type that encapsulates an `Rc<WasmRefCell<T>>` 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<T: ?Sized + 'static> {
+            // 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<WasmRefCell<T>>,
+        }
+
+        impl<T: ?Sized> RcRef<T> {
+            pub fn new(rc: Rc<WasmRefCell<T>>) -> Self {
+                let ref_ = unsafe { (*Rc::as_ptr(&rc)).borrow() };
+                Self { _rc: rc, ref_ }
+            }
+        }
+
+        impl<T: ?Sized> Deref for RcRef<T> {
+            type Target = T;
+
+            #[inline]
+            fn deref(&self) -> &T {
+                &self.ref_
+            }
+        }
+
+        impl<T: ?Sized> Borrow<T> for RcRef<T> {
+            #[inline]
+            fn borrow(&self) -> &T {
+                &self.ref_
+            }
+        }
+
+        /// A type that encapsulates an `Rc<WasmRefCell<T>>` 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<T: ?Sized + 'static> {
+            ref_: RefMut<'static, T>,
+            _rc: Rc<WasmRefCell<T>>,
+        }
+
+        impl<T: ?Sized> RcRefMut<T> {
+            pub fn new(rc: Rc<WasmRefCell<T>>) -> Self {
+                let ref_ = unsafe { (*Rc::as_ptr(&rc)).borrow_mut() };
+                Self { _rc: rc, ref_ }
+            }
+        }
+
+        impl<T: ?Sized> Deref for RcRefMut<T> {
+            type Target = T;
+
+            #[inline]
+            fn deref(&self) -> &T {
+                &self.ref_
+            }
+        }
+
+        impl<T: ?Sized> DerefMut for RcRefMut<T> {
+            #[inline]
+            fn deref_mut(&mut self) -> &mut T {
+                &mut self.ref_
+            }
+        }
+
+        impl<T: ?Sized> Borrow<T> for RcRefMut<T> {
+            #[inline]
+            fn borrow(&self) -> &T {
+                &self.ref_
+            }
+        }
+
+        impl<T: ?Sized> BorrowMut<T> for RcRefMut<T> {
+            #[inline]
+            fn borrow_mut(&mut self) -> &mut T {
+                &mut self.ref_
+            }
+        }
+    }
+
     if_std! {
         use std::alloc::{alloc, dealloc, realloc, Layout};
 

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();