Bug 18725 – Make postMessage asynchronous

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Adam Barth]

Ian has updated the HTML5 spec for postMessage in two ways.

1) The message is not delivered asynchronously.

2) The targetOrigin parameter is now required.  If the page passes the value "*", the browser can deliver the message to any origin.

We should update our implementation to match the spec.

[Adam Barth]

I have this implemented in my tree, but I'd like to write a bunch more tests.  I can upload a patch once Bug 17331 lands (so I can made a diff easily).

I didn't use FrameView::scheduleEvent to schedule the event because we need to do the access check for targetOrigin synchronously with dispatching the event and we want the MessageEvent to bind to the DOMWindow during navigation (not to its target, the document).  Instead, I used a design similar to HTMLMediaElement::dispatchEventAsync, storing the pending MessageEvents in DOMWindow.  This should work correctly with respect to frame navigation but means MessageEvents will not respect FrameView::pauseScheduledEvents and FrameView::resumeScheduledEvents.  Does this design sound reasonable?

[Adam Barth]

Created attachment 20850 [details]
Work in progress

Here is a work-in-progress version of the patch.  I haven't started working on the LayoutTests yet.

[Adam Barth]

Created attachment 20851 [details]
Work in progress

Oops.  Some old code snuck into that last patch.

[Jeff Walden (remove +bwo to email)]

Note also that the target of events changed from document to window.

I don't think the "clustering" of events you appear to implement is sound (they should interleave with other events), but I'm having a hard time thinking of how a webpage could actually have problems.

[Adam Barth]

Thanks for your comments Jeff.

(In reply to comment #4)
> Note also that the target of events changed from document to window.

There is a separate bug about that (Bug 18771).

> I don't think the "clustering" of events you appear to implement is sound
> (they should interleave with other events), but I'm having a hard time
> thinking of how a webpage could actually have problems.

Ok.  I'll figure out a way to integrate the events with the main event queue in FrameView.  Maybe adding a "custom dispatch" flag to events and a virtual function?

I'm going to be out of town for the next few days.  I'll post a revised patch either later this week or this weekend.

[Adam Barth]

> Ok.  I'll figure out a way to integrate the events with the main event queue
> in FrameView.

Hum...  Upon further investigation, it looks like the event queue in FrameView is something specially for delaying events during layout and uses the same event "clustering" pattern.  In fact, there are a number of other examples of this event clustering pattern in our codebase.  For example, Document::dispatchImageLoadEventSoon uses the same pattern.

One option is create an TaskQueue class that knows how to queue up "Tasks" using timers and run them in the proper order with respect to the main event loop.  Based on my current understanding, this would require creating one Timer object for each Task.

I'm inclined to keep the current implementation, but I can code up the TaskQueue approach if you'd like to see what that might look like.

[Adam Barth]

Created attachment 20896 [details]
Work in progress, no clustering

This turned out to be easier than I thought using the a pattern from the implementation of setTimeout.  I think this patch is wrong because the Timers can outlive the DOMWindow, but I'll work that out in the next update.

[Collin Jackson]

Created attachment 20978 [details]
Patch

[Sam Weinig]

Comment on attachment 20978 [details]
Patch

I assume this is due to a change in the spec.  Could you note this in the changelog.
-    : Event(messageEvent, true, true)
+    : Event(messageEvent, false, true)
I don't think these need to take PassRefPtrs.  You are not transferring ownership, you just want the timer to ref the objects.
+    PostMessageTimer(PassRefPtr<DOMWindow> window, PassRefPtr<MessageEvent> event, PassRefPtr<SecurityOrigin> targetOrigin)


The leading { should be on its own line.  I am not sure about the 'delete this'.  In the DOMWindowTimer case, we have the the postMessageTimerFired equivalent in JSDOMWindowBase delete the timer.  I think either is fine though.
+    virtual void fired() {
+        m_window->postMessageTimerFired(this);
+        delete this;
+    }

I think this would look better on one line.
+    PostMessageTimer* timer = new PostMessageTimer(
+            this, new MessageEvent(message, sourceOrigin, source), target);

Is the actual security check done in the postMessageTimerFired because of a possibility that the document has changed since the timer was fired?

This looks great.

r=me

[Collin Jackson]

Created attachment 20989 [details]
Updated patch to address Sam's comments

(In reply to comment #9)
> (From update of attachment 20978 [details] [edit])
> I assume this is due to a change in the spec.  Could you note this in the
> changelog.

Yes. I updated the changelog to mention the spec revision.

> -    : Event(messageEvent, true, true)
> +    : Event(messageEvent, false, true)
> I don't think these need to take PassRefPtrs.  You are not transferring
> ownership, you just want the timer to ref the objects.
> +    PostMessageTimer(PassRefPtr<DOMWindow> window, PassRefPtr<MessageEvent>
> event, PassRefPtr<SecurityOrigin> targetOrigin)

Done.

> The leading { should be on its own line.  I am not sure about the 'delete
> this'.  In the DOMWindowTimer case, we have the the postMessageTimerFired
> equivalent in JSDOMWindowBase delete the timer.  I think either is fine though.
> +    virtual void fired() {
> +        m_window->postMessageTimerFired(this);
> +        delete this;
> +    }

Changed.

> I think this would look better on one line.
> +    PostMessageTimer* timer = new PostMessageTimer(
> +            this, new MessageEvent(message, sourceOrigin, source), target);

Done.

> Is the actual security check done in the postMessageTimerFired because of a
> possibility that the document has changed since the timer was fired?

Well, it could have changed since the timer was *scheduled*. I added a comment to this effect.

[Sam Weinig]

Fix landed in r32922.