Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test postMessage() exception order #9672

Merged
merged 5 commits into from Mar 21, 2018

Conversation

Projects
None yet
6 participants
@annevk
Copy link
Member

annevk commented Feb 26, 2018

Helps with whatwg/html#3508.

@wpt-pr-bot wpt-pr-bot added the html label Feb 26, 2018

@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm, jgraham and zqzhang Feb 26, 2018

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 26, 2018

@surma this might interest you a bit. I haven't really tackled generalizing any of this. Just adding more corner cases.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 26, 2018

It seems that Edge/Safari generally follow the specification here. Chrome/Firefox seem to handle the transfer errors later. I think either way we have to loop over the transferables before structured serializing to store them in memory, so there's probably no good reason to change the specification here.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 26, 2018

Safari not throwing for duplicates: https://bugs.webkit.org/show_bug.cgi?id=183124.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Feb 26, 2018

And finally for Edge throwing the wrong exception for one test: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/16104788/.

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 26, 2018

Build PASSED

Started: 2018-03-20 17:56:57
Finished: 2018-03-20 18:06:48

View more information about this build on:

annevk added a commit to whatwg/html that referenced this pull request Feb 26, 2018

Duplicates in StructuredSerializeWithTransfer's transferList
Duplicates are not allowed in most implementations and throw an error (before any serialization exceptions). This aligns the standard with that behavior.

Tests: web-platform-tests/wpt#9672.

Fixes #3507.

@annevk annevk requested a review from mkruisselbrink Feb 27, 2018

@@ -0,0 +1,27 @@
function assert_transfer_error(transferList) {
assert_throws("DATA_CLONE_ERR", () => self.postMessage({ get whatever() { throw 1 } }, "*", transferList));

This comment has been minimized.

Copy link
@mkruisselbrink

mkruisselbrink Mar 9, 2018

Contributor

nit: DataCloneError (at least http://web-platform-tests.org/writing-tests/testharness-api.html says the _ERR form is only supported "for compatibility with existing tests")

nit

annevk added a commit to whatwg/html that referenced this pull request Mar 11, 2018

Check for transferable detachedness after serializing
Since serializing can make a transferable detached, it seems better to throw for that later.

This matches Chrome/Safari. Firefox ends up with a detached transferable. Edge crashes.

See https://bugs.chromium.org/p/chromium/issues/detail?id=816447 for more context.

Tests: ... (should update web-platform-tests/wpt#9672)

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 12, 2018

Throw a DataCloneError if a neutered ArrayBuffer occurs in the transf…
…er list.

Currently the exception is not thrown until after serialization.
This makes us consistent with Gecko and WebKit, which throw before serialization.

This fixes the second test case in web-platform-tests/wpt#9672
(not yet pushed to upstream WPT).

Bug: 816447
Change-Id: I30b798b5d21dc8d6f2a40d049ced6aa7f60dd090
Reviewed-on: https://chromium-review.googlesource.com/956254
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542484}
@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 14, 2018

I updated the tests to match the proposed change over at whatwg/html#3557.

@annevk annevk requested a review from mkruisselbrink Mar 14, 2018

aarongable pushed a commit to chromium/chromium that referenced this pull request Mar 14, 2018

Revert "Throw a DataCloneError if a neutered ArrayBuffer occurs in th…
…e transfer list."

This reverts commit ade1dc7.

Reason for revert: Further discussion on the bug (https://bugs.chromium.org/p/chromium/issues/detail?id=816447#c8); moving towards Chrome's previous behavior.

Original change's description:
> Throw a DataCloneError if a neutered ArrayBuffer occurs in the transfer list.
> 
> Currently the exception is not thrown until after serialization.
> This makes us consistent with Gecko and WebKit, which throw before serialization.
> 
> This fixes the second test case in web-platform-tests/wpt#9672
> (not yet pushed to upstream WPT).
> 
> Bug: 816447
> Change-Id: I30b798b5d21dc8d6f2a40d049ced6aa7f60dd090
> Reviewed-on: https://chromium-review.googlesource.com/956254
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Commit-Queue: Jeremy Roman <jbroman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#542484}

TBR=jbroman@chromium.org,mek@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 816447
Change-Id: I79950f8ba97e5481a872cc4897d62506fb2381b0
Reviewed-on: https://chromium-review.googlesource.com/960926
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543092}
@jeremyroman

This comment has been minimized.

Copy link
Contributor

jeremyroman commented Mar 14, 2018

Thanks; I've reverted Chromium to its previous behavior, which matches the change in whatwg/html#3557.

@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 20, 2018

@jeremyroman could you perhaps review these tests? Suggestions for more tests?

@jeremyroman
Copy link
Contributor

jeremyroman left a comment

Makes sense to me overall.

}, "Serialize throws before a transferred detached ArrayBuffer is found");

promise_test(() => {
return self.createImageBitmap(document.createElement("canvas")).then(bitmap => {

This comment has been minimized.

Copy link
@jeremyroman

jeremyroman Mar 20, 2018

Contributor

Do we want to be more exhaustive? We could do something like:

function transfer_tests(name, create) {
  promise_test(async () => {
    const transferable = await create();
    assert_transfer_error([transferable, transferable]);
  }, `Cannot transfer the same ${name} twice`);

  promise_test(async () => {
    const transferable = await create();
    self.postMessage(null, "*", [transferable]);
    assert_throws("DataCloneError", () => self.postMessage(null, "*", [transferable]);
  }, `Serialize should make the ${name} detached, so it cannot be transferred again.`);

  promise_test(async () => {
    const transferable = await create();
    self.postMessage(null, "*", [transferable]);
    assert_transfer_error([transferable], new Error("hi"));
  }, `Serialize should throw before a detached ${name} is found.`);

  promise_test(async () => {
    const transferable = await create();
    let seen = false;
    const message = {
      get a() {
        self.postMessage(null, '*', [transferable]);
        seen = true;
      }
    };
    assert_throws("DataCloneError", () => self.postMessage(message, "*", [transferable]));
    assert_true(seen);
  }, `Cannot transfer ${name} detached while the message was serialized.`);
}

transfer_tests("ArrayBuffer", () => new ArrayBuffer(1));
transfer_tests("MessagePort", () => new MessageChannel().port1);
transfer_tests("ImageBitmap", () => self.createImageBitmap(document.createElement("canvas")));
// OffscreenCanvas? I don't believe we have any other transferables, at least implemented in Blink.

That might be excessively clever, but I think the logic for all of these tests generalizes to all kinds of transferable checks, so why not check the whole matrix?

This comment has been minimized.

Copy link
@annevk

annevk Mar 20, 2018

Author Member

Yeah, why not. I'll make it so. Probably not today unfortunately.

@@ -0,0 +1,42 @@
function assert_transfer_error(transferList, exception = "DataCloneError") {
assert_throws(exception, () => self.postMessage({ get whatever() { throw exception } }, "*", transferList));

This comment has been minimized.

Copy link
@jeremyroman

jeremyroman Mar 20, 2018

Contributor

nit: Is this trying too hard to do what could be two separate functions? Throwing the string "DataCloneError" as the default behavior seems odd.

This comment has been minimized.

Copy link
@annevk

annevk Mar 20, 2018

Author Member

Will clean this up too.

annevk added some commits Mar 20, 2018

nit
@annevk

This comment has been minimized.

Copy link
Member Author

annevk commented Mar 20, 2018

Ended up making those fixes today anyway.

The one thing this loses is testing ImageBitmap's close() method, but that should probably be tested as part of that object.

@jeremyroman
Copy link
Contributor

jeremyroman left a comment

LGTM; thanks!

@annevk annevk merged commit fc04bb5 into master Mar 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the annevk/postMessage-exception-order branch Mar 21, 2018

annevk added a commit to whatwg/html that referenced this pull request Mar 21, 2018

Check for transferable detachedness after serializing
Since serializing can make a transferable detached, it seems better to throw for that later.

See https://bugs.chromium.org/p/chromium/issues/detail?id=816447 for more context.

Tests: web-platform-tests/wpt#9672.

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Duplicates in StructuredSerializeWithTransfer's transferList
Duplicates are not allowed in most implementations and throw an error (before any serialization exceptions). This aligns the standard with that behavior.

Tests: web-platform-tests/wpt#9672.

Fixes whatwg#3507.

alice added a commit to alice/html that referenced this pull request Jan 8, 2019

Check for transferable detachedness after serializing
Since serializing can make a transferable detached, it seems better to throw for that later.

See https://bugs.chromium.org/p/chromium/issues/detail?id=816447 for more context.

Tests: web-platform-tests/wpt#9672.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.