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

Define messageerror #2530

Merged
merged 1 commit into from Apr 24, 2017

Conversation

2 participants
@annevk
Member

annevk commented Apr 13, 2017

And remove StructuredCloneWithTransfer as deserializing errors need to
be handled on their own, to be consistent with MessageChannel.

This helps with #2260 and fixes #935.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 13, 2017

Member

I decided to separate this out into its own PR because it's a rather big change and deserves its own scrutiny.

A thing I noted is that we don't seem to initialize these MessageEvent objects with much context usually. I wonder if that's correct or if the specification has some holes.

Member

annevk commented Apr 13, 2017

I decided to separate this out into its own PR because it's a rather big change and deserves its own scrutiny.

A thing I noted is that we don't seem to initialize these MessageEvent objects with much context usually. I wonder if that's correct or if the specification has some holes.

@domenic

At first I was surprised at using MessageEvent for this. But looking into it further it seems like it's pretty standard practice to fill out only a few fields in MessageEvent. So, OK, MessageEvent is probably the right choice.

In the future we could consider filling in ports and data, by defining StructuredDeserialize to return a censored version of the data on failure (e.g. un-deserializable things replaced with null). We should wait until someone asks, though, IMO.

Show outdated Hide outdated source
</li>
<li><p>Let <var>eventName</var> be <code
data-x="event-messageerror">messageerror</code>.</p></li>

This comment has been minimized.

@domenic

domenic Apr 13, 2017

Member

This never gets set to "message" in the success case.

@domenic

domenic Apr 13, 2017

Member

This never gets set to "message" in the success case.

Show outdated Hide outdated source
<code>MessagePort</code> objects in <var>cloneRecord</var>.[[TransferredValues]], if any,
maintaining their relative order.</p></li>
</ol>
</li>
<li><p>Return, but continue running these steps in <span>in parallel</span>.</p></li>

This comment has been minimized.

@domenic

domenic Apr 13, 2017

Member

I wish I remembered why we go in parallel here. We should add a note if we can remember. It seems a bit weird to check the origin stuff while in parallel. I would have expected everything after serialize to be in the queued task, but nothing to be in parallel. Similar to postMessage() for MessagePort.

As-is it causes an awkward split where you have to set up all the data ahead of time before going in parallel, then fire the event etc. I guess I can't see a good way around that, unless we're comfortable changing this to be more like MessagePort.

@domenic

domenic Apr 13, 2017

Member

I wish I remembered why we go in parallel here. We should add a note if we can remember. It seems a bit weird to check the origin stuff while in parallel. I would have expected everything after serialize to be in the queued task, but nothing to be in parallel. Similar to postMessage() for MessagePort.

As-is it causes an awkward split where you have to set up all the data ahead of time before going in parallel, then fire the event etc. I guess I can't see a good way around that, unless we're comfortable changing this to be more like MessagePort.

This comment has been minimized.

@annevk

annevk Apr 14, 2017

Member

I think it uses that to give the illusion it could be a separate process, without actually going to the length required in other parts of the system to make that possible (such as actually duplicating Window and Location objects and set them up as message passing systems).

I'm okay with dropping it or putting all state in variables beforehand.

@annevk

annevk Apr 14, 2017

Member

I think it uses that to give the illusion it could be a separate process, without actually going to the length required in other parts of the system to make that possible (such as actually duplicating Window and Location objects and set them up as message passing systems).

I'm okay with dropping it or putting all state in variables beforehand.

This comment has been minimized.

@annevk

annevk Apr 14, 2017

Member

Though we should make sure to create the frozen array within the task, otherwise it has the wrong global... That's broken too.

@annevk

annevk Apr 14, 2017

Member

Though we should make sure to create the frozen array within the task, otherwise it has the wrong global... That's broken too.

This comment has been minimized.

@domenic

domenic Apr 14, 2017

Member

OK cool, I pushed a commit that changes that. We should remember to mention it in the ultimate commit message. Now off to work on tests...

@domenic

domenic Apr 14, 2017

Member

OK cool, I pushed a commit that changes that. We should remember to mention it in the ultimate commit message. Now off to work on tests...

<code>MessageEvent</code>, with the <code data-x="dom-MessageEvent-data">data</code> attribute
initialized to <var>messageClone</var> and the <code
data-x="dom-MessageEvent-ports">ports</code> attribute initialized to
<var>newPorts</var>.</p></li>

This comment has been minimized.

@domenic

domenic Apr 13, 2017

Member

Nice cleanup.

@domenic

domenic Apr 13, 2017

Member

Nice cleanup.

Show outdated Hide outdated source
@@ -96129,7 +96131,7 @@ interface <dfn>MessagePort</dfn> : <span>EventTarget</span> {
<tr><th><span data-x="event handlers">Event handler</span> <th><span>Event handler event type</span>
<tbody>
<tr><td><dfn><code data-x="handler-MessagePort-onmessage">onmessage</code></dfn> <td> <code data-x="event-message">message</code>
<!--ILLFATED <tr><td><dfn><code data-x="handler-MessagePort-onerror">onerror</code></dfn> <td> <code data-x="event-error">error</code>
<tr><td><dfn><code data-x="handler-MessagePort-onmessageerror">onmessageerror</code></dfn> <td> <code data-x="event-messageerror">messageerror</code><!--ILLFATED <tr><td><dfn><code data-x="handler-MessagePort-onerror">onerror</code></dfn> <td> <code data-x="event-error">error</code>

This comment has been minimized.

@domenic

domenic Apr 13, 2017

Member

Any reason to squash the comment and the new event onto the same line? Separate seems nicer.

@domenic

domenic Apr 13, 2017

Member

Any reason to squash the comment and the new event onto the same line? Separate seems nicer.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 13, 2017

Member

We should as part of committing this add tests for the new onmessageerror IDL attributes and HTML reflection for body. Tests of the actual functionality can land with #2518.

I will try to work on the tests myself ASAP, but feel free to start if lack of tests is blocking you and I'm asleep/busy. I think you're on holiday for a bit though :).

Member

domenic commented Apr 13, 2017

We should as part of committing this add tests for the new onmessageerror IDL attributes and HTML reflection for body. Tests of the actual functionality can land with #2518.

I will try to work on the tests myself ASAP, but feel free to start if lack of tests is blocking you and I'm asleep/busy. I think you're on holiday for a bit though :).

domenic added a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2017

Add tests for the messageerror IDL and content attributes
Follows whatwg/html#2530. As explained there, the circumstances in which this would actually fire are not tested in this PR, waiting instead for #5003.

domenic added a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2017

Add tests for the messageerror IDL and content attributes
Follows whatwg/html#2530. As explained there, the circumstances in which this would actually be fired by the UA are not tested in this PR, waiting instead for #5003.

As a side effect this syncs various IDL snippets with the HTML Standard.

@domenic domenic removed the needs tests label Apr 14, 2017

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 14, 2017

Member

So after my changes this has my LGTM, and now it has tests, so if you like it we can merge it.

Member

domenic commented Apr 14, 2017

So after my changes this has my LGTM, and now it has tests, so if you like it we can merge it.

@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 15, 2017

Member

Up to you, we could wait a bit for Apple to reply. It seems like Chrome is on board with this plan and I think Firefox is too. The only thing this is blocking is the final PR at this point and I'm probably not going to be working on that before Tuesday and now everything is in the nitpick stage it should be easy enough to base that on this and the agents PR.

Member

annevk commented Apr 15, 2017

Up to you, we could wait a bit for Apple to reply. It seems like Chrome is on board with this plan and I think Firefox is too. The only thing this is blocking is the final PR at this point and I'm probably not going to be working on that before Tuesday and now everything is in the nitpick stage it should be easy enough to base that on this and the agents PR.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 17, 2017

Member

Note that the service worker spec will need this too. One of us can work on that as appropriate; I'm assuming it'll be there for some SAB tests I'm writing (although we'll also need IDL tests for the presence of the IDL attribute).

Member

domenic commented Apr 17, 2017

Note that the service worker spec will need this too. One of us can work on that as appropriate; I'm assuming it'll be there for some SAB tests I'm writing (although we'll also need IDL tests for the presence of the IDL attribute).

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 18, 2017

Member

Another somewhat strange thing is how worker.postMessage/messagePort.postMessage does not set the origin property of the MessageEvent, but serviceWorker.postMessage does. We can carry that over into messageerror I guess, but maybe someone should look at it later... I guess I'll file an issue. w3c/ServiceWorker#1122

Member

domenic commented Apr 18, 2017

Another somewhat strange thing is how worker.postMessage/messagePort.postMessage does not set the origin property of the MessageEvent, but serviceWorker.postMessage does. We can carry that over into messageerror I guess, but maybe someone should look at it later... I guess I'll file an issue. w3c/ServiceWorker#1122

@domenic domenic referenced this pull request Apr 18, 2017

Merged

WIP: SharedArrayBuffer structured cloning tests #5003

19 of 21 tasks complete
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 18, 2017

Member

I already filed w3c/ServiceWorker#1116 on service workers by the way. Should maybe mention that in the commit message.

Member

annevk commented Apr 18, 2017

I already filed w3c/ServiceWorker#1116 on service workers by the way. Should maybe mention that in the commit message.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 18, 2017

Member

I think we should ideally line up the PR for them; that's my plan at least.

Member

domenic commented Apr 18, 2017

I think we should ideally line up the PR for them; that's my plan at least.

domenic added a commit to domenic/ServiceWorker that referenced this pull request Apr 20, 2017

Handle deserialization failures in postMessage()
This is part of whatwg/html#2530; see related links there for more information.

domenic added a commit to domenic/ServiceWorker that referenced this pull request Apr 20, 2017

Handle deserialization failures in postMessage()
This is part of whatwg/html#2530; see related links there for more information. Closes #1116.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 20, 2017

Member

Should .onmessageerror = trigger the message queue similar to .onmessage = ?

Member

domenic commented Apr 20, 2017

Should .onmessageerror = trigger the message queue similar to .onmessage = ?

domenic added a commit to domenic/ServiceWorker that referenced this pull request Apr 20, 2017

Handle deserialization failures in postMessage()
This is part of whatwg/html#2530; see related links there for more information. Closes #1116.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 21, 2017

Member

@domenic I would say no, as that means you can miss a message (if you add that later for some reason).

Member

annevk commented Apr 21, 2017

@domenic I would say no, as that means you can miss a message (if you add that later for some reason).

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 21, 2017

Member

Sounds good, just wanted to check.

Member

domenic commented Apr 21, 2017

Sounds good, just wanted to check.

Define messageerror
The messageerror event is used when deserialization fails. E.g., when
an ArrayBuffer object cannot be allocated.

This also removes StructuredCloneWithTransfer as deserializing errors
now need to be handled on their own.

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

Service workers follow-up:
w3c/ServiceWorker#1116.

Fixes part of #2260 and fixes #935.
@annevk

This comment has been minimized.

Show comment
Hide comment
@annevk

annevk Apr 24, 2017

Member

Since everything seems to be agreed upon here I went ahead and filed browser bugs. I haven't merged things yet, but I would be okay if we did that. Will wait for one more person to agree. (This can be "rebased and merged" as I made sure to clean up the commit message.)

https://bugzilla.mozilla.org/show_bug.cgi?id=1359017
https://bugs.webkit.org/show_bug.cgi?id=171216
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11764266/
https://bugs.chromium.org/p/chromium/issues/detail?id=714582

Member

annevk commented Apr 24, 2017

Since everything seems to be agreed upon here I went ahead and filed browser bugs. I haven't merged things yet, but I would be okay if we did that. Will wait for one more person to agree. (This can be "rebased and merged" as I made sure to clean up the commit message.)

https://bugzilla.mozilla.org/show_bug.cgi?id=1359017
https://bugs.webkit.org/show_bug.cgi?id=171216
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/11764266/
https://bugs.chromium.org/p/chromium/issues/detail?id=714582

@domenic domenic merged commit 25a94f6 into master Apr 24, 2017

2 checks passed

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

@domenic domenic deleted the annevk/messageerror branch Apr 24, 2017

domenic added a commit to web-platform-tests/wpt that referenced this pull request Apr 24, 2017

Add tests for the messageerror IDL and content attributes
Follows whatwg/html#2530. As explained there, the circumstances in which this would actually be fired by the UA are not tested in this PR, waiting instead for #5003.

As a side effect this syncs various IDL snippets with the HTML Standard.

domenic added a commit to domenic/ServiceWorker that referenced this pull request Apr 24, 2017

Handle deserialization failures in postMessage()
This is part of whatwg/html#2530; see related links there for more information. Closes #1116.

Also makes some minor updates to stop referencing terms that have been removed from HTML.

domenic added a commit to domenic/ServiceWorker that referenced this pull request Apr 28, 2017

Handle deserialization failures in postMessage()
This is part of whatwg/html#2530; see related links there for more information. Closes #1116.

Also makes some minor updates to stop referencing terms that have been removed from HTML.

jungkees added a commit to w3c/ServiceWorker that referenced this pull request Apr 29, 2017

Handle deserialization failures in postMessage() (#1130)
This is part of whatwg/html#2530; see related links there for more information. Closes #1116.

Also makes some minor updates to stop referencing terms that have been removed from HTML.

scheib pushed a commit to scheib/chromium that referenced this pull request May 13, 2017

Add messageerror event handler
This event is not currently wired up anywhere, but will be used in a
subsequent CL. The idea is that when posting a message that cannot be
decoded by the receiver, the receiver will dispatch a messageerror event
instead of a message event.

Spec changes: whatwg/html#2530
web platform tests: web-platform-tests/wpt#5567
Intent-to-implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Z_XzejHJTrs

BUG=chromium:714842

Review-Url: https://codereview.chromium.org/2860483002
Cr-Commit-Position: refs/heads/master@{#471491}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment