Bug 184254 - REGRESSION: MessagePort.postMessage() fails to send transferable objects
Summary: REGRESSION: MessagePort.postMessage() fails to send transferable objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 11
Hardware: Macintosh macOS 10.13
: P2 Major
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2018-04-02 21:44 PDT by Yann Cabon
Modified: 2018-04-19 16:59 PDT (History)
20 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yann Cabon 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.
Comment 1 Radar WebKit Bug Importer 2018-04-03 07:23:51 PDT
<rdar://problem/39140200>
Comment 2 Yann Cabon 2018-04-03 10:37:34 PDT
Created attachment 337087 [details]
HTML page of the test case
Comment 3 Yann Cabon 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.
Comment 4 Yann Cabon 2018-04-03 10:38:04 PDT
Created attachment 337088 [details]
The worker script of the test case
Comment 5 Tadeu Zagallo 2018-04-05 15:15:05 PDT
Created attachment 337306 [details]
Patch
Comment 6 Chris Dumez 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.
Comment 7 Tadeu Zagallo 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).
Comment 8 Jeremy Jones 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.
Comment 9 Tadeu Zagallo 2018-04-05 15:51:43 PDT
Created attachment 337308 [details]
Patch
Comment 10 Tadeu Zagallo 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.
Comment 11 Tadeu Zagallo 2018-04-09 14:16:59 PDT
Created attachment 337540 [details]
Patch
Comment 12 Tadeu Zagallo 2018-04-09 19:41:50 PDT
Created attachment 337575 [details]
Patch
Comment 13 Daniel Bates 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?
Comment 14 Daniel Bates 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.
Comment 15 Daniel Bates 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?
Comment 16 Tadeu Zagallo 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.
Comment 17 Tadeu Zagallo 2018-04-10 11:15:48 PDT
Created attachment 337622 [details]
Patch
Comment 18 Daniel Bates 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?
Comment 19 Tadeu Zagallo 2018-04-11 14:02:29 PDT
Created attachment 337732 [details]
Patch
Comment 20 Tadeu Zagallo 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!
Comment 21 Daniel Bates 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?
Comment 22 Ryosuke Niwa 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 ":".
Comment 23 Tadeu Zagallo 2018-04-16 11:41:52 PDT
Created attachment 338017 [details]
Patch
Comment 24 Brady Eidson 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
Comment 25 Ryosuke Niwa 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?
Comment 26 Tadeu Zagallo 2018-04-17 12:27:26 PDT
Created attachment 338142 [details]
Patch
Comment 27 Daniel Bates 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.
Comment 28 Tadeu Zagallo 2018-04-19 14:52:26 PDT
Created attachment 338361 [details]
Patch for landing
Comment 29 Tadeu Zagallo 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.
Comment 30 WebKit Commit Bot 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>
Comment 31 WebKit Commit Bot 2018-04-19 16:59:58 PDT
All reviewed patches have been landed.  Closing bug.