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.
Created attachment 140568 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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?
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.
> dispatchMessageEventIfSameOriginAsIntended Yes, either that or dispatchMessageEventWithOriginCheck would seem less confusing.
Created attachment 140620 [details] Patch
>>> 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.
Comment on attachment 140620 [details] Patch Can we unit test this change?
Created attachment 141037 [details] Patch
(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
Comment on attachment 141037 [details] Patch Thanks Charlie.
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
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
Created attachment 141064 [details] Patch
(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).
Comment on attachment 141064 [details] Patch Rejecting attachment 141064 [details] from commit-queue. creis@chromium.org 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.
(In reply to comment #16) > (From update of attachment 141064 [details]) > Rejecting attachment 141064 [details] from commit-queue. > > creis@chromium.org 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.
Comment on attachment 141064 [details] Patch Clearing flags on attachment: 141064 Committed r116608: <http://trac.webkit.org/changeset/116608>
All reviewed patches have been landed. Closing bug.