Define messageerror #2530

annevk · 2017-04-13T11:36:34Z

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 · 2017-04-13T11:38:17Z

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.

domenic · 2017-04-13T22:28:05Z

source

+   </li>
+
+   <li><p>Let <var>eventName</var> be <code
+   data-x="event-messageerror">messageerror</code>.</p></li>


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

domenic · 2017-04-13T22:31:43Z

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>


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.

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.

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

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 · 2017-04-13T22:34:47Z

source

+     <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>


Nice cleanup.

domenic · 2017-04-13T22:35:53Z

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>


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

domenic · 2017-04-13T22:45:57Z

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

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.

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 · 2017-04-14T20:57:44Z

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

annevk · 2017-04-15T05:27:04Z

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 · 2017-04-17T23:29:33Z

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 · 2017-04-18T00:41:41Z

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

annevk · 2017-04-18T08:12:21Z

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

domenic · 2017-04-18T13:08:58Z

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

This is part of whatwg/html#2530; see related links there for more information.

This is part of whatwg/html#2530; see related links there for more information. Closes w3c#1116.

domenic · 2017-04-20T20:53:04Z

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

This is part of whatwg/html#2530; see related links there for more information. Closes w3c#1116.

annevk · 2017-04-21T05:04:56Z

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

domenic · 2017-04-21T21:33:30Z

Sounds good, just wanted to check.

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 · 2017-04-24T11:07:41Z

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

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.

This is part of whatwg/html#2530; see related links there for more information. Closes w3c#1116. Also makes some minor updates to stop referencing terms that have been removed from HTML.

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.

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}

https://bugs.webkit.org/show_bug.cgi?id=171216 rdar://96467919 Reviewed by Darin Adler. Implement messageerror event: - whatwg/html#2530 Previously, WebKit would fire a `message` event with its `data` being null, whenever data deserialization would fail. This wasn't as per specification and did not match other browser engines. We're supposed to fire a `messageerror` event instead. This patch addresses the issue. * LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/event-handler-attributes-body-window-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/webappapis/scripting/events/event-handler-attributes-windowless-body-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/broadcastchannel-success-and-failure-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/wasm/serialization/module/window-sharedworker-failure-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/webmessaging/messageerror-expected.txt: * Source/WebCore/bindings/js/SerializedScriptValue.cpp: (WebCore::SerializedScriptValue::deserialize): * Source/WebCore/bindings/js/SerializedScriptValue.h: * Source/WebCore/dom/BroadcastChannel.cpp: (WebCore::BroadcastChannel::dispatchMessage): * Source/WebCore/dom/MessageEvent.cpp: (WebCore::MessageEvent::create): * Source/WebCore/dom/MessageEvent.h: * Source/WebCore/dom/MessagePort.cpp: (WebCore::MessagePort::dispatchMessages): * Source/WebCore/dom/MessagePort.idl: * Source/WebCore/html/HTMLAttributeNames.in: * Source/WebCore/page/DOMWindow.cpp: (WebCore::DOMWindow::postMessage): * Source/WebCore/page/WindowEventHandlers.idl: * Source/WebCore/workers/DedicatedWorkerGlobalScope.idl: * Source/WebCore/workers/Worker.idl: * Source/WebCore/workers/WorkerMessagingProxy.cpp: (WebCore::WorkerMessagingProxy::postMessageToWorkerObject): (WebCore::WorkerMessagingProxy::postMessageToWorkerGlobalScope): * Source/WebCore/workers/service/ServiceWorkerContainer.cpp: (WebCore::ServiceWorkerContainer::startMessages): (WebCore::ServiceWorkerContainer::postMessage): * Source/WebCore/workers/service/ServiceWorkerContainer.h: * Source/WebCore/workers/service/context/ServiceWorkerThread.cpp: (WebCore::fireMessageEvent): Canonical link: https://commits.webkit.org/256896@main

This was referenced Apr 13, 2017

Remove StructuredCloneWithTransfer and add a messageerror event w3c/ServiceWorker#1116

Closed

Revamp of SharedArrayBuffer PR #2518

Merged

Integrate JavaScript SharedArrayBuffers and agents/agent clusters #2361

Closed

domenic reviewed Apr 13, 2017

View reviewed changes

domenic added addition/proposal New features or enhancements topic: events needs tests Moving the issue forward requires someone to write tests labels Apr 13, 2017

domenic mentioned this pull request Apr 14, 2017

Add tests for the messageerror IDL and content attributes web-platform-tests/wpt#5567

Merged

domenic removed the needs tests Moving the issue forward requires someone to write tests label Apr 14, 2017

domenic mentioned this pull request Apr 14, 2017

Define agents and agent clusters #2521

Merged

annevk added the topic: serialize and transfer label Apr 15, 2017

annevk force-pushed the annevk/messageerror branch from ca310d7 to c6ffb89 Compare April 16, 2017 04:52

domenic mentioned this pull request Apr 18, 2017

WIP: SharedArrayBuffer structured cloning tests web-platform-tests/wpt#5003

Merged

21 tasks

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

Handle deserialization failures in postMessage()

6de2a8e

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

e0bf9c6

This is part of whatwg/html#2530; see related links there for more information. Closes w3c#1116.

domenic mentioned this pull request Apr 20, 2017

Handle deserialization failures in postMessage() w3c/ServiceWorker#1130

Merged

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

Handle deserialization failures in postMessage()

2b481ca

This is part of whatwg/html#2530; see related links there for more information. Closes w3c#1116.

annevk force-pushed the annevk/messageerror branch from f0e233d to 3497e8f Compare April 24, 2017 10:53

domenic merged commit 25a94f6 into master Apr 24, 2017

domenic deleted the annevk/messageerror branch April 24, 2017 22:30

cdumez mentioned this pull request Nov 18, 2022

Implement messageerror event WebKit/WebKit#6654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define messageerror #2530

Define messageerror #2530

annevk commented Apr 13, 2017

annevk commented Apr 13, 2017

domenic left a comment

domenic Apr 13, 2017

domenic Apr 13, 2017

annevk Apr 14, 2017

annevk Apr 14, 2017

domenic Apr 14, 2017

domenic Apr 13, 2017

domenic Apr 13, 2017

domenic commented Apr 13, 2017

domenic commented Apr 14, 2017

annevk commented Apr 15, 2017

domenic commented Apr 17, 2017

domenic commented Apr 18, 2017 •

edited

Loading

annevk commented Apr 18, 2017

domenic commented Apr 18, 2017

domenic commented Apr 20, 2017

annevk commented Apr 21, 2017

domenic commented Apr 21, 2017

annevk commented Apr 24, 2017

Define messageerror #2530

Define messageerror #2530

Conversation

annevk commented Apr 13, 2017

annevk commented Apr 13, 2017

domenic left a comment

Choose a reason for hiding this comment

domenic Apr 13, 2017

Choose a reason for hiding this comment

domenic Apr 13, 2017

Choose a reason for hiding this comment

annevk Apr 14, 2017

Choose a reason for hiding this comment

annevk Apr 14, 2017

Choose a reason for hiding this comment

domenic Apr 14, 2017

Choose a reason for hiding this comment

domenic Apr 13, 2017

Choose a reason for hiding this comment

domenic Apr 13, 2017

Choose a reason for hiding this comment

domenic commented Apr 13, 2017

domenic commented Apr 14, 2017

annevk commented Apr 15, 2017

domenic commented Apr 17, 2017

domenic commented Apr 18, 2017 • edited Loading

annevk commented Apr 18, 2017

domenic commented Apr 18, 2017

domenic commented Apr 20, 2017

annevk commented Apr 21, 2017

domenic commented Apr 21, 2017

annevk commented Apr 24, 2017

domenic commented Apr 18, 2017 •

edited

Loading