Bug 18725 - Make postMessage asynchronous
Summary: Make postMessage asynchronous
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: http://lists.whatwg.org/pipermail/wha...
Keywords:
Depends on: 17331
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-24 18:01 PDT by Adam Barth
Modified: 2008-05-06 15:04 PDT (History)
7 users (show)

See Also:


Attachments
Work in progress (9.02 KB, patch)
2008-04-27 10:50 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Work in progress (8.67 KB, patch)
2008-04-27 10:53 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Work in progress, no clustering (6.21 KB, patch)
2008-04-29 21:37 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (30.32 KB, patch)
2008-05-05 21:00 PDT, Collin Jackson
sam: review+
Details | Formatted Diff | Diff
Updated patch to address Sam's comments (30.52 KB, patch)
2008-05-06 14:21 PDT, Collin Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2008-04-24 18:01:08 PDT
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.
Comment 1 Adam Barth 2008-04-26 09:30:32 PDT
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?
Comment 2 Adam Barth 2008-04-27 10:50:22 PDT
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.
Comment 3 Adam Barth 2008-04-27 10:53:12 PDT
Created attachment 20851 [details]
Work in progress

Oops.  Some old code snuck into that last patch.
Comment 4 Jeff Walden (remove +bwo to email) 2008-04-27 13:12:44 PDT
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.
Comment 5 Adam Barth 2008-04-28 08:24:25 PDT
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.
Comment 6 Adam Barth 2008-04-29 20:46:29 PDT
> 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.
Comment 7 Adam Barth 2008-04-29 21:37:35 PDT
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.
Comment 8 Collin Jackson 2008-05-05 21:00:27 PDT
Created attachment 20978 [details]
Patch
Comment 9 Sam Weinig 2008-05-06 09:20:51 PDT
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
Comment 10 Collin Jackson 2008-05-06 14:21:01 PDT
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.
Comment 11 Sam Weinig 2008-05-06 15:04:08 PDT
Fix landed in r32922.