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).
<rdar://problem/29760348>
We must make sure that postMessage of a WebAssembly.Memory does the right thing, same of its underlying .buffer.
Created attachment 310230 [details] Patch
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
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?
> > 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.
Comment on attachment 310230 [details] Patch r+ back (with nits) because I derp.
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.
Created attachment 310237 [details] Patch
Created attachment 310238 [details] Patch for landing
(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.
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
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
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
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
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
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
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
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
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
Created attachment 310476 [details] Patch
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
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
Created attachment 310516 [details] Patch
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.
Created attachment 310518 [details] Patch for landing
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.
Comment on attachment 310518 [details] Patch for landing Clearing flags on attachment: 310518 Committed r217052: <http://trac.webkit.org/changeset/217052>
All reviewed patches have been landed. Closing bug.