TryFromJsValue::try_from_js_value() invalidates to converted value on failure

[Tim-Evans-Seequent]

Describe the Bug

I wanted to have function that accepts a JsValue argument and checked at run-time which of several #[wasm_bindgen] structs it was and does something slightly different for each one. I instead got a hard to diagnose panic in a different crate caused by ::js_sys::Reflect::set returning a leftover error.

Steps to Reproduce

The code I wrote looked like this:

    if let Ok(x) = FirstStruct::try_from_js_value(value.clone()) {
        ...
    } else if let Ok(mesh) = SecondStruct::try_from_js_value(value.clone()) {
        ...
    } else {
        return Err(JsError::new("should be FirstStruct or SecondStruct"))
    }

Shortly after this I made some wgpu calls, and one of them panicked out of nowhere on an assertion that said "setting properties should never fail".

Changing the order of the types checked fixed the problem, until I tried the other type.

Expected Behavior

Any error set by try_from_js_value should be contained in its return value, and not be able to affect other code. I should not be required to return an error when try_from_js_value fails; checking multiple types should be a valid.

Alternatively, this pattern should be prevented by the compiler somehow, or worst case documented as causing problems.

Actual Behavior

A subsequent call to js_sys::Reflect::set inside wgpu::backend::webgpu::webgpu_sys::gen_GpuBufferDescriptor::GpuBufferDescriptor::size failed and caused a panic. This assertion is debug only, in release mode I instead got an error from WebGPU due to the missing field.

This was running on Chrome on Windows in case that matters.

[tekacs]

I ran into this in an identical way recently, so to jump in as well --

In fairness when I did look this up I realized that there is this warning on docs.rs on this module and on TryFromJsValue and that's when I moved away from it, but this still resulted in UB-esque action-at-a-distance issues that were a nuisance to track down.

If I understood this correctly, the issue seems to be that... with the way that try_from_js_value is generated by the proc macro, it 'consumes' the JsValue that it's passed in a way that's destructive to its inner self.

That ends up meaning that if you clone a JsValue or turn an &JsValue into a JsValue with clone and then use try_from_js_value on it, you've potentially invalidated all other copies of that value. I have seen a JsValue spontaneously decaying into null after being passed into a function call on the Rust side, for example.

Below is the generated code for the moving parts in this flow. Initially I wondered if the Rc was being double-freed when a JsValue is cloned and used with this method, but __wbg_label_unwrap and Label.__unwrap do appear to work to short-circuit the path with the JS instanceof check before it touches the JsValue's underlying Rc.

I too would love to see a solution for this, because as far as I'm aware there is no other way to take back or clone out a Rust type passed to JS at present, nor even to check its type like this.

My temporary solution was a switch statement with a number of instanceof checks on the JS side, so I do know that the instanceof checks work as expected on the values which were being sent down this path.

#[automatically_derived]
impl wasm_bindgen::convert::TryFromJsValue for Label {
    type Error = wasm_bindgen::JsValue;
    fn try_from_js_value(
        value: wasm_bindgen::JsValue,
    ) -> wasm_bindgen::__rt::core::result::Result<Self, Self::Error> {
        let idx = wasm_bindgen::convert::IntoWasmAbi::into_abi(&value);
        #[link(wasm_import_module = "__wbindgen_placeholder__")]
        #[cfg(all(target_arch = "wasm32", target_os = "unknown"))]
        extern "C" {
            fn __wbg_label_unwrap(ptr: u32) -> u32;

        }
        #[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))]
        unsafe fn __wbg_label_unwrap(_: u32) -> u32 {
            {
                core::panicking::panic_fmt(core::const_format_args!(
                    "cannot convert from JsValue outside of the Wasm target"
                ));
            }
        }
        let ptr = unsafe { __wbg_label_unwrap(idx) };
        if ptr == 0 {
            wasm_bindgen::__rt::core::result::Result::Err(value)
        } else {
            #[allow(clippy::mem_forget)]
            wasm_bindgen::__rt::core::mem::forget(value);
            unsafe {
                wasm_bindgen::__rt::core::result::Result::Ok(
                    <Self as wasm_bindgen::convert::FromWasmAbi>::from_abi(ptr),
                )
            }
        }
    }
}


#[automatically_derived]
impl wasm_bindgen::convert::FromWasmAbi for Label {
    type Abi = u32;
    unsafe fn from_abi(js: u32) -> Self {
        use wasm_bindgen::__rt::alloc::rc::Rc;
        use wasm_bindgen::__rt::core::result::Result::{Err, Ok};
        use wasm_bindgen::__rt::{assert_not_null, WasmRefCell};
        let ptr = js as *mut WasmRefCell<Label>;
        assert_not_null(ptr);
        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",
            ),
        }
    }
}

export function __wbg_label_unwrap() { return logError(function (arg0) {
    const ret = Label.__unwrap(arg0);
    _assertNum(ret);
    return ret;
}, arguments) };

export class Label {

    constructor() {
        throw new Error('cannot invoke `new` directly');
    }

    static __wrap(ptr) {
        ptr = ptr >>> 0;
        const obj = Object.create(Label.prototype);
        obj.__wbg_ptr = ptr;
        LabelFinalization.register(obj, obj.__wbg_ptr, obj);
        return obj;
    }

    static __unwrap(jsValue) {
        if (!(jsValue instanceof Label)) {
            return 0;
        }
        return jsValue.__destroy_into_raw();
    }

    __destroy_into_raw() {
        const ptr = this.__wbg_ptr;
        this.__wbg_ptr = 0;
        LabelFinalization.unregister(this);
        return ptr;
    }

    free() {
        const ptr = this.__destroy_into_raw();
        wasm.__wbg_label_free(ptr, 0);
    }

    // ... snip
}

[daxpedda]

I assume you just didn't know about the JsCast trait? This should address your use case quite well I hope.

In any case, I assume the issue is resolved. Let me know if you are having trouble using JsCast or if it doesn't do what you want.

[Tim-Evans-Seequent]

@daxpedda JsCast is not automatically implemented for #[wasm_bindgen] wrapped Rust structs. I also can't see how I would go about implementing it myself. I don't understand how it could be used to solve this problem.

Are you suggesting that I need to re-import the generated JavaScript type and check against that, then use try_from_js_value only after confirming that the type matches? If so, it would be nice to have an example in the docs of how to do that.

To expand on what I'm trying to do I have an enum like this in Rust:

pub enum Shape {
    Square {
        side_length: f64,
    }
    Circle {
        radius: f64,
    }
}

The pattern I want to use is one I've used before when writing Python bindings: create a type for each variant, and use run-time type checks to work out which variant an argument is. So I create two wrapped type like these:

#[wasm_bindgen]
struct SquareShape {
    side_length: f64,
}

#[wasm_bindgen]
struct CircleShape {
    radius: f64,
}

Then in a wrapped function I want to determine which one a value is. What I wanted was something that would look like this:

#[wasm_bindgen]
pub fn do_something_with_shape(shape: JsValue) -> Result<(), JsError> {
    let real_shape = if let Some(square) = cast_ref::<SquareShape>(&shape) {
        wrapped_crate::Shape::Square {
            side_length: square.side_length,
        }
    } else if let Some(circle) = cast_ref::<CircleShape>(&shape) {
        wrapped_crate::Shape::Circle {
            radius: circle.radius,
        }
    } else {
        return Err(...);
    };
    wrapped_crate::do_something_with_shape(real_shape);
    Ok(JsError::new("expected a SquareShape or CircleShape"))
}

The gap here is what is cast_ref? I assumed try_from_js_value was it, but instead I got a hard to diagnose bug. JsCast doesn't seem to be it either, because it's not implemented on SquareShape or CircleShape.

[daxpedda]

Apologies, I didn't take a close look at your issue before responding.
Rather this is a duplicate of #2231.

You should be able to find more information and some workarounds there.

[Tim-Evans-Seequent]

@daxpedda I feel like you've still misunderstood my intentions when I created this issue.

This is not a feature request, or a question like #2231. I have read that case; in fact it's what suggested that TryFromJsValue was a useful tool for doing this.

It is a bug report for TryFromJsValue::try_from_js_value seeming to behave in ways that cause hard-to-find bugs. I was hoping to save others from spending the time on this that I did.

Assuming that it isn't easy to fix, how about changing the documentation from "no stability guarantees" to something like "don't call this yourself", or explaining that catching the error isn't a good idea? I realise that if this is, in fact, caused by the .clone() as the other commenter suggested then it may be harder to explain that that.

[daxpedda]

Assuming that it isn't easy to fix, how about changing the documentation from "no stability guarantees" to something like "don't call this yourself", or explaining that catching the error isn't a good idea? I realise that if this is, in fact, caused by the .clone() as the other commenter suggested then it may be harder to explain that that.

We might want to simply move it to the __rt module instead of leaving it in convert. I don't know the historical context about why we decided to expose the convert module in the first place, but I think it was a mistake putting TryFromJsValue in there.

However, the way this API is currently implemented is very easy to mis-use and the signature should change or it should simply be marked as unsafe, internal or not.

Ideally, we actually address #2231 and fix TryFromJsValue up and expose it publicly. However, the API design isn't clear yet. See the discussions in #3709.

---

Thank you for your persistence here, my apologies for not more carefully reading your responses!