Bug 184254

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:

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:

Description	Flags
HTML page of the test case	none
The worker script of the test case	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch for landing	none

Yann Cabon

Reported 2018-04-02 21:44:35 PDT

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.

Attachments
HTML page of the test case (996 bytes, text/html) 2018-04-03 10:37 PDT, Yann Cabon	no flags	Details
The worker script of the test case (679 bytes, application/javascript) 2018-04-03 10:38 PDT, Yann Cabon	no flags	Details
Patch (10.29 KB, patch) 2018-04-05 15:15 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (10.46 KB, patch) 2018-04-05 15:51 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (10.61 KB, patch) 2018-04-09 14:16 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (12.64 KB, patch) 2018-04-09 19:41 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (12.85 KB, patch) 2018-04-10 11:15 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (11.47 KB, patch) 2018-04-11 14:02 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (11.92 KB, patch) 2018-04-16 11:41 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch (11.95 KB, patch) 2018-04-17 12:27 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Patch for landing (11.94 KB, patch) 2018-04-19 14:52 PDT, Tadeu Zagallo	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-04-03 07:23:51 PDT

<rdar://problem/39140200>

Yann Cabon

Comment 2 2018-04-03 10:37:34 PDT

Created attachment 337087 [details] HTML page of the test case

Yann Cabon

Comment 3 2018-04-03 10:37:44 PDT

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.

Yann Cabon

Comment 4 2018-04-03 10:38:04 PDT

Created attachment 337088 [details] The worker script of the test case

Tadeu Zagallo

Comment 5 2018-04-05 15:15:05 PDT

Created attachment 337306 [details] Patch

Chris Dumez

Comment 6 2018-04-05 15:24:18 PDT

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.

Tadeu Zagallo

Comment 7 2018-04-05 15:39:07 PDT

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).

Jeremy Jones

Comment 8 2018-04-05 15:44:52 PDT

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.

Tadeu Zagallo

Comment 9 2018-04-05 15:51:43 PDT

Created attachment 337308 [details] Patch

Tadeu Zagallo

Comment 10 2018-04-05 15:53:33 PDT

(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.

Tadeu Zagallo

Comment 11 2018-04-09 14:16:59 PDT

Created attachment 337540 [details] Patch

Tadeu Zagallo

Comment 12 2018-04-09 19:41:50 PDT

Created attachment 337575 [details] Patch

Daniel Bates

Comment 13 2018-04-09 22:03:06 PDT

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?

Daniel Bates

Comment 14 2018-04-09 22:07:17 PDT

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.

Daniel Bates

Comment 15 2018-04-09 22:09:21 PDT

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?

Tadeu Zagallo

Comment 16 2018-04-10 10:27:09 PDT

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.

Tadeu Zagallo

Comment 17 2018-04-10 11:15:48 PDT

Created attachment 337622 [details] Patch

Daniel Bates

Comment 18 2018-04-10 20:35:54 PDT

(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?

Tadeu Zagallo

Comment 19 2018-04-11 14:02:29 PDT

Created attachment 337732 [details] Patch

Tadeu Zagallo

Comment 20 2018-04-11 14:05:50 PDT

(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!

Daniel Bates

Comment 21 2018-04-14 09:52:20 PDT

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?

Ryosuke Niwa

Comment 22 2018-04-16 11:32:31 PDT

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 ":".

Tadeu Zagallo

Comment 23 2018-04-16 11:41:52 PDT

Created attachment 338017 [details] Patch

Brady Eidson

Comment 24 2018-04-16 13:08:56 PDT

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

Ryosuke Niwa

Comment 25 2018-04-17 00:42:07 PDT

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?

Tadeu Zagallo

Comment 26 2018-04-17 12:27:26 PDT

Created attachment 338142 [details] Patch

Daniel Bates

Comment 27 2018-04-17 13:31:27 PDT

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.

Tadeu Zagallo

Comment 28 2018-04-19 14:52:26 PDT

Created attachment 338361 [details] Patch for landing

Tadeu Zagallo

Comment 29 2018-04-19 14:56:59 PDT

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.

WebKit Commit Bot

Comment 30 2018-04-19 16:59:56 PDT

Comment on attachment 338361 [details] Patch for landing Clearing flags on attachment: 338361 Committed r230828: <https://trac.webkit.org/changeset/230828>

WebKit Commit Bot

Comment 31 2018-04-19 16:59:58 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.