Bug 163899 – WebAssembly API: test with neutered inputs

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

[JF Bastien]

The TypedArray and Views passed to the WebAssembly APIs can be neutered. I'm adding basic checks for this but am being lazy in testing them for now, just leaving TODOs for now. I need to go back and fix this, and clarify what exception type needs to be thrown (TypeError seems to be what JS uses elsewhere).

[Radar WebKit Bug Importer]

<rdar://problem/29760348>

[JF Bastien]

We must make sure that postMessage of a WebAssembly.Memory does the right thing, same of its underlying .buffer.

[Keith Miller]

Created attachment 310230 [details]
Patch

[Saam Barati]

Comment on attachment 310230 [details]
Patch

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

r=me with comment

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3001
> +            if (!arrayBuffer->isTransferable()) {

I wouldn’t do it quite like this, since this error message only makes sense given the above isShared check. Maybe remove the above check and come up with a more descriptive way of having different error messages for different untransferable arrays? Alternatively, you could have a more generic message. Perhaps you could even have a bit that says if it’s Wasm, and if so, have a more descriptive message

[JF Bastien]

Comment on attachment 310230 [details]
Patch

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

Missing the FIXME in JSTests/wasm/js-api/test_basic_api.js

I'm not familiar with all the places where the transferability should be checked, so it would be good to have another set of eyes look and make sure this is correct.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:71
> +    m_buffer->makeNonTransferable();

There's also code in WebAssemblyModuleConstructor.cpp which needs this.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3003
> +                throwVMTypeError(&state, scope, ASCIILiteral("Cannot transfer a WebAssembly.Memory"));

Weird that the property is "non-transferable" but the error message knows it's a WebAssembly.Memory. I'd change one to match the other.

> LayoutTests/ChangeLog:7
> +

Can you explain that this dups Saam's dup of the de-modularized Builder?

[JF Bastien]

> > Source/JavaScriptCore/wasm/js/JSWebAssemblyMemory.cpp:71
> > +    m_buffer->makeNonTransferable();
> 
> There's also code in WebAssemblyModuleConstructor.cpp which needs this.

Oops ignore me, this one's fine because it's always a copy.

[JF Bastien]

Comment on attachment 310230 [details]
Patch

r+ back (with nits) because I derp.

[Keith Miller]

Comment on attachment 310230 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3001
>> +            if (!arrayBuffer->isTransferable()) {
> 
> I wouldn’t do it quite like this, since this error message only makes sense given the above isShared check. Maybe remove the above check and come up with a more descriptive way of having different error messages for different untransferable arrays? Alternatively, you could have a more generic message. Perhaps you could even have a bit that says if it’s Wasm, and if so, have a more descriptive message

How would you feel changing the code above to:

if (!arrayBuffer->isTransferable()) {
    auto scope = DECLARE_THROW_SCOPE(vm);
    throwVMTypeError(&state, scope, transferErrorForArrayBuffer(arrayBuffer))
    return Exception { ExistingExceptionError };
}

where transferErrorForArrayBuffer(arrayBuffer) maps to "Cannot transfer a WebAssembly.Memory" / "Cannot transfer a SharedArrayBuffer" for wasm memory / SAB, respectively.

>> LayoutTests/ChangeLog:7
>> +
> 
> Can you explain that this dups Saam's dup of the de-modularized Builder?

Done.

[Keith Miller]

Created attachment 310237 [details]
Patch

[Keith Miller]

Created attachment 310238 [details]
Patch for landing

[Saam Barati]

(In reply to Keith Miller from comment #8)
> Comment on attachment 310230 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310230&action=review
> 
> >> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3001
> >> +            if (!arrayBuffer->isTransferable()) {
> > 
> > I wouldn’t do it quite like this, since this error message only makes sense given the above isShared check. Maybe remove the above check and come up with a more descriptive way of having different error messages for different untransferable arrays? Alternatively, you could have a more generic message. Perhaps you could even have a bit that says if it’s Wasm, and if so, have a more descriptive message
> 
> How would you feel changing the code above to:
> 
Sounds good. 

> if (!arrayBuffer->isTransferable()) {
>     auto scope = DECLARE_THROW_SCOPE(vm);
>     throwVMTypeError(&state, scope, transferErrorForArrayBuffer(arrayBuffer))
>     return Exception { ExistingExceptionError };
> }
> 
> where transferErrorForArrayBuffer(arrayBuffer) maps to "Cannot transfer a
> WebAssembly.Memory" / "Cannot transfer a SharedArrayBuffer" for wasm memory
> / SAB, respectively.
> 
> >> LayoutTests/ChangeLog:7
> >> +
> > 
> > Can you explain that this dups Saam's dup of the de-modularized Builder?
> 
> Done.

[Build Bot]

Comment on attachment 310238 [details]
Patch for landing

Attachment 310238 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3749045

New failing tests:
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-cjit
wasm.yaml/wasm/js-api/memory-grow.js.wasm-no-call-ic
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-ftl
stress/class-subclassing-string.js.ftl-eager
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-ftl-no-cjit
wasm.yaml/wasm/js-api/memory-grow.js.wasm-no-cjit
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-dfg-eager-no-cjit
wasm.yaml/wasm/js-api/memory-grow.js.wasm-eager-jettison
wasm.yaml/wasm/js-api/memory-grow.js.default-wasm
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout
jsc-layout-tests.yaml/js/script-tests/parser-syntax-check.js.layout-no-llint

[Build Bot]

Comment on attachment 310238 [details]
Patch for landing

Attachment 310238 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3749141

New failing tests:
workers/wasm-mem-post-message.html
workers/sab/postMessage-transfer-type-error.html

[Build Bot]

Created attachment 310243 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310238 [details]
Patch for landing

Attachment 310238 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3749152

New failing tests:
workers/wasm-mem-post-message.html
workers/sab/postMessage-transfer-type-error.html

[Build Bot]

Created attachment 310245 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310238 [details]
Patch for landing

Attachment 310238 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3749144

New failing tests:
workers/wasm-mem-post-message.html
workers/sab/sent-from-worker-no-transfer.html
workers/sab/postMessage-transfer-type-error.html

[Build Bot]

Created attachment 310246 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310238 [details]
Patch for landing

Attachment 310238 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3749234

New failing tests:
workers/wasm-mem-post-message.html
workers/sab/postMessage-transfer-type-error.html

[Build Bot]

Created attachment 310248 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Keith Miller]

Created attachment 310476 [details]
Patch

[Build Bot]

Comment on attachment 310476 [details]
Patch

Attachment 310476 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3766663

New failing tests:
workers/wasm-mem-post-message.html

[Build Bot]

Created attachment 310488 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Keith Miller]

Created attachment 310516 [details]
Patch

[Saam Barati]

Comment on attachment 310516 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:9
> +        Make it not possible to transfer an ArrayBuffer that is backing a
> +        wasm memory.

backing => backed by

> Source/JavaScriptCore/runtime/ArrayBuffer.h:163
> +    bool m_isWasmMemory : 1;

Don't you need to always initialize this to false? I don't see where you do that.

[Keith Miller]

Created attachment 310518 [details]
Patch for landing

[Keith Miller]

Comment on attachment 310516 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        wasm memory.
> 
> backing => backed by

Changed.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:163
>> +    bool m_isWasmMemory : 1;
> 
> Don't you need to always initialize this to false? I don't see where you do that.

Good catch, Fixed.

[WebKit Commit Bot]

Comment on attachment 310518 [details]
Patch for landing

Clearing flags on attachment: 310518

Committed r217052: <http://trac.webkit.org/changeset/217052>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.