Bug 191776

Summary: Creating a wasm memory that is bigger than the ArrayBuffer limit but smaller than the spec limit should throw OOME not RangeError
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed patch.
saam: review+, ews-watchlist: commit-queue-
patch for landing.
none
patch for landing. none

Description Filip Pizlo 2018-11-16 14:38:47 PST
...
Comment 1 Mark Lam 2018-11-21 12:48:32 PST
<rdar://problem/46152851>
Comment 2 Mark Lam 2018-11-21 13:36:45 PST
Created attachment 355436 [details]
proposed patch.
Comment 3 Saam Barati 2018-11-21 14:22:31 PST
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?
Comment 4 Mark Lam 2018-11-21 14:36:34 PST
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.
Comment 5 EWS Watchlist 2018-11-21 15:04:40 PST
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
Comment 6 Mark Lam 2018-11-21 15:56:50 PST
Created attachment 355442 [details]
patch for landing.
Comment 7 Mark Lam 2018-11-21 15:58:05 PST
Created attachment 355443 [details]
patch for landing.
Comment 8 WebKit Commit Bot 2018-11-21 17:51:43 PST
Comment on attachment 355443 [details]
patch for landing.

Clearing flags on attachment: 355443

Committed r238433: <https://trac.webkit.org/changeset/238433>
Comment 9 WebKit Commit Bot 2018-11-21 17:51:44 PST
All reviewed patches have been landed.  Closing bug.