Bug 191776 – Creating a wasm memory that is bigger than the ArrayBuffer limit but smaller than the spec limit should throw OOME not RangeError

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Mark Lam]

<rdar://problem/46152851>

[Mark Lam]

Created attachment 355436 [details]
proposed patch.

[Saam Barati]

Comment on attachment 355436 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=355436&action=review

> JSTests/wasm/regress/wasm-memory-requested-more-than-MAX_ARRAY_BUFFER_SIZE-2.js:88
> +var exception;
> +try {
> +    var module = new WasmModuleBuilder();
> +    module.addMemory(32768);
> +    module.instantiate();
> +} catch (e) {
> +    exception = e;
> +}
> +
> +if (exception != "Error: Out of memory") {
> +    print(exception);
> +    throw "FAILED";
> +}

Why not just write this as a wasm test so you're not copying part of the module builder?

[Mark Lam]

Comment on attachment 355436 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=355436&action=review

Thanks for the review.

>> JSTests/wasm/regress/wasm-memory-requested-more-than-MAX_ARRAY_BUFFER_SIZE-2.js:88
>> +}
> 
> Why not just write this as a wasm test so you're not copying part of the module builder?

This test is a cleaned up version of one provided by an external contributor.  I rather keep it close to its original form.

Also, this "Out of memory" behavior is not required (though allowed) by the WASM spec: it is an artifact of our implementation that does not allow ArrayBuffers to be greater than MAX_ARRAY_BUFFER_SIZE in size.  The bug is that we were previously throwing a RangeError or crashing on a RELEASE_ASSERT.  This patch fixes it to throw an OOME instead.  Since this is not spec required behavior, I don't think it's right to put it in the wasm/spec-tests directory.

[EWS Watchlist]

Comment on attachment 355436 [details]
proposed patch.

Attachment 355436 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10102730

New failing tests:
stress/big-wasm-memory-grow-no-max.js.ftl-eager-no-cjit
stress/big-wasm-memory.js.ftl-eager
stress/big-wasm-memory-grow-no-max.js.no-cjit-validate-phases
stress/big-wasm-memory-grow.js.dfg-eager
stress/big-wasm-memory-grow.js.no-cjit-collect-continuously
stress/big-wasm-memory.js.ftl-eager-no-cjit-b3o1
stress/big-wasm-memory-grow.js.no-cjit-validate-phases
stress/big-wasm-memory.js.ftl-no-cjit-small-pool
stress/big-wasm-memory-grow-no-max.js.ftl-eager-no-cjit-b3o1
stress/big-wasm-memory-grow.js.default
stress/big-wasm-memory-grow-no-max.js.ftl-eager
stress/big-wasm-memory-grow-no-max.js.no-ftl
stress/big-wasm-memory-grow-no-max.js.default
stress/big-wasm-memory-grow-no-max.js.dfg-eager-no-cjit-validate
stress/big-wasm-memory-grow.js.ftl-no-cjit-small-pool
stress/big-wasm-memory-grow.js.ftl-eager
stress/big-wasm-memory-grow.js.ftl-no-cjit-validate-sampling-profiler
stress/big-wasm-memory.js.ftl-no-cjit-no-inline-validate
stress/big-wasm-memory.js.ftl-no-cjit-validate-sampling-profiler
stress/big-wasm-memory-grow-no-max.js.no-llint
stress/big-wasm-memory-grow.js.ftl-no-cjit-no-inline-validate
stress/big-wasm-memory.js.default
stress/big-wasm-memory.js.dfg-eager-no-cjit-validate
stress/big-wasm-memory-grow.js.no-ftl
stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-no-inline-validate
stress/big-wasm-memory.js.no-llint
stress/big-wasm-memory.js.ftl-eager-no-cjit
stress/big-wasm-memory-grow.js.ftl-no-cjit-no-put-stack-validate
stress/big-wasm-memory.js.no-cjit-validate-phases
stress/big-wasm-memory-grow-no-max.js.dfg-eager
stress/big-wasm-memory-grow.js.dfg-eager-no-cjit-validate
stress/big-wasm-memory.js.no-cjit-collect-continuously
stress/big-wasm-memory-grow.js.dfg-maximal-flush-validate-no-cjit
stress/big-wasm-memory-grow.js.no-llint
stress/big-wasm-memory.js.ftl-no-cjit-b3o1
stress/big-wasm-memory.js.dfg-maximal-flush-validate-no-cjit
stress/big-wasm-memory.js.ftl-no-cjit-no-put-stack-validate
stress/big-wasm-memory-grow-no-max.js.dfg-maximal-flush-validate-no-cjit
stress/big-wasm-memory-grow.js.ftl-no-cjit-b3o1
stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-small-pool
stress/big-wasm-memory-grow.js.ftl-eager-no-cjit-b3o1
stress/big-wasm-memory.js.dfg-eager
stress/big-wasm-memory-grow-no-max.js.no-cjit-collect-continuously
stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-validate-sampling-profiler
stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-no-put-stack-validate
stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-b3o1
stress/big-wasm-memory.js.no-ftl
stress/big-wasm-memory-grow.js.ftl-eager-no-cjit
apiTests

[Mark Lam]

Created attachment 355442 [details]
patch for landing.

[Mark Lam]

Created attachment 355443 [details]
patch for landing.

[WebKit Commit Bot]

Comment on attachment 355443 [details]
patch for landing.

Clearing flags on attachment: 355443

Committed r238433: <https://trac.webkit.org/changeset/238433>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.