1861434 - WebAssembly.instantiate sometimes rejects with string instead of Error when out of memory

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Andrew Swan [:aswan]]

I don't have specific steps to reproduce this, this is based on errors we're receiving from figma users in the field.
Sometimes the Promise returned from WebAssembly.instantiateStreaming() is rejecting with the string "out of memory" rather than with an Error instance with that string as the message. I'm not familiar enough with the specifications to know if this is technically in compliance or not, but it is inconsistent with other web apis, it would be handy if it return an Error instance...

Comment 1

[Andrew Swan [:aswan]]

And looking at more error data from the field, it appears this is happening with plain old WebAssembly.instantiate() in addition to instantiateStreaming

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rhunt, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[Ryan Hunt [:rhunt]]

Thanks for the report! Looking at the Wasm JS-API spec [1], OOM errors are defined to match whatever JS does, which is itself implementation defined. It looks like SpiderMonkey's default behavior is to throw 'out of memory' as you're observing. I don't know if this is something we can change on our end, I'll check.

Do you get a different kind of OOM from other browsers? Or are you only getting OOM's in Firefox?

[1] https://webassembly.github.io/spec/js-api/index.html#out-of-memory

Comment 4

[Andrew Swan [:aswan]]

I don't have an example of the message property on the Error that blink or webkit produces in this case readily available, but I can say for sure that they reject with an instance of Error (or at least something with a .message property, I just noticed this since we had some code that was roughly

try {
  await WebAssembly.instantiateStreaming(...);
} catch (err) {
  if (err.message.includes("something...")) { ... }
}

And the code in the catch block was throwing (since err.message is undefined), but only in Firefox.

Comment 5

[Andrew Swan [:aswan]]

It looks like SpiderMonkey's default behavior is to throw 'out of memory'

Oh interesting, I overlooked this at first, I guess maybe this is to try to avoid things that might require additional big allocations like gathering a stack trace to stuff into an Error?

Comment 6

[Ryan Hunt [:rhunt]]

(In reply to Andrew Swan [:aswan] from comment #5)

It looks like SpiderMonkey's default behavior is to throw 'out of memory'

Oh interesting, I overlooked this at first, I guess maybe this is to try to avoid things that might require additional big allocations like gathering a stack trace to stuff into an Error?

That would be my guess as well.

Jan, have we had compatibility issues before with throwing out of memory instead of an Error? Is this something we can change?

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Ryan Hunt [:rhunt] from comment #6)

Jan, have we had compatibility issues before with throwing out of memory instead of an Error? Is this something we can change?

OOM errors used to be uncatchable exceptions (similar to, for example, the slow script dialog stopping execution). This was changed in bug 865960 and that bug also has some discussion. Comment 5 is correct that we do this to avoid allocating an Error object etc. We also can't GC under ReportOutOfMemory because many callers depend on this.

If there's a case where OOM is more likely to happen, for example due to implementation limits instead of a real OOM, maybe we can change that to throw an actual InternalError?

Comment 8

[Ryan Hunt [:rhunt]]

(In reply to Jan de Mooij [:jandem] from comment #7)

(In reply to Ryan Hunt [:rhunt] from comment #6)

Jan, have we had compatibility issues before with throwing out of memory instead of an Error? Is this something we can change?

OOM errors used to be uncatchable exceptions (similar to, for example, the slow script dialog stopping execution). This was changed in bug 865960 and that bug also has some discussion. Comment 5 is correct that we do this to avoid allocating an Error object etc. We also can't GC under ReportOutOfMemory because many callers depend on this.

If there's a case where OOM is more likely to happen, for example due to implementation limits instead of a real OOM, maybe we can change that to throw an actual InternalError?

I think in many cases with compiling modules, the OOM is due to there not being enough code memory, not due to lack of general heap memory. So throwing some actual error object might make sense here.

I also wonder if there is some last ditch GC'ing we could do to try to free up code memory in these cases.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Ryan Hunt [:rhunt] from comment #8)

I think in many cases with compiling modules, the OOM is due to there not being enough code memory, not due to lack of general heap memory. So throwing some actual error object might make sense here.

Agreed. That would also let us use a more accurate error message.

I also wonder if there is some last ditch GC'ing we could do to try to free up code memory in these cases.

We do have some code here to call the large-allocation-failure callback which will trigger a GC.

Comment 10

[Ryan Hunt [:rhunt]]

Attachment: Bug 1861434 - wasm: Report OOM error instead of OOM string for module compilation. r?jandem

Module compilation fails most frequently from large heap allocations or
not enough code memory. In these cases, we're very likely able to allocate
a proper error object. This commit switches module compilation to report
an InternalError now in these cases.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25e5c4ea04b1
wasm: Report OOM error instead of OOM string for module compilation. r=jandem

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/25e5c4ea04b1