Fix Rust values getting GC'd while still borrowed and not getting GC'd if created via. constructor

CHANGELOG.md

@@ -36,6 +36,12 @@
 * Fix MSRV compilation.
   [#3927](https://github.com/rustwasm/wasm-bindgen/pull/3927)
 
+* 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::option::Option::{Some, None};
                     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::into_inner(rc) {
+                        Some(cell) => cell.into_inner(),
+                        None => #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

@@ -1039,7 +1039,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
   (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
   (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))

crates/shared/src/lib.rs

@@ -6,7 +6,7 @@ mod schema_hash_approval;
 // This gets changed whenever our schema changes.
 // At this time versions of wasm-bindgen and wasm-bindgen-cli are required to have the exact same
 // SCHEMA_VERSION in order to work together.
-pub const SCHEMA_VERSION: &str = "0.2.92";
+pub const SCHEMA_VERSION: &str = "0.2.93";
 
 #[macro_export]
 macro_rules! shared_api {

crates/shared/src/schema_hash_approval.rs

@@ -8,7 +8,7 @@
 // If the schema in this library has changed then:
 //  1. Bump the version in `crates/shared/Cargo.toml`
 //  2. Change the `SCHEMA_VERSION` in this library to this new Cargo.toml version
-const APPROVED_SCHEMA_FILE_HASH: &str = "10197913343580353876";
+const APPROVED_SCHEMA_FILE_HASH: &str = "16355783286637101026";
 
 #[test]
 fn schema_version() {

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