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

* add Descriptor RESULT and Instruction::UnwrapResult

* ResultAbi / ResultAbiUnion

basic wasmresult support

one WasmResult ctor per WasmAbi

Remove WasmResult class

* Reverse the fields on ResultAbi, remove is_ok as err can be 0

impl OptionIntoWasmAbi for JsValue

* implement ResultAbi

* Add tests for `-> Result<T, JsError>`

* split result.rs tests in two

remove console_log

* initial implementation of `-> Result<T, JsError>`

describe Result<_,  JsError>

implement Intrinsics::ErrorNew

Revert impl From<()> for JsValue (src/lib.rs)

impl WasmDescribe etc for JsError

remove unused JsError

* docs on JsError, move to lib.rs

* Add test for returning Result<(), _>

* Add failing test for returning `Result<Option<f64>, _>`

This fails because the generated LoadRetptr instructions do not
have any conception of how big the previous args are. It only fails
when any part of T is an f64.

* Make LoadRetptr offsets factor in alignment and previously read values

* Add a doc comment to UnwrapResult

* Slight correction to a comment

* Better error message

* un-implement OptionIntoWasmAbi for JsValue, use discriminant instead

* Add some documentation from the PR discussion to ResultAbi

* un-implement OptionIntoWasmAbi for &'a JsValue too

* bless some UI tests, not sure why

* bless the CLI's output tests

* fix indentation of if (is_ok === 0) { throw ... } code

* add tests for async fn() -> Result<_, JsError> and custom error types

* cargo fmt

* fix bug where takeObject was missing

* support externref in UnwrapResult

* add a WASM_BINDGEN_EXTERNREF=1 test to ci

* getFromExternrefTable -> takeFromExternrefTable

Now we do not leak externrefs, as the take function
calls drop on them.

* rewrite outgoing_result

There was a crash where _outgoing inserted more than
one instruction, e.g. for string. In that case,
the deferred free() call was using the wrong popped
values, and tried to free nonsense formed by
the is_ok/err pair.

Now it does a similar idea, but without assuming exactly
one instruction will be pushed by self._outgoing().

* rename is_ok -> is_err, which makes generated glue easier to read

* update ui tests

* add a crashing (if you uncomment the throw) test of Result<String, _>

* add result-string reference test

* Fix the crashy Result<String, _> by setting ptr/len to 0 on error

.github/workflows/main.yml

@@ -62,6 +62,11 @@ jobs:
     - run: cargo test --target wasm32-unknown-unknown --test wasm --features serde-serialize
       env:
         WASM_BINDGEN_WEAKREF: 1
+    - run: cargo test --target wasm32-unknown-unknown
+      env:
+        WASM_BINDGEN_EXTERNREF: 1
+        NODE_ARGS: --experimental-wasm-reftypes
+
 
   test_wasm_bindgen_windows:
     name: "Run wasm-bindgen crate tests (Windows)"

crates/cli-support/src/descriptor.rs

@@ -36,6 +36,7 @@ tys! {
     RUST_STRUCT
     CHAR
     OPTIONAL
+    RESULT
     UNIT
     CLAMPED
 }
@@ -68,6 +69,7 @@ pub enum Descriptor {
     RustStruct(String),
     Char,
     Option(Box<Descriptor>),
+    Result(Box<Descriptor>),
     Unit,
 }
 
@@ -133,6 +135,7 @@ impl Descriptor {
             SLICE => Descriptor::Slice(Box::new(Descriptor::_decode(data, clamped))),
             VECTOR => Descriptor::Vector(Box::new(Descriptor::_decode(data, clamped))),
             OPTIONAL => Descriptor::Option(Box::new(Descriptor::_decode(data, clamped))),
+            RESULT => Descriptor::Result(Box::new(Descriptor::_decode(data, clamped))),
             CACHED_STRING => Descriptor::CachedString,
             STRING => Descriptor::String,
             EXTERNREF => Descriptor::Externref,

crates/cli-support/src/externref.rs

@@ -70,6 +70,7 @@ pub fn process(module: &mut Module) -> Result<()> {
     aux.externref_table = Some(meta.table);
     if module_needs_externref_metadata(&aux, section) {
         aux.externref_alloc = meta.alloc;
+        aux.externref_drop = meta.drop;
         aux.externref_drop_slice = meta.drop_slice;
     }
 
@@ -108,6 +109,15 @@ pub fn process(module: &mut Module) -> Result<()> {
                 } => {
                     *table_and_alloc = meta.alloc.map(|id| (meta.table, id));
                 }
+
+                Instruction::UnwrapResult {
+                    ref mut table_and_drop,
+                }
+                | Instruction::UnwrapResultString {
+                    ref mut table_and_drop,
+                } => {
+                    *table_and_drop = meta.drop.map(|id| (meta.table, id));
+                }
                 _ => continue,
             };
         }

crates/cli-support/src/intrinsic.rs

@@ -220,6 +220,9 @@ intrinsics! {
         #[symbol = "__wbindgen_rethrow"]
         #[signature = fn(Externref) -> Unit]
         Rethrow,
+        #[symbol = "__wbindgen_error_new"]
+        #[signature = fn(ref_string()) -> Externref]
+        ErrorNew,
         #[symbol = "__wbindgen_memory"]
         #[signature = fn() -> Externref]
         Memory,

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

@@ -596,16 +596,20 @@ fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) ->
         }
 
         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),
+            let (mem, quads) = match ty {
+                AdapterType::I32 => (js.cx.expose_int32_memory(*mem), 1),
+                AdapterType::F32 => (js.cx.expose_f32_memory(*mem), 1),
+                AdapterType::F64 => (js.cx.expose_f64_memory(*mem), 2),
                 other => bail!("invalid aggregate return type {:?}", other),
             };
+            let size = quads * 4;
+            // Separate the offset and the scaled offset, because otherwise you don't guarantee
+            // that the variable names will be unique.
+            let scaled_offset = offset / quads;
             // 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);
+            let expr = format!("{}()[retptr / {} + {}]", mem, size, scaled_offset);
             js.prelude(&format!("var r{} = {};", offset, expr));
             js.push(format!("r{}", offset));
         }
@@ -783,6 +787,69 @@ fn instruction(js: &mut JsBuilder, instr: &Instruction, log_error: &mut bool) ->
             js.push(format!("len{}", i));
         }
 
+        Instruction::UnwrapResult { table_and_drop } => {
+            let take_object = if let Some((table, drop)) = *table_and_drop {
+                js.cx
+                    .expose_take_from_externref_table(table, drop)?
+                    .to_string()
+            } else {
+                js.cx.expose_take_object();
+                "takeObject".to_string()
+            };
+            // is_err is popped first. The original layout was: ResultAbi {
+            //    abi: ResultAbiUnion<T>,
+            //    err: u32,
+            //    is_err: u32,
+            // }
+            // So is_err is last to be added to the stack.
+            let is_err = js.pop();
+            let err = js.pop();
+            js.prelude(&format!(
+                "
+                if ({is_err}) {{
+                    throw {take_object}({err});
+                }}
+                ",
+                take_object = take_object,
+                is_err = is_err,
+                err = err,
+            ));
+        }
+
+        Instruction::UnwrapResultString { table_and_drop } => {
+            let take_object = if let Some((table, drop)) = *table_and_drop {
+                js.cx
+                    .expose_take_from_externref_table(table, drop)?
+                    .to_string()
+            } else {
+                js.cx.expose_take_object();
+                "takeObject".to_string()
+            };
+            let is_err = js.pop();
+            let err = js.pop();
+            let len = js.pop();
+            let ptr = js.pop();
+            let i = js.tmp();
+            js.prelude(&format!(
+                "
+                var ptr{i} = {ptr};
+                var len{i} = {len};
+                if ({is_err}) {{
+                    ptr{i} = 0; len{i} = 0;
+                    throw {take_object}({err});
+                }}
+                ",
+                take_object = take_object,
+                is_err = is_err,
+                err = err,
+                i = i,
+                ptr = ptr,
+                len = len,
+            ));
+            js.push(format!("ptr{}", i));
+            js.push(format!("len{}", i));
+        }
+
         Instruction::OptionString {
             mem,
             malloc,

crates/cli-support/src/js/mod.rs

@@ -2194,6 +2194,34 @@ impl<'a> Context<'a> {
         true
     }
 
+    fn expose_take_from_externref_table(
+        &mut self,
+        table: TableId,
+        drop: FunctionId,
+    ) -> Result<MemView, Error> {
+        let view = self.memview_table("takeFromExternrefTable", table);
+        assert!(self.config.externref);
+        if !self.should_write_global(view.to_string()) {
+            return Ok(view);
+        }
+        let drop = self.export_name_of(drop);
+        let table = self.export_name_of(table);
+        self.global(&format!(
+            "
+                function {view}(idx) {{
+                    const value = wasm.{table}.get(idx);
+                    wasm.{drop}(idx);
+                    return value;
+                }}
+            ",
+            view = view,
+            table = table,
+            drop = drop,
+        ));
+
+        Ok(view)
+    }
+
     fn expose_add_to_externref_table(
         &mut self,
         table: TableId,
@@ -3141,6 +3169,11 @@ impl<'a> Context<'a> {
                 format!("throw {}", args[0])
             }
 
+            Intrinsic::ErrorNew => {
+                assert_eq!(args.len(), 1);
+                format!("new Error({})", args[0])
+            }
+
             Intrinsic::Module => {
                 assert_eq!(args.len(), 0);
                 if !self.config.mode.no_modules() && !self.config.mode.web() {

crates/cli-support/src/wit/incoming.rs

@@ -135,6 +135,7 @@ impl InstructionBuilder<'_, '_> {
             Descriptor::Function(_) |
             Descriptor::Closure(_) |
 
+            Descriptor::Result(_) |
             // Always behind a `Ref`
             Descriptor::Slice(_) => bail!(
                 "unsupported argument type for calling Rust function from JS: {:?}",

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

@@ -1346,9 +1346,11 @@ impl<'a> Context<'a> {
         });
         if uses_retptr {
             let mem = ret.cx.memory()?;
-            for (i, ty) in ret.input.into_iter().enumerate() {
+            let mut unpacker = StructUnpacker::new();
+            for ty in ret.input.into_iter() {
+                let offset = unpacker.read_ty(&ty)?;
                 instructions.push(InstructionData {
-                    instr: Instruction::LoadRetptr { offset: i, ty, mem },
+                    instr: Instruction::LoadRetptr { offset, ty, mem },
                     stack_change: StackChange::Modified {
                         pushed: 1,
                         popped: 0,
@@ -1569,3 +1571,46 @@ fn verify_schema_matches<'a>(data: &'a [u8]) -> Result<Option<&'a str>, Error> {
 fn concatenate_comments(comments: &[&str]) -> String {
     comments.iter().map(|&s| s).collect::<Vec<_>>().join("\n")
 }
+
+/// The C struct packing algorithm, in terms of u32.
+struct StructUnpacker {
+    next_offset: usize,
+}
+
+impl StructUnpacker {
+    fn new() -> Self {
+        Self { next_offset: 0 }
+    }
+    fn align_up(&mut self, alignment_pow2: usize) -> usize {
+        let mask = alignment_pow2 - 1;
+        self.next_offset = (self.next_offset + mask) & (!mask);
+        self.next_offset
+    }
+    fn append(&mut self, quads: usize, alignment_pow2: usize) -> usize {
+        let ret = self.align_up(alignment_pow2);
+        self.next_offset += quads;
+        ret
+    }
+    /// Returns the offset for this member, with the offset in multiples of u32.
+    fn read_ty(&mut self, ty: &AdapterType) -> Result<usize, Error> {
+        let (quads, alignment) = match ty {
+            AdapterType::I32 | AdapterType::U32 | AdapterType::F32 => (1, 1),
+            AdapterType::F64 => (2, 2),
+            other => bail!("invalid aggregate return type {:?}", other),
+        };
+        Ok(self.append(quads, alignment))
+    }
+}
+
+#[test]
+fn test_struct_packer() {
+    let mut unpacker = StructUnpacker::new();
+    let i32___ = &AdapterType::I32;
+    let double = &AdapterType::F64;
+    let mut read_ty = |ty| unpacker.read_ty(ty).unwrap();
+    assert_eq!(read_ty(i32___), 0); // u32
+    assert_eq!(read_ty(i32___), 1); // u32
+    assert_eq!(read_ty(double), 2); // f64, already aligned
+    assert_eq!(read_ty(i32___), 4); // u32, already aligned
+    assert_eq!(read_ty(double), 6); // f64, NOT already aligned, skips up to offset 6
+}

crates/cli-support/src/wit/nonstandard.rs

@@ -52,6 +52,7 @@ pub struct WasmBindgenAux {
     pub externref_table: Option<walrus::TableId>,
     pub function_table: Option<walrus::TableId>,
     pub externref_alloc: Option<walrus::FunctionId>,
+    pub externref_drop: Option<walrus::FunctionId>,
     pub externref_drop_slice: Option<walrus::FunctionId>,
 
     /// Various intrinsics used for JS glue generation