Fix fn () -> Result<T, JsValue> leaking stack space

[cormacrelf]

Fixes #1963

Overview

• No longer implements -> Result<T, JsValue> by throwing it from within the wasm stack frame
• Instead, defers the throw until the outer glue code's return path
• Expands the acceptable return-position types to Result<T, E> where T: IntoWasmAbi, E: Into<JsValue>, so you can easily throw your own custom error classes imported from JS
• Adds a new JsError that makes the common need for throw new Error("message") easier to use. It converts automatically from std::error::Error for use with ?.

Basically, while you can continue using -> Result<T, JsValue> and it will no longer leak, this is overall much better at its job:

 #[wasm_bindgen]
-fn exported_fn() -> Result<i32, JsValue> {
+fn exported_fn() -> Result<i32, JsError> {
     let v = [0u8; 1000];
-    Err("string errors are terrible anyway".into())
+    Err(JsError::new("error message"))
 }

You can also use ? with any std::error::Error implementation, e.g. anyhow::Error, using thiserror on your own types, or just adding impl core::fmt::Display for MyType { ... } impl std::error::Error for MyType {}.

The remaining questions are

• What to do about existing Result<T, JsValue> where T: IntoWasmAbi. That's distinct from T: Into<JsValue> (for example, #[wasm_bindgen] enum A { ... } does not implement Into<JsValue>), so you can't just change the impl to T: Into<JsValue>.
  • You could just deprecate it. It leaks. Throwing non-Error objects is a terrible anti-pattern anyway.
• What to do about async. I think just supporting JsError in exported async function signatures with minimal conversion should be enough.
• How to support custom error classes.
  • You probably want this to make your errors catchable separately from eg a wasm RuntimeError.
  • You would have to expose a JsError constructor that converts from a JsValue without implementing From<JsValue> lest ? convert any old JsValue without you meaning to.
  • You might want a way to annotate on an imported class that it happens to subclass Error.
  • Very hard to get ? conversion perfectly on these while also allowing conversion from T: std::error::Error. You could pick one or the other. Or you could add a second type JsErrorSubclass or something, but that's getting a bit much.
• Documentation on the new types
• Figure out how it might fit in with the Exception Handling proposal. (Implementations)

Obsolete

Overview (obsolete)

• new type JsError equivalent to js_sys::Error that implements conversions from T where T: std::error::Error using T::to_string()
• impl ReturnWasmAbi for Result<T, JsError> distinct from the Result<T, JsValue>, that just converts the JsError to a JsValue and returns it instead of using wasm_bindgen::throw_val(e)
• adds a function throwIfError(x) { if (x instanceof Error) { throw x } }
• Modifies the glue code to let tmp0 = takeObject(...); throwIfError(tmp0); return tmp0

Notes

This is more lightweight than the WasmResult I described in the issue. It only works for Result types with T: Into<JsValue> + WasmDescribe and E = JsError. The outcome is similar to WasmResult in that every result return involves creating a JsValue (ok.into() or JsError::from something, e.g. via ?) but it does not require a whole WasmResult class in the glue code. The glue code just checks if the returned heap object happens to be an instanceof Error and throws the error if so. You get a reasonably good stack trace because the Error was created in a Rust stack frame.

[cormacrelf]

Hm, actually I think the original WasmResult class idea was better and doesn't have the limitations this does/works for current code without changes. But this is is a better starting point than using macro-expanded wasm_bindgen output in the wasm_bindgen crate, or something.

The tricky thing is getting arbitrary T: IntoWasmAbi into the constructor for such an object. We are no longer returning T but passing it as an argument. You need one constructor variant for each uniquely WasmDescribed T you wish to return in a result, which will do whatever a plain return would do in the glue code, wraps it in a WasmResult, and returns it to wasm, which then returns this JsValue to JS.

[alexcrichton]

Thanks for putting this together! I'm trying to understand this at a high-level before diving too much into the details, and one of the things I'm wondering is whether this is a breaking change? It sounds like this is funneling returns into JS values/objects where it wasn't before, or this is otherwise limiting return values which previously weren't limited before?

[cormacrelf]

Thanks for having a look. I owe a lot to this library (my job!) and I hope I can give back.

As it stands, this is not a breaking change. It only applies when you use JsError instead of JsValue, and JsError is a new Rust type so nobody is using it yet. The added test file notes that the old style Result<T, JsValue> will still leak stack space, but it does still "work" unchanged. The new style has some differences in terms of what you can return, but it's a voluntary upgrade.

However, fixing the old style results as well would require first adding the original WasmResult idea instead of checking x instanceof Error, and then either:

1. A convoluted system for converting these T: IntoWasmAbi into JsValues; or
2. Changing the API, so you can only return T: Into<JsValue>.

Like the JsError version, both options involve converting to JsValue before returning, they only differ in how. If anyone wants to build a solution using multiple return values, they are welcome to and it would probably be the best implementation by miles, but my own requirements include targeting an ancient version of Firefox, so I can't use that and really neither can anyone else yet.

I have built most of option 1, will probably PR separately so you can see it and weigh up whether a breaking change is worthwhile. Some general observations about them:

• With option 1, you'd be adding a lot of generated JS shims. It's quite awkward from a macro / codegen perspective. You can avoid some of these by hard coding a list of syn::Typea that are known to implement Into<JsValue>. That can't work for any other types eg RustStructs.
• Option 1's treatment of RustStruct in the glue code is difficult and I'm still working on that.
• Option 1 appears to imply the obsolescence of the ReturnWasmAbi trait ReturnWasmAbi::return_abi(self) function, because the codegen module has to special case Results in order to generate the into-jsvalue shims, and you cannot do that inside a trait impl / default function. It would be lovely to have one shim per type, but this is a Rust macro, the best you can really do is one shim per exported function. (Some discussion of generic externs on internals.)
• With option 2, The resulting difference between the type bounds on plain returned values and result-wrapped ones is a bit of an unpleasant implementation detail / limitation. (But also, Result is never going to perfectly match JS exceptions.)
• Performance is likely a wash, both options and even the JsError solution all involve one more WASM -> JS call than the leaky version, only difference is amount of JS glue spewed out. Option 1 spews most, JsError (which like option 2, uses Into<JsValue>) spews least.
• One more, a convincing one IMO, in terms of forward compatibility with multiple return values: you probably want to keep the T: IntoWasmAbi for when you can use it to write the first return value, and a separate (nullable) error in the second return value that the glue code simply checks first. If there is indeed a future for multiple return values in wasm_bindgen, then a bit of temporary pain through option 1 is probably fine as it could later be deleted.

[gthb]

👏 I wish more people explained their work so meticulously.

[alexcrichton]

Ok thanks for the explanation! It's good to know that this doesn't break things. I realize that the previous design isn't great but I would prefer to not break existing users.

Naively though what I would expect is that this:

impl<T: IntoWasmAbi> ReturnWasmAbi for Result<T, JsValue>

would probably change to

impl<T: IntoWasmAbi, U: Into<JsValue>> ReturnWasmAbi for Result<T, U>

and the "abi" would be something like:

#[repr(C)]
pub struct ResultAbi<T> {
    is_ok: u32,
    abi: ResultAbiUnion<T>,
}

#[repr(C)]
pub union ResultAbiUnion<T> {
    ok: T,
    err: u32,
}

where ok is determined by T and err is the index of the JsValue if present (and its what gets thrown). The changes you have here of altering Describe for Result<T, U> would stay where it becomes a more first-class thing that the generated glue handles (it'd read the result and use the integer discriminant to figure out what happened).

One of the things I'm worried about with the system you describe is something like Result<JsError, JsError> which isn't possible since it unconditionally gets thrown based on type information. I also think it would be best to allow the T of Result<T, _> to be anything as opposed to only other JS values.

[cormacrelf]

That's an interesting idea, it pretty neatly addresses the shim problem and the RustStruct problem at the same time. Nicely done.

Regarding the edge cases with JsError, I think simply removing any custom implementation and relying on the generic one would resolve them. (Edit, duh, this is what you mean.) JsError would then simply be a friendly helper type for creating Errors with no effect on ABI. I like that.

[alexcrichton]

Ah yeah I was hoping that with an explicit discriminant of sorts we don't have to check types and that way Result<JsError, JsError> would work well.

I think if this works out we can update most documentation and examples to having Result<T, js_sys::Error> instead of JsValue since JsValue is rarely what you'd want anyway.

[cormacrelf]

You reckon ditch the JsError entirely in favour of js_sys::Error? Move the convenient From<T> where T: std::Error::Error impl there? (Maybe it already has one.)

Also, as for custom JS error subclasses, with the error bound expanded to Into<JsValue> it becomes very easy to just implement that on your custom error enum and instantiate one there. That'll be really good too.

Final thing to address is async. Result<_, JsValue> is all over those definitions. Promise, async imports, async exports. Maybe the bound expansion could be applied elsewhere too. Probably best I have a crack implementing before I get carried away thinking about that, it'll be clearer once it's working. (But also, that can be done later.)

[alexcrichton]

Hm that's true, given how fundamental errors are I can see having them in wasm-bindgen itself would be quite useful. Ideally we could just move js_sys::Error into wasm-bindgen and then reexport it from js-sys, but I'm not sure if that would work out. I do think it would be best to avoid having wasm_bindgen::JsError and js_sys::Error, though, that sounds a little too confusing.

For async I would naively hope that the macro could have syntactic sugar which accepts Result<T, U> but all the runtime internals deal with Result<T, JsValue> or w/e they are today. I forget precisely how all the expansions work though and whether that is workable.

[cormacrelf]

Is it correct that JsValue could implement OptionIntoWasmAbi by returning 0? If you generate a rust struct, then it implements it this way. Is there a reason why JsValue doesn't already?

[cormacrelf]

Alright, this works overall pretty well! Assuming that I'm right about JsValue/OptionIntoWasmAbi. But there is was a bug in the existing stack return reading (LoadRetptr) that's been revealed.

wasm-bindgen/crates/cli-support/src/wit/mod.rs

Lines 1347 to 1358 in e447729

	if uses_retptr {
	let mem = ret.cx.memory()?;
	for (i, ty) in ret.input.into_iter().enumerate() {
	instructions.push(InstructionData {
	instr: Instruction::LoadRetptr { offset: i, ty, mem },
	stack_change: StackChange::Modified {
	pushed: 1,
	popped: 0,
	},
	});
	}
	}

wasm-bindgen/crates/cli-support/src/js/binding.rs

Lines 598 to 611 in e447729

	Instruction::LoadRetptr { ty, offset, mem } => {
	let (mem, size) = match ty {
	AdapterType::I32 => (js.cx.expose_int32_memory(*mem), 4),
	AdapterType::F32 => (js.cx.expose_f32_memory(*mem), 4),
	AdapterType::F64 => (js.cx.expose_f64_memory(*mem), 8),
	other => bail!("invalid aggregate return type {:?}", other),
	};
	// If we're loading from the return pointer then we must have pushed
	// it earlier, and we always push the same value, so load that value
	// here
	let expr = format!("{}()[retptr / {} + {}]", mem, size, offset);
	js.prelude(&format!("var r{} = {};", offset, expr));
	js.push(format!("r{}", offset));
	}

This gives you:

#[wasm_bindgen]
fn return_option_err() -> Result<Option<f64>, JsError> { ... }

module.exports.return_option_err = function() {
    try {
        const retptr = wasm.__wbindgen_add_to_stack_pointer(-32);
        wasm.return_option_err(retptr);
        var r0 = getInt32Memory0()[retptr / 4 + 0];
        var r1 = getFloat64Memory0()[retptr / 8 + 1];
        var r2 = getInt32Memory0()[retptr / 4 + 2];
        var err0 = r2;
        if (err0 !== 0) { throw takeObject(err0); }
        return r0 === 0 ? undefined : r1;
    } finally {
        wasm.__wbindgen_add_to_stack_pointer(32);
    }
};

Those offsets are nonsensical. We needed a miniature version of the C struct packing algorithm.

[alexcrichton]

Is it correct that JsValue could implement OptionIntoWasmAbi by returning 0?

For now this was explicitly omitted because otherwise bindings have no way of differentiating undefined and Some(undefined), they would both look like undefined when converted into a JsValue. This works for all other types that aren't JsValue, though, since undefined isn't a valid value for them. (hence Option<MyStruct>, when None, is represented as undefined)

---

Those offsets are nonsensical. We needed a miniature version of the C struct packing algorithm.

Hm yeah it's been a long time since I look at all this, but I think we can probably do some tricks to make this easy for us and not try to reimplement C struct layouts. For example the discriminant could be forced to a u64 to force 8-byte alignment of the rest of the fields, and loading the rest of the fields should hopefully just need to factor in a new offset. I think some low-level instructions may need tweaking to do this but I would hope that we could load the raw values with a new optionally-specified offset or something like that to handle this.

[cormacrelf]

Ah, I will flip it back to use a separate discriminant then.

the discriminant could be forced to a u64 to force 8-byte alignment of the rest of the fields

I chose to place the JsValue's u32 at the end and reduce the problem of getting its value off the stack down to pop(), so that was the field being misread. This turned out to simplify the struct unpacking too. The simple field aligner/unpacker I implemented can handle WasmAbi structs that satisfy:

• repr(C) of course
• u32/i32/f32/f64 leaf fields only of course
• layout equivalent to a completely flattened struct, constructed by an in order traversal of all the leaf fields in it. (This means that you can't embed struct A(u32, f64) as struct B(u32, A); because the "completely flattened" struct AB(u32, u32, f64) would miss the 4 byte padding within B and then as a consequence also miss the 4 byte padding within A that repr(C) inserts.)

The type in question (not having fixed the JsValue zero thing yet) is ResultAbi<OptionalF64>, which boils down to { { u32, f64 }, u32 } (in particular, not { u32, { u32, f64 } }) and can therefore be unpacked by the limited implementation. It is unpacked correctly as { u32, pad[4], f64, u32 }. If there are no WasmAbis that don't have this property, then this would be technically good enough, but it should at least be documented on the WasmAbi trait for later reference. I haven't spotted any counterexamples so far. If there aren't, that is probably due in part to the limited recursion (eg Option can't contain more Options, wasm-bindgen won't do it.)

I agree full-on struct packing is overboard. We only need to generate aligned glue code for WasmAbis we know about, so the instruction builder has perfect knowledge of where the fields should be, and it would be unnecessary (however cool!) to expose this via eg WasmDescribe. All you need to do is communicate this information from the instruction builder to the stack reader, for each field you know needs tweaking. I don't think you would need to actually use a u64, for instance, just annotate the adapter types.

• You could sprinkle some AdapterType::Padding and simply advance the offset in the unpacker without emitting a LoadRetptr
• Similarly AdapterType::AlignTo(usize) to align the next field read, slightly less thinking required than padding.

Review the use of f64 within other structures (basically Option), a bit of fiddling with the sanity checks on inputs.len or whatever, and I think that would do it and be a little bit future proof.

[alexcrichton]

Hm so another option is something like #[repr(packed)] on the Rust side of things so there's never any padding and no alignment issues, it's just one field to the next. Would that help the decoding side?

I also think it's fine to place any field wherever, what you're doing already it seems for whatever's easiest feels like the best way to go to me.

[cormacrelf]

Nah, the Float64Array view of the wasm memory can only do aligned reads. You'd have copy from a UInt32Array view into a buffer and reinterpret as f64.

[alexcrichton]

Oh I since learned awhile back that there's DataView which supports unaligned reads which the bindings could switch to using instead of various views with typed arrays (only needs one DataView for all reads as opposed to a view-per-type)

[cormacrelf]

This last thing, I am finally out of my depth, but:

1. takeObject is now exposed, but this failed when you had externref enabled in one of the cli reference tests (anyref-import-catch) since exposing takeObject fails when it is
2. I basically copied the way Instruction::I32FromOptionExternref does the addHeapObject/addToExternrefTable switching, because that looked like it would work
3. i.e. switching between takeObject/getFromExternrefTable. My implementation just goes table.get(idx) and returns it. Who knows how WebAssembly.Table does dropObject? Does it just never release an index?
4. it did work, in the sense that the tests can now run with externref enabled, and I added a WASM_BINDGEN_EXTERNREF=1 version of the test suite to CI

[alexcrichton]

This is looking really good to me, I think the only semi-major thing is what you've pointed out, but otherwise I'm quite happy with how this is shaping up!

[alexcrichton]

Are the changes in this block still necessary?

[cormacrelf]

Yes. The offsets placed in the generated LoadRetptr instructions have changed from .enumerate() indexes, into correct quad offsets. So before, Optionalf64 worked by a fluke, because the f64 LoadRetptr had offset=1 size=8, and the f64 field was aligned to 8 bytes, and that happened to line up. Now, the same situation gives you offset=2. So this code has to change at least a little bit, in order to get f64 LoadRetptrs to divide that quad offset by 2 to get an offset into the Float64Array.

The variable names uniqueness is the rest of it, we introduce scaled_offset to keep offset for use in the var r{} = ... below, as offset is unique to each LoadRetptr but scaled_offset is not (sometimes being divided by two).

[alexcrichton]

Hm sorry so discuss this further, I don't really understand why the term "quad" is being used in wasm-bindgen. I think it would make more sense for everything to be byte-based instead of quad-based. I realize that wasm-bindgen only works with 4 and 8-byte things but I think it makes more sense to still use bytes internally instead of bytes some places and quads/etc other places

[cormacrelf]

Fair enough, it would be better with only bytes. I'll submit a separate PR, it requires modifying this function alongside the StructUnpacker. Fortunately doing so is at most a glue code variable name change (as opposed to an ABI change).

[cormacrelf]

Alright there is ONE MORE.... consider this:

module.exports.return_string_err = function() {
    try {
        const retptr = wasm.__wbindgen_add_to_stack_pointer(-16);
        wasm.return_string_err(retptr);
        var r0 = getInt32Memory0()[retptr / 4 + 0];
        var r1 = getInt32Memory0()[retptr / 4 + 1];
        var r2 = getInt32Memory0()[retptr / 4 + 2];
        var r3 = getInt32Memory0()[retptr / 4 + 3];
        if (r3) {
            throw takeObject(r2);
        }
        return getStringFromWasm0(r0, r1);
    } finally {
        wasm.__wbindgen_add_to_stack_pointer(16);
        wasm.__wbindgen_free(r0, r1);
    }
};

If you throw takeObject(r2); you still hit the finally block, and it frees r0, r1, which is an undefined pointer/len combo as we didn't write to them at all. So I think we need either

• a conditional deferred call instruction, i.e. if (r3) { wasm.__wbindgen_free(r0, r1); }, or
• an equivalent intrinsic i.e. wasm.__wbindgen_free_conditional(r3, r0, r1); to use with the normal DeferredCallCore.

[cormacrelf]

Or actually, __wbindgen_free is fine to call with len=0. We can just set r1 = 0! This is now done, and there's an assert to ensure no DeferCallCore instructions area added for any other descriptor types.

[alexcrichton]

This looks fantastic to me, thanks again for your work on this! I've got one remaining nit about terminology of quads vs bytes, but that's pretty minor and can be fixed in a follow-up if truly necessary. Otherwise I'm gonna go ahead and merge this.

[cormacrelf]

It was a pleasure working with you! If there are any issues related to this or other ABI composition problems, feel free to ping me.

(Also, you can probably also close #1742 now (edit: although the docs book needs updating).)

[SebastienGllmt]

This stack space leak has been a plague on so many projects for years. Thank you so much for helping to solve this issue. I am eternally grateful ❤