| Summary: | REGRESSION: MessagePort.postMessage() fails to send transferable objects | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yann Cabon <ycabon> | ||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Tadeu Zagallo <tzagallo> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Major | CC: | alecflett, beidson, cdumez, commit-queue, dbates, dpaddock, esprehn+autocc, ews-watchlist, jeremyj-wk, jsbell, kangil.han, keith_miller, koivisto, mark.lam, msaboff, rniwa, saam, tzagallo, webkit-bug-importer, youennf | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar, Regression | ||||||||||||||||||||||||
| Version: | Safari 11 | ||||||||||||||||||||||||||
| Hardware: | Mac | ||||||||||||||||||||||||||
| OS: | macOS 10.13 | ||||||||||||||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=184502 https://bugs.webkit.org/show_bug.cgi?id=184285 |
||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
Created attachment 337087 [details]
HTML page of the test case
I'm adding another test case to more represent how we actually setup the MessagePort communication. We spawn a Worker, then when it replies we send him a MessagePort, then the communication between the 2 ports begin. We do this as we handle multiple communication channels, agnostic of where the worker code is actually running. I attached 2 files, The html page that spawn the worker and the worker script. Created attachment 337088 [details]
The worker script of the test case
Created attachment 337306 [details]
Patch
Comment on attachment 337306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337306&action=review > LayoutTests/workers/message-port.html:30 > + debug(array[0]); This should expect a specific value and display a PASS or FAIL line in the expected results. Otherwise, you can be sure someone will come and simply rebaseline the test if it starts outputing a different value or no value. Since you're using js-test, you can use shouldBe() utility function. It seems that the regression was introduce in https://bugs.webkit.org/show_bug.cgi?id=181922 due to `SerializedScriptValue`s being encoded with `toWireData`, which does not include ArrayBuffers (or any other extra data contained in the serialized value for that matter). However, I'm not sure if this is the best approach, and if it's OK to expose the ArrayBufferContents ctor that I had to expose. Also, we should probably throw an error when a SharedArrayBuffer is part of transfer list (similar to what Chrome does). Comment on attachment 337306 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337306&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:142 > + Remove extra blank line. Created attachment 337308 [details]
Patch
(In reply to Chris Dumez from comment #6) > Comment on attachment 337306 [details] > Since you're using js-test, you can use shouldBe() utility function. `shouldBe` warned that it didn't like numbers, so I wrote the check by hand. Hopefully that's ok. Created attachment 337540 [details]
Patch
Created attachment 337575 [details]
Patch
Comment on attachment 337575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337575&action=review > Source/JavaScriptCore/wasm/WasmMemory.h:89 > + void checkDeletionHasntBegun() { ASSERT(!deletionHasBegun()); } We prefer to avoid contractions when naming things. Maybe checkDidStartDeletion or checkIsDeleting or checkDeletionHasNotBegun. I do not like any of these names, but this is what I came up with at the moment. I suspect we have functions that serve a similar purpose in the codebase. Maybe we can draw inspiration from such code. Have you grepped the codebase for “deletion” or “hasBegun”? > Source/WebCore/bindings/js/SerializedScriptValue.h:142 > + std::unique_ptr<ArrayBufferContentsArray> arrayBufferContentsArray; Please move this declaration and the declaration for blobURL as close to where they are used as possible. We do not need to use C89 conventions and declare all variables at the start of the function. Although this is unlikely to be hot code it is inefficient to construct both this std::unique_ptr and blobURLs when we may bail out early. > Source/WebCore/bindings/js/SerializedScriptValue.h:166 > + void *buffer = Gigacage::tryMalloc(Gigacage::Primitive, bufferSize); * on the wrong side. > Source/WebCore/bindings/js/SerializedScriptValue.h:167 > + auto destructor = [] (void* p) { p => ptr > LayoutTests/ChangeLog:8 > + The regression test provided with the bug report verifies that the ArrayBuffer is properly serialized - before, the whole data object would be null. It is an unwritten convention that we wrap ChangeLog lines to around ~100 characters. > LayoutTests/workers/message-port.html:1 > +<html> I do not see the need to render this page using quirks mode. Please add <!DOCTYPE html> to the top of this file. > LayoutTests/workers/message-port.html:3 > +<script src="../resources/js-test-pre.js"></script> We prefer that new tests include LayoutTests/resources/js-test.js (combines js-test-pre.js and js-test-post.js) instead of explicitly including js-test-pre.js and js-test-post.js. > LayoutTests/workers/message-port.html:7 > +'use strict'; Is this necessary? Comment on attachment 337575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337575&action=review > Source/JavaScriptCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=184254 Please add the Radar bug URL under this line. Comment on attachment 337575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337575&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:34 > +#include <JavaScriptCore/WasmModule.h> I am not at a computer with a checkout at the moment. What functionality are we using from this header? Comment on attachment 337575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337575&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.h:34 >> +#include <JavaScriptCore/WasmModule.h> > > I am not at a computer with a checkout at the moment. What functionality are we using from this header? I had to add this due to the implementation of `SerializedScriptValue::decode` on this header. It was failing to compile because of the unique_ptr referencing the destructor of an incomplete type. Created attachment 337622 [details]
Patch
(In reply to Tadeu Zagallo from comment #16) > Comment on attachment 337575 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=337575&action=review > > >> Source/WebCore/bindings/js/SerializedScriptValue.h:34 > >> +#include <JavaScriptCore/WasmModule.h> > > > > I am not at a computer with a checkout at the moment. What functionality are we using from this header? > > I had to add this due to the implementation of > `SerializedScriptValue::decode` on this header. It was failing to compile > because of the unique_ptr referencing the destructor of an incomplete type. Would we be able to avoid including this header by using a forwarding constructor? Specifically, can we declare a new constructor overload in this file that took all arguments except the std::unique_ptr to the WasmModule and then implement it in the .cpp file such that it forwards to the constructor that takes the std::unique_ptr passing nullptr? Created attachment 337732 [details]
Patch
(In reply to Daniel Bates from comment #18) > (In reply to Tadeu Zagallo from comment #16) > > Comment on attachment 337575 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=337575&action=review > > > > >> Source/WebCore/bindings/js/SerializedScriptValue.h:34 > > >> +#include <JavaScriptCore/WasmModule.h> > > > > > > I am not at a computer with a checkout at the moment. What functionality are we using from this header? > > > > I had to add this due to the implementation of > > `SerializedScriptValue::decode` on this header. It was failing to compile > > because of the unique_ptr referencing the destructor of an incomplete type. > > Would we be able to avoid including this header by using a forwarding > constructor? Specifically, can we declare a new constructor overload in this > file that took all arguments except the std::unique_ptr to the WasmModule > and then implement it in the .cpp file such that it forwards to the > constructor that takes the std::unique_ptr passing nullptr? Yes, that's much better. I've updated the patch, thanks! Comment on attachment 337732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337732&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:160 > + if (hasArray) { If hasArray is true then arrayLength > 0? If so, we should add an ASSERT() for this. If not, then can we avoid instantiating an empty array on line 165? > LayoutTests/workers/message-port.html:40 > +var arr = new Float64Array([Math.PI]); This test is good. It only tests the case of an array with one value. We should also add a test to cover an empty array. Can we test the serialization logic for blob URLs and the data argument in the serialization code? Comment on attachment 337732 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337732&action=review > Source/WebCore/bindings/js/SerializedScriptValue.cpp:3177 > + , nullptr > +#endif > + ) Nit: Wrong indentation. "," should be aligned to :, and ) should be exactly 4 spaces to the right of "," and ":". Created attachment 338017 [details]
Patch
Comment on attachment 338017 [details]
Patch
I don't truly know enough about the array buffer stuff to judge it, and it's the squirreliest part of this.
Otherwise LGTM
Comment on attachment 338017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338017&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:134 > + auto hasArray = m_arrayBufferContentsArray && m_arrayBufferContentsArray->size() > 0; Nit: should read m_arrayBufferContentsArray && m_arrayBufferContentsArray->size(). https://webkit.org/code-style-guidelines/#zero-comparison > Source/WebCore/bindings/js/SerializedScriptValue.h:137 > + if (hasArray) { We prefer an early return over nested ifs. This should be if (!hasArray) return; instead. > Source/WebCore/bindings/js/SerializedScriptValue.h:158 > + if (hasArray) { Again, we should early return here. > LayoutTests/workers/message-port.html:13 > + return new Promise(function(resolve) { > + var channel = new MessageChannel(); Please use 4-space indentation here and in the rest of the file. We usually put a space between function and (. Also, use const? > LayoutTests/workers/message-port.html:16 > + if (event.data == null) These should really be !event.data > LayoutTests/workers/message-port.html:32 > + if (array.length === 0) And !array.length > LayoutTests/workers/message-port.html:40 > +var array = new Float64Array([Math.PI]); > +var emptyArray = new Float64Array(); > +var emptyArray2 = new Float64Array(); Use const? > LayoutTests/workers/message-port.html:46 > + var array = new Float64Array(data.buf[0]); Use const? Created attachment 338142 [details]
Patch
Comment on attachment 338142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338142&action=review > Source/WebCore/bindings/js/SerializedScriptValue.h:164 > + ASSERT(arrayLength > 0); ArrayLength is an unsigned data type so it can never be < 0. By our style guidelines you should remove the “> 0”. > Source/WebCore/bindings/js/SerializedScriptValue.h:171 > + Does it ever make sense that bufferSize is zero? If not, we should add an assert. > Source/WebCore/bindings/js/SerializedScriptValue.h:172 > + void* buffer = Gigacage::tryMalloc(Gigacage::Primitive, bufferSize); For your consideration I suggest we change the data type of this local from void* to uint8_t*. Then we can remove the static_cast<> below. Created attachment 338361 [details]
Patch for landing
Comment on attachment 338142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338142&action=review >> Source/WebCore/bindings/js/SerializedScriptValue.h:171 >> + > > Does it ever make sense that bufferSize is zero? If not, we should add an assert. When the array is empty it will have size 0. I also tried skipping the malloc in case the array is empty, but unfortunately, that doesn't work since the array is considered neutered if the buffer is null. >> Source/WebCore/bindings/js/SerializedScriptValue.h:172 >> + void* buffer = Gigacage::tryMalloc(Gigacage::Primitive, bufferSize); > > For your consideration I suggest we change the data type of this local from void* to uint8_t*. Then we can remove the static_cast<> below. I tried that, but it failed to compiled. I could have casted it here instead, but that didn't seem better. Comment on attachment 338361 [details] Patch for landing Clearing flags on attachment: 338361 Committed r230828: <https://trac.webkit.org/changeset/230828> All reviewed patches have been landed. Closing bug. |
Test case: <html> <body> <script> var channel = new MessageChannel(); channel.port1.addEventListener("message", (event) => { console.log("port1: message received"); // data SHOULD defined and containing an ArrayBuffer named buf if (event.data == null) { console.error("message data null!"); return; } var data = event.data; event.target.postMessage({ buf: data.buf }, [data.buf]); }); channel.port2.addEventListener("message", (event) => { console.log("port2: message received"); var data = event.data; console.log(data.buf); }); channel.port1.start(); channel.port2.start(); var arr = new Float64Array(); channel.port2.postMessage({ buf: arr.buffer }, [arr.buffer]); </script> </body> </html> Transferring an ArrayBuffer using MessagePort.postMessage() fails in Safari 11.1. The data in the received event by port1 is null. This used to work in previous stable Safari release.