RefFromWasmAbi causes panic in dev builds but not in prod

[fhilgers]

When compiling the wasm code via wasm-back (wasm-pack build --target nodejs --dev), passing a reference of a tsify derived type results in a panic:

error: Uncaught (in promise) RuntimeError: unreachable
    at __rust_start_panic (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3644715)
    at rust_panic (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3643380)
    at std::panicking::rust_panic_with_hook::h71ba58a1616d777a (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:2845421)
    at std::panicking::begin_panic_handler::{{closure}}::h0bd7a2536d7b59e3 (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3067647)
    at std::sys_common::backtrace::__rust_end_short_backtrace::hbe77ead45e850971 (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3644569)
    at rust_begin_unwind (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3544857)
    at core::panicking::panic_fmt::h2d38c77886454fe8 (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3579401)
    at <wasm_bindgen::JsValue as core::ops::drop::Drop>::drop::hd52544ba4a9dace2 (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:2536813)
    at core::ptr::drop_in_place<wasm_bindgen::JsValue>::hb1756d1c617b98c5 (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3605920)
    at core::ptr::drop_in_place<qrcloak_bindings::_::JsType>::hacadc9faad1857ca (wasm://wasm/qrcloak_bindings.wasm-012e668a:1:3581226)

Note that compiling without --dev option results in working code.

Minimal rust code to reproduce my issue:

#[derive(Tsify, Serialize, Deserialize)]
#[tsify(into_wasm_abi, from_wasm_abi)]
pub struct Foo {
    bar: String
}

#[wasm_bindgen]
pub fn take_foo(blubs: &Foo) {}

with the following js snippet:

import { take_foo } from "qrcloak-bindings";

let foo = { bar: "test" };
take_foo(foo);

I tested with both stable (rustc 1.75.0) and nightly (rustc 1.78.0-nightly).

[fhilgers]

Okay, when I looked at the implementation of the generated JsType, RefFromWasmAbi is defined the in the following way:

impl RefFromWasmAbi for JsType {
    type Abi = <JsValue as RefFromWasmAbi>::Abi;
    type Anchor = core::mem::ManuallyDrop<JsType>;
    #[inline]
    unsafe fn ref_from_abi(js: Self::Abi) -> Self::Anchor {
        let tmp = <JsValue as RefFromWasmAbi>::ref_from_abi(js);
            core::mem::ManuallyDrop::new(JsType {
                obj: core::mem::ManuallyDrop::into_inner(tmp).into(),
            })
        }
    }
}

But the generated RefFromWasmAbi for the derived types just calls Self::from_abi which in turn calls JsType::from_abi, passes a reference of JsType to Tsify::from_js and drops the JsType afterwards. But JsType is just a wrapper with property obj: JsValue. This JsValue is then dropped, trying to free it from the wasm heap, but it is not there because called the function with a reference on the js side.

See the following difference in the generated js:

module.exports.take_foo = function(blubs) {
    wasm.take_foo(addHeapObject(blubs));
};

module.exports.take_foo_ref = function(blubs) {
    try {
        wasm.take_foo_ref(addBorrowedObject(blubs));
    } finally {
        heap[stack_pointer++] = undefined;
    }
};

In the reference case, js is responsible for the heap and in the other case, rust is responsible.

impl Drop for JsValue {
    #[inline]
    fn drop(&mut self) {
        unsafe {
            // We definitely should never drop anything in the stack area
            debug_assert!(self.idx >= JSIDX_OFFSET, "free of stack slot {}", self.idx);

            // Otherwise if we're not dropping one of our reserved values,
            // actually call the intrinsic. See #1054 for eventually removing
            // this branch.
            if self.idx >= JSIDX_RESERVED {
                __wbindgen_object_drop_ref(self.idx);
            }
        }
    }
}

My proposed fix would be implementing RefFromWasmAbi without dropping JsType:

diff --git a/tsify-next-macros/src/wasm_bindgen.rs b/tsify-next-macros/src/wasm_bindgen.rs
index d179d34..45dd823 100644
--- a/tsify-next-macros/src/wasm_bindgen.rs
+++ b/tsify-next-macros/src/wasm_bindgen.rs
@@ -171,7 +171,11 @@ fn expand_from_wasm_abi(cont: &Container) -> TokenStream {
             type Anchor = SelfOwner<Self>;
 
             unsafe fn ref_from_abi(js: Self::Abi) -> Self::Anchor {
+                let result = Self::from_js(&*JsType::ref_from_abi(js));
+                if let Err(err) = result {
+                    wasm_bindgen::throw_str(err.to_string().as_ref());
+                }
+                SelfOwner(result.unwrap_throw())
-                SelfOwner(Self::from_abi(js))
             }
         }
     }

Edit: the diff was in the wrong order.

[Pantamis]

Really interesting issue, I learn a lot by reading your very well-written report of it !

Is the debug_assert the reason why we only get the compile error with --dev option ?

Maybe @cwfitzgerald could be interested by this :)

[fhilgers]

Yes the debug_assert is most likely the reason why it only crashes in dev.
Thankfully the drop function does not free in that case regardless of the build profile, because it checks the index afterwards again.

Edit: Now that I looked at it again I realized that it checks two different indeces. I am not sure whether the stack is corrupted in release mode when passing tsify references, I might try this out later.

[fhilgers]

Alright, so my first intuition was right, in release mode the drop does nothing in this case as the indeces are defined like this:

const JSIDX_OFFSET: u32 = 128; // keep in sync with js/mod.rs
const JSIDX_UNDEFINED: u32 = JSIDX_OFFSET;
const JSIDX_NULL: u32 = JSIDX_OFFSET + 1;
const JSIDX_TRUE: u32 = JSIDX_OFFSET + 2;
const JSIDX_FALSE: u32 = JSIDX_OFFSET + 3;
const JSIDX_RESERVED: u32 = JSIDX_OFFSET + 4;

[Pantamis]

Ok at least there is that, no double free !

Your proposal makes a lot of sense to me but it would be great to have the input of @cwfitzgerald to be sure.

I guess the worst that could happen if the reasoning is wrong is a memory leak 😌

[Pantamis]

By the way:

It seems that the current implementation of RefFromWasmAbi could actually be the correct implementation of LongRefFromWasmAbi. I don't really get the difference between the two but looking at the implementation of the two traits for JsValue in wasm-bindgen suggest me that.

impl RefFromWasmAbi for JsValue {
    type Abi = u32;
    type Anchor = ManuallyDrop<JsValue>;

    #[inline]
    unsafe fn ref_from_abi(js: u32) -> Self::Anchor {
        ManuallyDrop::new(JsValue::_new(js))
    }
}

impl LongRefFromWasmAbi for JsValue {
    type Abi = u32;
    type Anchor = JsValue;

    #[inline]
    unsafe fn long_ref_from_abi(js: u32) -> Self::Anchor {
        Self::from_abi(js)
    }
}

[fhilgers]

Should we leave this open until #35 is merged or just close it as there is also #34?

[siefkenj]

Since it causes a runtime error, let's leave this open for now. It seems like more than a missing feature.

[fhilgers]

I fixed this RuntimeError in #33 or did I miss something?

[siefkenj]

Fixed by #33

My mistake. I assumed this issue would be auto-closed by github!