Thrown exceptions do not restore the stack pointer

[SebastienGllmt]

Some more info below

Describe the Bug

When a String type is used the memory is never dropped on the WASM side. This is problematic because in our codebase we have functions like

 pub fn try_parse(hex: &str) -> Result<WasmStruct, JsValue> {
    RustStruct::from_hex(hex)
        .map_err(|e| JsValue::from_str(&format! {"{:?}", e}))
        .map(WasmStruct)
}

so whenever try_parse fails, the format! call allocates memory that just piles up during runtime until eventually you run out of memory.

Steps to Reproduce

Define a function such as

pub fn foo() -> Result<(), JsValue> {
    format! {"{:?}", "foo"}; // removing this lines stops the memory leak
    Err(JsValue::from_str(""))
}

or

pub fn foo() -> Result<(), JsValue> {
    "foo".to_string(); // removing this lines stops the memory leak
    Err(JsValue::from_str(""))
}

then you can call it in Javascript in this way

WasmModule
  .then(WasmModule => {
    for (let i = 0; i < 100000; i++) {
      try {
        WasmModule.foo();
      } catch (e) {
        // RuntimeError: "index out of bounds"
        // indicates out of memory
        if (e.toString().includes('index')) {
          console.log(i);
          break;
        }
      }
    }
});

Expected Behavior

Loop completes without any errors

Actual Behavior

On my computer this will crash after 65525 iterations. If you remove the string allocation though it does not crash.

Additional Context

Generates using wasm-pack 0.8.1 and happens on both nodejs and browser builds.

[alexcrichton]

Thanks for the report! I'm unfortunately able to reproduce this though. Can you share some more information? For example:

• What wasm-bindgen version are you using?
• What rustc version are you using?
• What is the build command you're using?
• What's the version of node and/or what browsers are you testing in?

[Pauan]

@SebastienGllmt In your example functions, the string is never sent to JS, it exists purely in Rust, and follows normal Rust semantics, so it will be dropped at the end of the statement, and so it cannot leak.

So if you are seeing a memory leak, then that's a bug in Rust itself (or possibly the Wasm engine), not wasm-bindgen.

How much RAM does your computer have? How much RAM does your Wasm program use before it crashes?

65525 iterations is very notable, because that's close to 65536, which is 2^16. So you might be running into host implementation limits.

[SebastienGllmt]

I made a super simple repo. You can find it here: https://github.com/SebastienGllmt/wasm-memory-test

Happens on both latest Firefox (72.0.1) & Chrome (79.0.3945.117)

Running rustc 1.40.0 (73528e339 2019-12-16) with wasm-bindgen = { version = "0.2.58" } and node v12.11.1

Computer has 32GBs of RAM on a 64bit OS so it's definitely not an OS-level problem

[Pauan]

@SebastienGllmt Thanks. So, this has nothing to do with strings, even if I do this I still get the error:

    pub fn foo() -> Result<(), JsValue> {
        Err(JsValue::from(0))
    }

As far as I can tell, there is no memory leak. A memory leak would require the memory usage to increase over time, but that's not happening here.

The error message for indexing out of bounds means just that: it's indexing out of bounds. It would give a different error message if it had run out of memory, like this:

Uncaught RangeError: WebAssembly.Memory.grow(): Maximum memory size exceeded

What I think is happening is that there is some sort of internal corruption because you're catching the error and then ignoring it. Basically, wasm-bindgen expects that after it throws an error the program will finish, but that's not happening in this case.

[SebastienGllmt]

Ah you're right maybe the issue is actually with Err and adding strings just made it hit the error faster.

After some additional digging I found this as part of the wasm-bindgen documentation

By default wasm-bindgen will take no action when wasm calls a JS function which ends up throwing an exception. The wasm spec right now doesn't support stack unwinding and as a result Rust code will not execute destructors. This can unfortunately cause memory leaks in Rust right now, but as soon as wasm implements catching exceptions we'll be sure to add support as well!

It seems the way that Err is implemented is that it makes a call to one of

module.exports.__wbindgen_throw = function(arg0, arg1) {
    throw new Error(getStringFromWasm0(arg0, arg1));
};

module.exports.__wbindgen_rethrow = function(arg0) {
    throw takeObject(arg0);
};

which throws an exception in JS-land so presumably this is the same issue causing the stack not to unwind.

If that's the case, what's the best way to implement something like try_parse as shown in my first post?

[alexcrichton]

Sorry I still can't seem to reproduce this. I checked out that repo and then executed wasm-pack build --target nodejs. I added index.js that looks like this:

const { memory } = require('./pkg/wasm_repro_bg');
const { WasmObject } = require('./pkg/wasm_repro');

let last = 0;
for (let i = 0; i < 100000000; i++) {
  try {
    WasmObject.foo();
  } catch (e) {
    const size = memory.grow(0);
    if (size > last) {
      console.log(size);
      last = size;
    }
  }
}

I don't see any memory growth so I'm not sure what's causing you to think that there's a leak. Is this perhaps a different bug where, when reduced, isn't showing the original bug?

Destructors are tricky with wasm right now, but the examples given here should not be leaking memory. If you're using wasm-bindgen APIs nothing should get leaked, and if they are that's a bug or a clarification we need to make.

[alexcrichton]

Ah ok I believe I see what's happening now. The wasm module isn't running out of memory, but it's running out of stack.

When the exception is thrown no more wasm code runs, and this includes wasm code to restore the stack pointer.

This is a fundamental design flaw in returning Result which I don't believe we ever considered, and is something that will take some time to fix!

[alexcrichton]

Ok I'm going to close this in favor of #1963 which is a bit more targeted and is only about the stack growth issue.