Bug 73883 – Give embedders a chance to handle postMessage calls

• Component: UI Events
• Status: RESOLVED FIXED

[Karl Koscher]

To support cross-process postMessage calls in Chromium (bug 73337), we need to intercept postMessage calls to proxy windows. Originally we were just going to add a native event listener on the Chromium side, but that required more changes to WebKit and was a bit of a hack. See bug 73359 for a discuss about moving to this approach.

[Karl Koscher]

Created attachment 117975 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Adam Barth]

Comment on attachment 117975 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

It would be helpful to explain in the ChangeLog why we're making this change.  Also, you'll need to say something about testing here.  Perhaps we should add something to DumpRenderTree to test this change?  Alternatively, we can use an API test, which might be easier.

> Source/WebCore/page/DOMWindow.cpp:916
> +      return;

Four-space indent, pls.

> Source/WebKit/chromium/public/WebFrameClient.h:395
> +    virtual bool interceptPostMessage(int64 frameId,

Why the frameId?  Most of these callbacks seem to use a WebFrame* for context.

[Adam Barth]

(In reply to comment #0)
> To support cross-process postMessage calls in Chromium (bug 73337), we need to intercept postMessage calls to proxy windows. Originally we were just going to add a native event listener on the Chromium side, but that required more changes to WebKit and was a bit of a hack. See bug 73359 for a discuss about moving to this approach.

^^^ This is great text for the ChangeLog.

[WebKit Review Bot]

Comment on attachment 117975 [details]
Patch

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

New failing tests:
fast/dom/Window/window-postmessage-args.html

[Alexey Proskuryakov]

Comment on attachment 117975 [details]
Patch

Why FrameLoaderClient? You are not loading anything here.

[Adam Barth]

> Why FrameLoaderClient? You are not loading anything here.

Which client would you prefer?  FrameLoaderClient contains a number of functions not directly related to loading.  Maybe we should split those off into a new client?

[Karl Koscher]

Created attachment 118156 [details]
Patch

[Karl Koscher]

(In reply to comment #3)
> (From update of attachment 117975 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117975&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> It would be helpful to explain in the ChangeLog why we're making this change.  Also, you'll need to say something about testing here.  Perhaps we should add something to DumpRenderTree to test this change?  Alternatively, we can use an API test, which might be easier.

ChangeLog updated, and a layout test added.

 
> > Source/WebCore/page/DOMWindow.cpp:916
> > +      return;
> 
> Four-space indent, pls.

Fixed.

 
> > Source/WebKit/chromium/public/WebFrameClient.h:395
> > +    virtual bool interceptPostMessage(int64 frameId,
> 
> Why the frameId?  Most of these callbacks seem to use a WebFrame* for context.

Updated to pass a WebFrame*

As for using FrameLoaderClient, it's the only way to get to WebFrameClient, which seems like the most appropriate place for the callback.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118156 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:913
> +    RefPtr<MessageEvent> event = timer->event(document());

I'm curious why it is better to add interception code here instead of just adding
an event handler on the window.

> Source/WebKit/chromium/public/WebFrameClient.h:397
> +                                      WebDOMEvent) { return false; }

why not pass WebDOMMessageEvent here?

[WebKit Review Bot]

Comment on attachment 118156 [details]
Patch

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

New failing tests:
http/tests/security/mixedContent/data-url-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-script-in-main-frame-blocked.html
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
http/tests/security/mixedContent/redirect-http-to-https-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-allowed.html
platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-blocked.html
http/tests/security/mixedContent/about-blank-iframe-in-main-frame.html
http/tests/security/mixedContent/insecure-image-in-main-frame.html
http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-script-in-main-frame-allowed.html

[Karl Koscher]

Created attachment 118618 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 118618 [details]
Patch

Attachment 118618 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10834273

[Adam Barth]

Comment on attachment 118618 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:289
> +    // Returns true if the embedder intercepted the postMessage call
> +    bool interceptPostMessage(SecurityOrigin* target, PassRefPtr<MessageEvent>) const;

We should not need to edit FrameLoader in this patch.  The FrameLoaderClient is public to avoid needing these trampolines.

[Karl Koscher]

Created attachment 118643 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 118643 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:919
> +    if (m_frame->loader()->client()->interceptPostMessage(timer->targetOrigin(), event))

nit: I would prefer a more specific name that conveys what is going on at the
point of this notification.  interceptPostMessage sounds like it might be called
when postMessage is called, but actually it is called before dispatchEvent.
Perhaps a slightly better name for this would be willDispatchMessageEvent.

However, I have a slight problem with that name since it is possible that the
security check will prevent dispatchEvent from being called.  This brings me
back to my previous concern.  I would prefer if we put this notification
after the security check.  This probably means that we'd need to ensure that
the SecurityOrigin of the Document is setup right, but perhaps that would
actually be a good thing for keeping the design of out-of-process iframes
simpler and easier to maintain.

[Karl Koscher]

(In reply to comment #16)
> nit: I would prefer a more specific name that conveys what is going on at the
> point of this notification.  interceptPostMessage sounds like it might be called
> when postMessage is called, but actually it is called before dispatchEvent.
> Perhaps a slightly better name for this would be willDispatchMessageEvent.

It is essentially called when postMessage is called. I could move it into DOMWindow::postMessage, although the comments there suggest that most of the processing should happen in DOMWindow::postMessageTimerFired. 

> However, I have a slight problem with that name since it is possible that the
> security check will prevent dispatchEvent from being called.  This brings me
> back to my previous concern.  I would prefer if we put this notification
> after the security check.  This probably means that we'd need to ensure that
> the SecurityOrigin of the Document is setup right, but perhaps that would
> actually be a good thing for keeping the design of out-of-process iframes
> simpler and easier to maintain.

We have to do the same security check in the target renderer process to avoid a TOCTTOU security bug where the target origin could change while the message event is in flight. I'm pretty sure we'll need to do the same thing with out-of-process iframes, since the iframe's origin can change while an IPC is in flight. Keeping the proxy's SecurityOrigin in sync would add a fair amount of complexity that really wouldn't let us simplify anything.

[Darin Fisher (:fishd, Google)]

(In reply to comment #17)
> (In reply to comment #16)
> > nit: I would prefer a more specific name that conveys what is going on at the
> > point of this notification.  interceptPostMessage sounds like it might be called
> > when postMessage is called, but actually it is called before dispatchEvent.
> > Perhaps a slightly better name for this would be willDispatchMessageEvent.
> 
> It is essentially called when postMessage is called. I could move it into DOMWindow::postMessage, although the comments there suggest that most of the processing should happen in DOMWindow::postMessageTimerFired. 
> 
> > However, I have a slight problem with that name since it is possible that the
> > security check will prevent dispatchEvent from being called.  This brings me
> > back to my previous concern.  I would prefer if we put this notification
> > after the security check.  This probably means that we'd need to ensure that
> > the SecurityOrigin of the Document is setup right, but perhaps that would
> > actually be a good thing for keeping the design of out-of-process iframes
> > simpler and easier to maintain.
> 
> We have to do the same security check in the target renderer process to avoid a TOCTTOU security bug where the target origin could change while the message event is in flight. I'm pretty sure we'll need to do the same thing with out-of-process iframes, since the iframe's origin can change while an IPC is in flight.

So, your argument is that we should skip the check locally because it will be wasted work?  That makes sense.


> Keeping the proxy's SecurityOrigin in sync would add a fair amount of complexity that really wouldn't let us simplify anything.

It would simply the notification that you are proposing adding.  My concern is not just about whether your hook makes sense for this application, but I'm concerned about whether it will make sense to future applications.  Someone else may want to hook postMessage, so we should make sure that it makes sense on its own.  For this reason, getting the name right is worth the effort to avoid future confusion.

When you forward the MessageEvent, do you just repeat the postMessage in the target process?  I assume that the existing security checks run in the target process, so we don't have to duplicate any security checking code, right?

[Karl Koscher]

(In reply to comment #18)
> > Keeping the proxy's SecurityOrigin in sync would add a fair amount of complexity that really wouldn't let us simplify anything.
> 
> It would simply the notification that you are proposing adding.  My concern is not just about whether your hook makes sense for this application, but I'm concerned about whether it will make sense to future applications.  Someone else may want to hook postMessage, so we should make sure that it makes sense on its own.  For this reason, getting the name right is worth the effort to avoid future confusion.

Two ideas come to mind. One is to name it something like interceptInsecurePostMessage / interceptUncheckedPostMessage, and the other is to explicitly pass the result of WebKit's origin check.

If it still seems too risky, I can look into what would be involved in keeping the proxies' SecurityOrigins in sync. 

> When you forward the MessageEvent, do you just repeat the postMessage in the target process?  I assume that the existing security checks run in the target process, so we don't have to duplicate any security checking code, right?

This is currently being done on the Chromium side to minimize the changes to WebKit. Bug 73359 would have moved the check into SecurityOrigin so it could be shared by Chromium and WebKit. Another alternative is to add another method to DOMWindow to let external postMessages be injected.

[Eric Seidel (no email)]

Comment on attachment 118643 [details]
Patch

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

> Source/WebCore/dom/MessageEvent.h:31
> +#include "fileapi/Blob.h"

This isn't standard webkit style.

> Source/WebKit/chromium/public/WebDOMMessageEvent.h:38
> +#include "dom/Event.h"
> +#include "dom/MessageEvent.h"

These are also not standard WebKit style.

[Charles Reis]

(In reply to comment #19)
> (In reply to comment #18)
> Two ideas come to mind. One is to name it something like interceptInsecurePostMessage / interceptUncheckedPostMessage, and the other is to explicitly pass the result of WebKit's origin check.
> 
> If it still seems too risky, I can look into what would be involved in keeping the proxies' SecurityOrigins in sync. 

I don't think we want to try to keep the SecurityOrigins in sync across processes.  Finding an appropriate name to convey that the security check still needs to happen seems like the right thing to me.  It allows the embedder to handle a postMessage event externally, whether that's in another WebKit process (like our intended use case) or some other entity in a different application.

I'm not super-familiar with the WebKit API naming scheme, but something close to Darin's suggestion would be willCheckAndDispatchMessageEvent.

Darin, does that sound right to you?

Karl, can you fix the includes that Eric pointed out?

[Eric Seidel (no email)]

The various platforms all have differnet ways of adding directories to their include path.  Windows requires editing the vcproj file (which you can do in your text editor).  I would suggest searching for existing include paths for examples.

[Karl Koscher]

Created attachment 120247 [details]
Patch

[WebKit Review Bot]

Attachment 120247 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Inform the scrolling coordinator when scrollbar layers come and go
Using index info to reconstruct a base tree...
<stdin>:474806: trailing whitespace.
        [Chromium] DatabaseTrackerChromium: iterating DatabaseSet races with Database disposal on worker thread 
<stdin>:474827: trailing whitespace.
        Nothing to test, just removing redundant code. Correct behavior tested by 
<stdin>:475346: trailing whitespace.
    
warning: 3 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 167528 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
CONFLICT (content): Merge conflict in LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/page/ScrollingCoordinator.h
CONFLICT (content): Merge conflict in Source/WebCore/page/ScrollingCoordinator.h
Auto-merging Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
CONFLICT (content): Merge conflict in Source/WebCore/page/mac/ScrollingCoordinatorMac.mm
Auto-merging Source/WebCore/platform/ScrollView.cpp
Auto-merging Source/WebCore/rendering/RenderLayerCompositor.cpp
CONFLICT (content): Merge conflict in Source/WebCore/rendering/RenderLayerCompositor.cpp
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Auto-merging Tools/Scripts/build-webkit
Auto-merging Tools/Scripts/webkitdirs.pm
CONFLICT (content): Merge conflict in Tools/Scripts/webkitdirs.pm
Failed to merge in the changes.
Patch failed at 0001 Inform the scrolling coordinator when scrollbar layers come and go

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Karl Koscher]

(In reply to comment #22)
> The various platforms all have differnet ways of adding directories to their include path.  Windows requires editing the vcproj file (which you can do in your text editor).  I would suggest searching for existing include paths for examples.

Eric, can you take a look at why it's failing the style check now? The output doesn't seem to make sense, and I'm not sure how to run it through the EWS again without uploading the same patch again.

[WebKit Review Bot]

Comment on attachment 120247 [details]
Patch

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

New failing tests:
http/tests/security/mixedContent/data-url-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-script-in-main-frame-blocked.html
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
http/tests/security/mixedContent/redirect-http-to-https-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-allowed.html
http/tests/inspector-enabled/dedicated-workers-list.html
http/tests/inspector/network-preflight-options.html
http/tests/security/mixedContent/insecure-css-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-allowed.html
platform/chromium/http/tests/security/mixedContent/insecure-image-in-main-frame-blocked.html
http/tests/security/mixedContent/about-blank-iframe-in-main-frame.html
http/tests/security/mixedContent/insecure-image-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-blocked.html
http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
platform/chromium/http/tests/security/mixedContent/insecure-script-in-main-frame-allowed.html

[Adam Barth]

Comment on attachment 120247 [details]
Patch

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

> Source/WebKit/efl/CMakeListsEfl.txt:13
> +    "${WEBCORE_DIR}/fileapi"

I don't understand why this is needed.

[Adam Barth]

> Eric, can you take a look at why it's failing the style check now?

Sorry, the style bot was borked for a few hours.  Should be fixed now.

[Adam Barth]

This patch is in a confusing state.  Do we really need to add fileapi to the include path?  I don't understand what in this patch is requiring us to do that.

Also, you appear to have broken a number of tests that use postMessage.  Are those breakages real?  Also, your patch doesn't seem to compile on AppleWindows.

[Adam Barth]

Comment on attachment 120247 [details]
Patch

Clearing the review flag.  I think the content of this patch is great, but we'll probably need a tweaked version that builds and passes tests.

[Karl Koscher]

(In reply to comment #29)
> This patch is in a confusing state.  Do we really need to add fileapi to the include path?  I don't understand what in this patch is requiring us to do that.

Including MessageEvent.h in FrameLoaderClient.h (so that we pass around a DOMMessageEvent, not just a DOMEvent) causes the build to break under EFL and Windows (and maybe Mac) because it can't locate fileapi/Blob.h. An earlier patch fixed this with #include "fileapi/Blob.h", but this isn't correct WebKit style.

Would a more correct fix be to move Blob.h?

> Also, you appear to have broken a number of tests that use postMessage.  Are those breakages real?  Also, your patch doesn't seem to compile on AppleWindows.

AFAICT, they are only sort of real. If the TestShell's shouldDumpFrameLoadCallbacks is set, we now print out that willCheckAndDispatchMessageEvent is called since it's a frame loader callback (and it's used by the test for this patch). Should I just re-baseline these tests where this is the case?

Finally, is there any way to test the build on Mac without having a Mac?

[Adam Barth]

(In reply to comment #31)
> (In reply to comment #29)
> > This patch is in a confusing state.  Do we really need to add fileapi to the include path?  I don't understand what in this patch is requiring us to do that.
> 
> Including MessageEvent.h in FrameLoaderClient.h (so that we pass around a DOMMessageEvent, not just a DOMEvent) causes the build to break under EFL and Windows (and maybe Mac) because it can't locate fileapi/Blob.h. An earlier patch fixed this with #include "fileapi/Blob.h", but this isn't correct WebKit style.
> 
> Would a more correct fix be to move Blob.h?

I see.  We're adding the include path to the WebKit layer and it already exists at the WebCore layer.  Ok.  That makes sense.

> AFAICT, they are only sort of real. If the TestShell's shouldDumpFrameLoadCallbacks is set, we now print out that willCheckAndDispatchMessageEvent is called since it's a frame loader callback (and it's used by the test for this patch). Should I just re-baseline these tests where this is the case?

It sounds like you should.  Generally, we prefer the tests to all be green when a patch lands.  Sometimes that involves changing the expected results for tests, which we review at the same time we review the change that causes the change in expectations.

> Finally, is there any way to test the build on Mac without having a Mac?

Unfortunately not.  This is a consequence of Apple not licensing Mac OS X for use inside a virtual machine.  Without the ability to run Mac OS X inside a virtual machine, we are unable to run code (including the build scripts) from non-committers.

The WebKit project has a strong Mac bias.  The vast majority of the core developers use Macs and it's generally difficult to contributor to the project without a Mac.  If you're planning to make more changes to WebKit, I would encourage you to get a Mac.

[Karl Koscher]

Created attachment 120380 [details]
Patch

[Adam Barth]

Comment on attachment 120380 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:325
> +        // Returns true if the embedder intercepted the postMessage call
> +        virtual bool willCheckAndDispatchMessageEvent(SecurityOrigin* /*target*/, PassRefPtr<MessageEvent>) const { return false; }

Why PassRefPtr?  Is the client expected to take ownership of the MessageEvent?  From the caller, it looks like not, so this should probably just be a MessageEvent*

> Source/WebKit/chromium/public/WebDOMMessageEvent.h:55
> +    WebDOMMessageEvent(const WTF::PassRefPtr<WebCore::MessageEvent>& e) : WebDOMEvent(e) { }

explicit

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:1212
> +        fputs(" - willCheckAndDispatchMessageEvent\n", stdout);

You've only made this change in the Chromium version of DumpRenderTree, but you've updated a bunch of cross-platform test results to contain this string.  Doesn't that mean those tests will fail in non-Chromium ports?  I would skip this print statement.  It looks like more trouble than it's worth.

> LayoutTests/platform/chromium/fast/events/intercept-postmessage.html:15
> +          layoutTestController.interceptPostMessage = true;

We probably need a test that proves that we allow intercepting before the security check because some review comments were worried about messing that up in the future.

[Karl Koscher]

(In reply to comment #34)
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:1212
> > +        fputs(" - willCheckAndDispatchMessageEvent\n", stdout);
> 
> You've only made this change in the Chromium version of DumpRenderTree, but you've updated a bunch of cross-platform test results to contain this string.  Doesn't that mean those tests will fail in non-Chromium ports?  I would skip this print statement.  It looks like more trouble than it's worth.

Err... right. I'll remove it.

[Karl Koscher]

Created attachment 120411 [details]
Patch

[Adam Barth]

Comment on attachment 120411 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:919
> +    if (m_frame->loader()->client()->willCheckAndDispatchMessageEvent(timer->targetOrigin(), PassRefPtr<MessageEvent>(event).leakRef()))

Can m_frame be null here?  Perhaps we should check isCurrentlyDisplayedInFrame() ?

[Adam Barth]

This looks great.  I think we're one null check away from landing this patch.

[Karl Koscher]

Created attachment 120418 [details]
Patch

[WebKit Review Bot]

Comment on attachment 120418 [details]
Patch

Clearing flags on attachment: 120418

Committed r103620: <http://trac.webkit.org/changeset/103620>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

Rolled out because of crashes.

[Charles Reis]

Created attachment 121024 [details]
Patch

[Charles Reis]

I've added a null check to Karl's patch in FrameLoaderClientImpl::willCheckAndDispatchMessageEvent, which should prevent the crashes we saw.  All the accesses to m_webFrame->client() in that file are similarly guarded, so it seems consistent to me.  Can we try again?

[Adam Barth]

Comment on attachment 121024 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 121024 [details]
Patch

Clearing flags on attachment: 121024

Committed r104005: <http://trac.webkit.org/changeset/104005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[John Knottenbelt]

Comment on attachment 121024 [details]
Patch

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

> LayoutTests/platform/chromium/fast/events/intercept-postmessage.html:40
> +        window.layoutTestController.dumpFrameLoadCallbacks();

This test is producing two outputs: See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=platform%2Fchromium%2Ffast%2Fevents%2Fintercept-postmessage.html depending on whether the intercepted messages occur before or after the frame loading messages. 

Are the frame loading messages necessary? If not, it sounds like a simple fix would be to remove laoutTestController.dumpFrameLoadCallbacks()

[Charles Reis]

(In reply to comment #48)
> (From update of attachment 121024 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121024&action=review
> 
> > LayoutTests/platform/chromium/fast/events/intercept-postmessage.html:40
> > +        window.layoutTestController.dumpFrameLoadCallbacks();
> 
> This test is producing two outputs: See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=platform%2Fchromium%2Ffast%2Fevents%2Fintercept-postmessage.html depending on whether the intercepted messages occur before or after the frame loading messages. 
> 
> Are the frame loading messages necessary? If not, it sounds like a simple fix would be to remove laoutTestController.dumpFrameLoadCallbacks()

No, I think it should be safe to remove them.  We just care that the "intercepted postMessage" lines show up.  Thanks for the suggestion.

I can get to it later this morning if that's soon enough.

[Charles Reis]

(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 121024 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=121024&action=review
> > 
> > > LayoutTests/platform/chromium/fast/events/intercept-postmessage.html:40
> > > +        window.layoutTestController.dumpFrameLoadCallbacks();
> > 
> > This test is producing two outputs: See http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=platform%2Fchromium%2Ffast%2Fevents%2Fintercept-postmessage.html depending on whether the intercepted messages occur before or after the frame loading messages. 
> > 
> > Are the frame loading messages necessary? If not, it sounds like a simple fix would be to remove laoutTestController.dumpFrameLoadCallbacks()
> 
> No, I think it should be safe to remove them.  We just care that the "intercepted postMessage" lines show up.  Thanks for the suggestion.
> 
> I can get to it later this morning if that's soon enough.

I've posted a fix for review to https://bugs.webkit.org/show_bug.cgi?id=75718.