Bug 85815 – Add checkAndDispatchMessageEvent to DOMWindow

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Charles Reis]

In bug 73883, we gave embedders a willCheckAndDispatchMessageEvent.  This allows multi-process ports like Chromium to intercept postMessages and deliver them to the correct process, for bug 73337.

On the destination side, we need to do the target origin security check and deliver the message.  Instead of exposing the security check to embedders (see bug 75155), we can instead expose checkAndDispatchMessageEvent, which balances nicely with willCheckAndDispatchMessageEvent.

[Charles Reis]

Created attachment 140568 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexey Proskuryakov]

Comment on attachment 140568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140568&action=review

> Source/WebCore/page/DOMWindow.h:238
> +        void checkAndDispatchMessageEvent(SecurityOrigin* targetOrigin, PassRefPtr<Event>, PassRefPtr<ScriptCallStack>);

What does it mean to "check" an event?

[Adam Barth]

Comment on attachment 140568 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140568&action=review

>> Source/WebCore/page/DOMWindow.h:238
>> +        void checkAndDispatchMessageEvent(SecurityOrigin* targetOrigin, PassRefPtr<Event>, PassRefPtr<ScriptCallStack>);
> 
> What does it mean to "check" an event?

Maybe dispatchMessageEventIfSameOriginAsIntended?  Maybe rename targetOrigin to intendedTargetOrigin?

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1899
> +    // Pass an empty call stack, since we don't have the one from the other process.
> +    RefPtr<ScriptCallStack> stackTrace;
> +    m_frame->domWindow()->checkAndDispatchMessageEvent(targetOrigin.get(), event, stackTrace);

You should be able to just pass 0 rather than having a dummy local variable.

[Alexey Proskuryakov]

> dispatchMessageEventIfSameOriginAsIntended

Yes, either that or dispatchMessageEventWithOriginCheck would seem less confusing.

[Charles Reis]

Created attachment 140620 [details]
Patch

[Charles Reis]

>>> Source/WebCore/page/DOMWindow.h:238
>>> +        void checkAndDispatchMessageEvent(SecurityOrigin* targetOrigin, PassRefPtr<Event>, PassRefPtr<ScriptCallStack>);
>> 
>> What does it mean to "check" an event?
> 
> Maybe dispatchMessageEventIfSameOriginAsIntended?  Maybe rename targetOrigin to intendedTargetOrigin?

Yes, I like Alexey's dispatchMessageEventWithOriginCheck plus the intendedTargetOrigin parameter.  I think those two make it more self explanatory.  Updated.

>> Source/WebKit/chromium/src/WebFrameImpl.cpp:1899
>> +    m_frame->domWindow()->checkAndDispatchMessageEvent(targetOrigin.get(), event, stackTrace);
> 
> You should be able to just pass 0 rather than having a dummy local variable.

Done.

[Adam Barth]

Comment on attachment 140620 [details]
Patch

Can we unit test this change?

[Charles Reis]

Created attachment 141037 [details]
Patch

[Charles Reis]

(In reply to comment #8)
> (From update of attachment 140620 [details])
> Can we unit test this change?

It was a little tricky, but yes.  I fixed another test while I was at it and added comments to help other test writers.

I also discovered a bug in willCheckAndDispatchMessageEvent (which was using the wrong source frame pointer).  I tried writing a unit test for that but it's called asynchronously, and I didn't see an easy way to wait for the result.

Please take another look!

Thanks,
Charlie

[Adam Barth]

Comment on attachment 141037 [details]
Patch

Thanks Charlie.

[WebKit Review Bot]

Comment on attachment 141037 [details]
Patch

Attachment 141037 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12645884

New failing tests:
http/tests/appcache/deferred-events-delete-while-raising-timer.html
fast/dom/Window/Location/set-location-after-close.html
http/tests/security/xss-DENIED-synchronous-form.html
http/tests/appcache/deferred-events-delete-while-raising.html

[WebKit Review Bot]

Created attachment 141055 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Charles Reis]

Created attachment 141064 [details]
Patch

[Charles Reis]

(In reply to comment #12)
> (From update of attachment 141037 [details])
> Attachment 141037 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12645884
> 
> New failing tests:
> http/tests/appcache/deferred-events-delete-while-raising-timer.html
> fast/dom/Window/Location/set-location-after-close.html
> http/tests/security/xss-DENIED-synchronous-form.html
> http/tests/appcache/deferred-events-delete-while-raising.html

Fixed by adding a null check to FrameLoaderClientImpl::willCheckAndDispatchMessageEvent.

Also, I removed my change to the ChromePageNoJavascript test at Tom's request, filing bug 86046 for him instead (since he had some other changes to land in that test).

[WebKit Review Bot]

Comment on attachment 141064 [details]
Patch

Rejecting attachment 141064 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Charles Reis]

(In reply to comment #16)
> (From update of attachment 141064 [details])
> Rejecting attachment 141064 [details] from commit-queue.
> 
> [e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.
> 
> - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
> 
> - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

Oops, I haven't updated that file yet since becoming a committer.  I'll likely get to it tomorrow.

[WebKit Review Bot]

Comment on attachment 141064 [details]
Patch

Clearing flags on attachment: 141064

Committed r116608: <http://trac.webkit.org/changeset/116608>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.