Related:
https://github.com/whatwg/html/issues/3506
https://bugzilla.mozilla.org/show_bug.cgi?id=1440754

Using a timer to defer the message might help but this sounds like a bigger problem because we haven't really thought about maintaining ordering of things like this across the entire pipeline. In particular, Resize and PostMessage are sent to the OOPIF renderer over different IPC channels.

Comment 2: I'm not sure that's true, unless you're talking about non-associated Mojo interfaces.  With traditional IPC, they should be well-ordered.

That's probably okay then. Adding tests will be good to guard against, for instance, a PostTask gets inserted anywhere in the processing of a size change notification. Are there any other properties that need to be synchronized ahead of a PostMessage?

#4: yes, we've discussed this over lunch, and some other examples we thought of were visibility, focus, and fullscreen.  There are probably others.

A few updates:

1. Adding a timer/PostTask to the cross-process case is a bit tricky because we might need to make sure that the message will still be delivered if the source document or even source process goes away before the timer has fired.  The same-process case adds a timer on the destination Document.  To match that behavior we were thinking of accumulating postMessages on the RenderFrameProxyHost and sending them out when either (1) the source sends a second IPC triggered when the event loop iteration that scheduled the postMessage finishes running, or (2) when the source process dies (and the RFPH is destroyed).

2. I did a simple experiment to delay RemoteDOMWindow::SchedulePostMessage by doing a PostTask on the source document, to see if that helps, using something like
  source->GetTaskRunner(TaskType::kPostedMessage)->PostTask(...)

This does solve this problem for window.focus(), using a repro along the lines of the mozilla bug referenced in #1.  Interestingly, it does *not* fix the size-changing repro above.  What seems to happen is in the same-process case, the frame rects are changed when the subframe looks up window.innerWidth:

#1 0x7fcaa6c1db57 blink::LocalFrameView::FrameRectsChanged()
#2 0x7fcaa6c1d8f0 blink::LocalFrameView::SetFrameRect()
#3 0x7fcaa6feac3e blink::LayoutEmbeddedContent::UpdateGeometry()
#4 0x7fcaa6c24c08 blink::LocalFrameView::UpdateGeometry()
#5 0x7fcaa6c288f3 blink::LocalFrameView::UpdateGeometriesIfNeeded()
#6 0x7fcaa6c2270e blink::LocalFrameView::PerformPostLayoutTasks()
#7 0x7fcaa6c2386f blink::LocalFrameView::ScheduleOrPerformPostLayoutTasks()
#8 0x7fcaa6c20244 blink::LocalFrameView::UpdateLayout()
#9 0x7fcaa6927104 blink::Document::UpdateStyleAndLayout()
#10 0x7fcaa6926f4a blink::Document::UpdateStyleAndLayoutIgnorePendingStylesheets()
#11 0x7fcaa6c06562 blink::LocalDOMWindow::GetViewportSize()
#12 0x7fcaa6c0667c blink::LocalDOMWindow::innerWidth()
#13 0x7fcaa76747e6 blink::V8Window::innerWidthAttributeGetterCallback()
#14 0x7fcaa7d19459 v8::internal::FunctionCallbackArguments::Call()
#15 0x7fcaa7e09b2d v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#16 0x7fcaa7e083a7 v8::internal::Builtins::InvokeApiFunction()
#17 0x7fcaa839fd50 v8::internal::Object::GetPropertyWithAccessor()
#18 0x7fcaa82b4eb7 v8::internal::LoadIC::Load()

However, with OOPIFs, the frame rects are changed asynchronously via

#1 0x7f3d29fe99f3 content::RenderFrameProxy::WasResized()
#2 0x7f3d24ab529c blink::RemoteFrameClientImpl::FrameRectsChanged()
#3 0x7f3d24ab6248 blink::RemoteFrameView::FrameRectsChanged()
#4 0x7f3d24e47c3e blink::LayoutEmbeddedContent::UpdateGeometry()
#5 0x7f3d24a858f3 blink::LocalFrameView::UpdateGeometriesIfNeeded()
#6 0x7f3d24a7f70e blink::LocalFrameView::PerformPostLayoutTasks()
#7 0x7f3d24a8086f blink::LocalFrameView::ScheduleOrPerformPostLayoutTasks()
#8 0x7f3d24a7d244 blink::LocalFrameView::UpdateLayout()
#9 0x7f3d24784104 blink::Document::UpdateStyleAndLayout()
#10 0x7f3d24783f4a blink::Document::UpdateStyleAndLayoutIgnorePendingStylesheets()
#11 0x7f3d2494d056 blink::(anonymous namespace)::RootEditableElementOfSelection()
#12 0x7f3d2494d4e8 blink::InputMethodController::TextInputType()
#13 0x7f3d2a01e6b3 content::RenderWidget::GetTextInputType()
#14 0x7f3d2a01b0b3 content::RenderWidget::UpdateTextInputStateInternal()
#15 0x7f3d2a01a39e content::RenderWidget::WillBeginCompositorFrame()
#16 0x7f3d28786387 cc::ProxyMain::BeginMainFrame()

And this resizing happens after the PostTask to forward the cross-process postMessage has fired, so we still end up forwarding the postMessage before forwarding the new size.

Ken, any thoughts on this?  Is there anything we could/should do to force layout sooner, when something is going to change the size for a remote frame?

I also checked and the PostTask experiment doesn't help with issue 826457.  Couldn't easily check 822958 due to https://crbug.com/822958#c2.

Comment 8: That's interesting.  I wonder why the subframe code is running before the postMessage task fires, and whether/why that's deterministic.  I'm also curious about the layout question for Ken.  (+chrishtr@ just in case.)

For repro'ing issue 822958, maybe try comment 22:
> I can now reliably repro this bug in M67 with Site Isolation if I change chrome://flags#autoplay-policy to "No user gesture is required."

Comment 8: What would make the most sense to me is to force a layout on the source document in RemoteDOMWindow::SchedulePostMessage(). That would flush any layout changes that need to happen before the message is dispatched.

Would forcing a layout on postMessage to remote frame cause unnecessary layouts? I thought layout is an expensive operation, so if the postMessage is not related to sizing, then we are spending CPU cycles on data that is unused.

Layout only happens if dirty bits are set on layout objects, indicating that it is needed. In that case it is just moving up something that was going to happen very soon after anyway.

I guess to clarify, by "force" layout I mean just call one of the UpdateLayout methods on the Document.

Forced layouts might cause unnecessary layouts, because the forced layout may
be succeeded by another layout-dirtying mutation before the next frame. Therefore
without the forced layout, we would have done one layout, whereas now it would be
two.

That's true. One alternative I can think of then is to delay the PostMessage dispatch until after the next BeginFrame.

I think RenderWidget::QueueMessage() could be used to do that.

A few interesting findings:

I tried out a quick version of this proposed fix using PostTask.  As Alex noted, it does not fix the repro steps in this bug without doing something to trigger FrameRectsChanged.  However, it *does* fix the video ad in issue 822958!  (To get that to play, I needed both the autoplay flag mentioned in comment 9 and a build config change for the video codecs.  Just add 'proprietary_codecs = true ffmpeg_branding = "Chrome"' to gn args.)

From Ken's suggestion in comment 13, I found that Document::UpdateStyleAndLayout() seems to be the method to trigger the right RemoteFrameClientImpl::FrameRectsChanged call (if we were to go down that path, though chrishtr@ is right that it might not be a good plan).  That combo does fix the repro steps in this bug, causing the console log to print "Frame width: 800".

Unfortunately neither approach fixes the text fitting problem in issue 826457.  We should try to get a minimal repro for that bug to understand what specific race or ordering change it's having trouble with.


I think this means we should definitely proceed with some version of this fix to resolve issue 822958 and possibly look for a way to make it cover the resize steps in this bug as well (maybe in a followup CL).  For that, I'm curious if there's a way to trigger FrameRectsChanged for the remote frame at the same point that we hit it in the same-process case, since that seems to be the key difference.

Alex, will you be able to continue this on Thursday?  Does tackling the non-resize case first make sense?

> For that, I'm curious if there's a way to trigger FrameRectsChanged for the
> remote frame at the same point that we hit it in the same-process case

In the same-process case, a layout is triggered when the child frame checks its size after it receives the PostMessage. It has to be layout clean for that size to be accurate. That's not feasible now that the child frame is out of process.

Updating layout before we send the PostMessage as I suggested in #10 is similar, forcing the frame to be layout clean at the right time. But as noted in #14, this advances the point where we do layout (in the same process case it is done upon the child frame receiving the PostMessage, whereas this would mean layout when the parent frame sends it), and in some cases that would increase the number of layouts needed.

The alternative to doing layout would be to delay the PostMessage until after the next layout. Queueing the PostMessage IPC using RenderWidget::QueueMessage() and the MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP policy might work, but I don't know if there might be drawbacks to tying this to the compositor. Alternatively we could build our own queue and try to flush it from LocalFrameView::PerformPostLayoutTasks().

Thanks for the analysis and discussion so far!  

#16: Yes, I'll continue poking at this today.  Thanks for verifying that this resolves issue 822958!  Handling the non-resize case first makes sense.  I also want to verify that buffering in the browser process is necessary (i.e., find a repro where we'd lose the message due to the source process going away).

#17: With something like RenderWidget::QueueMessage, is there a risk that we'll delay the message too much, for example if nothing actually required layout updates after the postMessage call in the same event loop iteration, will this still send the message or wait for something else (potentially much later) to update layout?

#18: The comment on RenderWidget::QueueMessage says if there is no need to generate a new compositor frame, then the message will be sent immediately via normal IPC. That seems like the right behavior to me.

Re: browser-side queueing, Nasko and I reasoned through a few cases, and we think that browser-side queueing is not necessary, and that a renderer-side PostTask won't make things any worse than they already are today.  

Basically, postMessages sent from an unload handler to another frame are already broken with --site-per-process, if the sender frame is the last active frame in the process and the process is going away.  Consider this example: A(B(C)), B navigates to D, and C sends a postMessage to A in its unload handler.  During RFHM::CommitPending for B->D, we call ResetForNewProcess, which destroys the subtree rooted at B, triggering ~RFHI for C.  That runs through the following steps:
1. Unregister the B RFHI routing ID from the B process.
2. DecrementActiveFrameCount() on the RFHI's SiteInstance (i.e., B).
3. Send FrameMsg_Delete to C, which triggers the unload handler on the renderer side.

I think we already know that the subframe handler that runs in step 3 is already kind of busted, because it can't communicate back to its RFH, which has already been destroyed: see issues 606975, 609963, and 763549.  However, when this is the last active frame in the process, DecrementActiveFrameCount() in step 2 ends up also destroying any other proxies in that process, *before* we trigger unload in step 3.  So the unload handler can't really postMessage *any other* frame.  I've verified that this whole scenario is already broken with --site-per-process (the postMessage from C to A is sent from RFP and then dropped on the floor due to RFPH being gone), and that it works fine without --site-per-process.  This bug might turn out to be important to fix (e.g., with issue 609963) for shipping site isolation, but doing a renderer-side PostTask rather than browser-side queueing for this issue won't make it any worse.

We've tried thinking about any other ways where the process might go away, and the postMessage is not running in an unload handler, i.e.:

  cross_site_frame.postMessage("","*"); // this will now do a PostTask
  <do something to shut down current process, before the postMessage task can execute>;

Things like "another_frame.location = ..." shouldn't be a problem because navigations are scheduled.  Reaching into an ancestor frame to synchronously detach self implies that the process isn't going away (ancestor has to be same-origin and will stay).  If postMessage races with a navigation initiated in the browser or another process (e.g., such that OnSwapOut gets scheduled in between postMessage being called and the PostTask to actually send it), I don't think there are any synchronization/delivery guarantees we need to provide.  Anything else that I might be missing?

A couple of other interesting cases suggested by creis@ and lukasza@:

- postMessage() followed by window.close() is not a problem, as window.close() is also asynchronous and uses a PostTask to let the current script finish running.  See this comment in RenderWidget::CloseWidgetSoon(), which is eventually called from DOMWindow::close(): https://cs.chromium.org/chromium/src/content/renderer/render_widget.cc?l=1650&rcl=5385338f81319f72a20351beffb566c8b385ffdf

- cases where the sender process crashes right after postMessage.  (e.g., due to a bug or chrome://crash.)  We wouldn't have delivered the message anyway in the same-process case, so I don't think there's anything further we should do here.

I ran a CL that implements a renderer-side PostTask through tryjobs, and there are some failures and (worse) tests that become flaky.

Here's one example that's worth discussing (From external/wpt/content-security-policy/embedded-enforcement/subsumption_algorithm-self.html):

We have A(B), where B is:
  <script>
    window.top.postMessage(...);
  </script>

and A is:
  var i = document.createElement('iframe'); // this is the frame B
  window.onmessage = function (e) { ... };
  i.onload = function () {
    // Delay the check until after the postMessage has a chance to execute.
    setTimeout(t.step_func_done(function () {
      <smth that must be done after postMessage is received>;
    }), 1);
  }

In the same-process case, we used to fire events for A in the following deterministic order:
1. onload for i, setTimeout scheduled
2. onmessage
3. setTimeout fires

In the cross-process case, *before* the scheduling change here, we used to do this:
1. onmessage
2. onload for i, setTimeout scheduled  (onload is forwarded via the DispatchLoad IPC)
3. setTimeout fires
I think this is wrong just like the other races discussed in this bug (as the same-process case shows, onload should come before onmessage), but this still met the test's guarantees, because the postMessage would always be received before the setTimeout fired.

In the cross-process case, *after* the scheduling change, we get:
1. onload for i, setTimeout scheduled
2. setTimeout fires
3. onmessage
where steps 2 and 3 are ordered non-deterministically, depending on whether the postMessage IPC from the browser process arrives before or after the timer fires.  In practice, the test starts failing when it checks multiple frames in parallel, which probably slows down some of the postMessage IPCs just enough.

The problem is that in the same-process case, LocalDOMWindow::SchedulePostMessage queues up the postMessage dispatch (with a timer) before we (synchronously) dispatch onload on the FrameOwner, so the onmessage is guaranteed to be dispatched right after the onload, and before the setTimeout fires (as it's all on the same event queue).  In the cross-process case, we no longer have that guarantee.

We could also do a PostTask for sending DispatchLoad to match our previous behavior in this case, but that would still break the expected same-process semantics of onload happening before onmessage.

 Issue 828699  has been merged into this issue.

I guess we could synchronize load event forwarding with postMessage forwarding somehow, though I'm not sure if that'll resolve all such races.  For example, we could check how many postMessages we have scheduled when we fire the load event on the sender side, then on the receiver side, delay dispatching the load event until all those postMessages have been received.  Those postMessages would have to be scheduled again on the receiver side, rather than dispatched immediately, so that they get in the event queue before firing the onload.

Comment 22: That's a tough call.  I agree that preserving the order (onload before onmessage) matters, so our current behavior is wrong even if the test happens to pass that way.

I would like to think that doing a setTimeout(..., 1) to wait for a postMessage to arrive is a bad approach, even if it happened to work before.  It's making assumptions about what has been put in the event queue, which seems like it could be wrong (e.g., maybe based on task scheduling decisions?).  Shouldn't the test code just use onmessage to wait for the message to arrive?

Then again, pages in the wild could expect this behavior, and it's hard to know what will break.  Preserving the behavior would require some kind of complex queueing such that all the IPCs from the sending process get delivered at once to the receiving process, since there could be more than just load and postMessage involved.  (Nick and I were putting together some interesting tests, though I haven't turned them into pages we can try out yet.)

I'm kind of curious how reliable the setTimeout assumption is across browsers.  If it's not always true, then I say we just update the test.

I've got a draft CL @ https://chromium-review.googlesource.com/c/chromium/src/+/999182.

I've tested the cross-browser behavior for these races and it's surprisingly inconsistent.  

I put together this test page: http://alexmos17.github.io/tests/ipc-race.html
This has a cross-site iframe which does a bunch of postMessages to the parent in an inline script.  The iframe's onload handler in the parent schedules a setTimeout for 0ms.  The page just outputs events in the order they are received.

Chrome without --site-per-process:
1. <iframe> onload, which schedules a setTimeout
2. all postMessages are delivered
3. setTimeout fires

Chrome with --site-per-process without the fix:
1. all postMessages are delivered
2. <iframe> onload, which schedules a setTimeout
3. setTimeout fires

Chrome with --site-per-process and the proposed PostTask fix:
1. <iframe> onload, which schedules a setTimeout
2. Some postMessages are delivered
3. setTimeout fires (i.e., it gets scheduled somewhere in between the steady stream of postMessages IPCs)
4. Some more postMessages are delivered

Firefox:
1. all postMessages are delivered
2. <iframe> onload, which schedules a setTimeout
3. setTimeout fires
Note: this actually matches existing Chrome --site-per-process behavior!  I'm guessing that FF is also scheduling the onload event, which makes it fire after all postMessages.  I did check that FF isn't susceptible to issue 822958 though.

Edge:
1. <iframe> onload, which schedules a setTimeout
2. setTimeout fires
3. all postMessages are delivered
This is the weirdest one - it appears that Edge is prioritizing the setTimeout(...,0) over the postMessages?

I think given this experiment, it's reasonable to conclude that the behavior that the CSP tests rely on in #22 can't really be relied on across browsers.  I think this strengthens the argument that the simple PostTask approach should be ok for now, even if it doesn't perfectly match the same-process (Chromium) behavior.

Nice findings!  Good that we seem to have a bit of flexibility here, and I agree that the postTask approach keeps what seems like the important ordering (same-task IPCs before postMessage) while not adding a ton of complexity around the case that's inconsistent between browsers anyway (setTimeout on the receiving side).

We can come back to the layout question later.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b

commit ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Apr 12 18:37:34 2018

Use PostTask to schedule cross-process postMessage forwarding.

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Change-Id: I1138bcdcd18c458a92dec92e145761b226596c7a
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550284}
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed26770582c5a12952a47cb79ea07697dc376237

commit ed26770582c5a12952a47cb79ea07697dc376237
Author: Menglu Huang <huangml@chromium.org>
Date: Thu Apr 12 23:59:39 2018

Revert "Use PostTask to schedule cross-process postMessage forwarding."

This reverts commit ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b.

Reason for revert: <INSERT REASONING HERE>
Breaks browser_tests on linux-chromeos-rel
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6852

Original change's description:
> Use PostTask to schedule cross-process postMessage forwarding.
> 
> Previously, we sent the IPC to forward a cross-process postMessage
> immediately.  This caused a behavioral difference from the
> same-process case where the postMessage is always scheduled.  Namely,
> in a scenario like this:
> 
>   frame.postMessage(...);
>   doSomethingThatSendsIPCsToFrame(frame);
> 
> the IPCs from JS following the postMessage would've been ordered
> incorrectly, causing |frame| to see their side effects after the
> postMessage dispatch in the cross-process case, whereas they would be
> seen before the postMessage dispatch in the same-process case.  One
> example of this is frame.focus(), and another is a frame element
> onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
> a postMessage dispatched from an inline script while the frame was
> still loading.
> 
> To resolve these ordering concerns, this CL changes cross-process
> postMessage to do a PostTask on the sender side before sending the
> message to the browser process.  This improves the current state of
> the world, but does not yet achieve a perfect match for the IPC
> ordering in the same-process case - see discussion on the bug.
> 
> Bug: 828529
> Change-Id: I1138bcdcd18c458a92dec92e145761b226596c7a
> Reviewed-on: https://chromium-review.googlesource.com/999182
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#550284}

TBR=xiyuan@chromium.org,wjmaclean@google.com,dcheng@chromium.org,alexmos@chromium.org

Change-Id: I11e88ed08c096be2534a6ab103f5553f3a4cfb9f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 828529
Reviewed-on: https://chromium-review.googlesource.com/1011564
Reviewed-by: Menglu Huang <huangml@chromium.org>
Commit-Queue: Menglu Huang <huangml@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550421}
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/loader/empty_clients.h

Looks like the revert in #29 was due to two test failures on linux-chromeos-rel, wcich are probably flaky in a similar way to the other test I fixed in webview_login_browsertest.cc.  Pasting the failure details before we lose them:

WebviewProxyAuthLoginTest.ProxyAuthTransfer
../../chrome/browser/chromeos/login/test/js_checker.cc:61: Failure
Value of: GetBool(expression)
  Actual: false
Expected: true
$('signin-frame').src.indexOf('#identifier') != -1

WebviewLoginTest.StoragePartitionHandling
../../chrome/browser/chromeos/login/test/js_checker.cc:61: Failure
Value of: GetBool(expression)
  Actual: false
Expected: true
$('signin-frame').src.indexOf('#identifier') != -1

I'll try to repro this locally, fix, and reland.

OK, managed to repro failures in #30 on a local CrOS build after artificially delaying postMessage dispatch by 100ms with PostDelayedTask.  It's a race similar to what I fixed in the other ChromeOS webview login tests.  Description and reland attempt up at https://chromium-review.googlesource.com/c/chromium/src/+/1011287.  The reland should also fix the Firefox WPT  issue 832319 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9

commit 7c3d1d13f940e88ef6706fd8b5c257a81d340ed9
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Apr 13 15:06:53 2018

Reland: Use PostTask to schedule cross-process postMessage forwarding.

Changes from original attempt at https://crrev.com/c/999182:
- fix flakiness in two additional ChromeOS login tests
- fix CSP WPT tests to not depend on ordering between iframe's onload
  and postMessage - see  https://crbug.com/832319 .

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Change-Id: I62a627c501526d09900be4f5bd2c899acf4d1e07
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550284}
Reviewed-on: https://chromium-review.googlesource.com/1011287
Cr-Commit-Position: refs/heads/master@{#550621}
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4

commit c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4
Author: Ben Pastene <bpastene@chromium.org>
Date: Fri Apr 13 16:49:20 2018

Revert "Reland: Use PostTask to schedule cross-process postMessage forwarding."

This reverts commit 7c3d1d13f940e88ef6706fd8b5c257a81d340ed9.

Reason for revert: WebviewLoginTest.Basic is still flaky on linux-chromeos-rel
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6886
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6887


Original change's description:
> Reland: Use PostTask to schedule cross-process postMessage forwarding.
> 
> Changes from original attempt at https://crrev.com/c/999182:
> - fix flakiness in two additional ChromeOS login tests
> - fix CSP WPT tests to not depend on ordering between iframe's onload
>   and postMessage - see  https://crbug.com/832319 .
> 
> Previously, we sent the IPC to forward a cross-process postMessage
> immediately.  This caused a behavioral difference from the
> same-process case where the postMessage is always scheduled.  Namely,
> in a scenario like this:
> 
>   frame.postMessage(...);
>   doSomethingThatSendsIPCsToFrame(frame);
> 
> the IPCs from JS following the postMessage would've been ordered
> incorrectly, causing |frame| to see their side effects after the
> postMessage dispatch in the cross-process case, whereas they would be
> seen before the postMessage dispatch in the same-process case.  One
> example of this is frame.focus(), and another is a frame element
> onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
> a postMessage dispatched from an inline script while the frame was
> still loading.
> 
> To resolve these ordering concerns, this CL changes cross-process
> postMessage to do a PostTask on the sender side before sending the
> message to the browser process.  This improves the current state of
> the world, but does not yet achieve a perfect match for the IPC
> ordering in the same-process case - see discussion on the bug.
> 
> Bug: 828529
> Change-Id: I62a627c501526d09900be4f5bd2c899acf4d1e07
> Reviewed-on: https://chromium-review.googlesource.com/999182
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#550284}
> Reviewed-on: https://chromium-review.googlesource.com/1011287
> Cr-Commit-Position: refs/heads/master@{#550621}

TBR=xiyuan@chromium.org,dcheng@chromium.org,alexmos@chromium.org

Change-Id: Ic0637a6038bed6e5334a26e1934bee81faad3b9e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 828529
Reviewed-on: https://chromium-review.googlesource.com/1012138
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550649}
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/loader/empty_clients.h

The reland was reverted again due to another source of flakiness in WebviewLoginTest.Basic, which is leading to this failure:

../../chrome/browser/chromeos/login/test/js_checker.cc:61: Failure
Value of: GetBool(expression)
  Actual: false
Expected: true
$('signin-frame').src.indexOf('#identifier') != -1

There's some discussion of this in issue 832365 (filed before the revert).  I've managed to repro locally by artificially delaying postMessage as in #31, though I had to delay it by 1000ms instead of 100ms.  Basically, WaitForGaiaPageLoad() isn't sufficient to guarantee that properties checked in ExpectIdentifierPage() are already updated.  It's also sad that the trybots consistently aren't catching this but the waterfall bots are.

I'm trying out a fix for this in https://chromium-review.googlesource.com/c/chromium/src/+/1012472/1..2.

Also, the M67 branch cut is at r550428.  That means that both the initial attempt (r550284) and its revert (r550421) have made the cut, but the subsequent reland/revert didn't.  So this means that once the reland sticks, it should be sufficient to merge it to M67.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07a131898bfa3810ba928189472f45d4c1784a0e

commit 07a131898bfa3810ba928189472f45d4c1784a0e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Apr 13 23:02:18 2018

Reland 2: Use PostTask to schedule cross-process postMessage forwarding.

Changes from first reland attempt at https://crrev.com/c/1011287:
- fix an additional source of flakiness in ChromeOS login tests

Changes from original attempt at https://crrev.com/c/999182:
- fix flakiness in two additional ChromeOS login tests
- fix CSP WPT tests to not depend on ordering between iframe's onload
  and postMessage - see  https://crbug.com/832319 .

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Tbr: dcheng@chromium.org
Change-Id: If2cc6591db31471adff0d84ec0b689905e21cdf1
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#550284}
Reviewed-on: https://chromium-review.googlesource.com/1011287
Cr-Original-Commit-Position: refs/heads/master@{#550621}
Reviewed-on: https://chromium-review.googlesource.com/1012472
Cr-Commit-Position: refs/heads/master@{#550769}
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b

commit ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Apr 12 18:37:34 2018

Use PostTask to schedule cross-process postMessage forwarding.

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Change-Id: I1138bcdcd18c458a92dec92e145761b226596c7a
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550284}
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed26770582c5a12952a47cb79ea07697dc376237

commit ed26770582c5a12952a47cb79ea07697dc376237
Author: Menglu Huang <huangml@chromium.org>
Date: Thu Apr 12 23:59:39 2018

Revert "Use PostTask to schedule cross-process postMessage forwarding."

This reverts commit ca9c8b3898a50707660c0e8b16f4dbc8e35b6a2b.

Reason for revert: <INSERT REASONING HERE>
Breaks browser_tests on linux-chromeos-rel
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6852

Original change's description:
> Use PostTask to schedule cross-process postMessage forwarding.
> 
> Previously, we sent the IPC to forward a cross-process postMessage
> immediately.  This caused a behavioral difference from the
> same-process case where the postMessage is always scheduled.  Namely,
> in a scenario like this:
> 
>   frame.postMessage(...);
>   doSomethingThatSendsIPCsToFrame(frame);
> 
> the IPCs from JS following the postMessage would've been ordered
> incorrectly, causing |frame| to see their side effects after the
> postMessage dispatch in the cross-process case, whereas they would be
> seen before the postMessage dispatch in the same-process case.  One
> example of this is frame.focus(), and another is a frame element
> onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
> a postMessage dispatched from an inline script while the frame was
> still loading.
> 
> To resolve these ordering concerns, this CL changes cross-process
> postMessage to do a PostTask on the sender side before sending the
> message to the browser process.  This improves the current state of
> the world, but does not yet achieve a perfect match for the IPC
> ordering in the same-process case - see discussion on the bug.
> 
> Bug: 828529
> Change-Id: I1138bcdcd18c458a92dec92e145761b226596c7a
> Reviewed-on: https://chromium-review.googlesource.com/999182
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#550284}

TBR=xiyuan@chromium.org,wjmaclean@google.com,dcheng@chromium.org,alexmos@chromium.org

Change-Id: I11e88ed08c096be2534a6ab103f5553f3a4cfb9f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 828529
Reviewed-on: https://chromium-review.googlesource.com/1011564
Reviewed-by: Menglu Huang <huangml@chromium.org>
Commit-Queue: Menglu Huang <huangml@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550421}
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/ed26770582c5a12952a47cb79ea07697dc376237/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9

commit 7c3d1d13f940e88ef6706fd8b5c257a81d340ed9
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Apr 13 15:06:53 2018

Reland: Use PostTask to schedule cross-process postMessage forwarding.

Changes from original attempt at https://crrev.com/c/999182:
- fix flakiness in two additional ChromeOS login tests
- fix CSP WPT tests to not depend on ordering between iframe's onload
  and postMessage - see  https://crbug.com/832319 .

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Change-Id: I62a627c501526d09900be4f5bd2c899acf4d1e07
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#550284}
Reviewed-on: https://chromium-review.googlesource.com/1011287
Cr-Commit-Position: refs/heads/master@{#550621}
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/7c3d1d13f940e88ef6706fd8b5c257a81d340ed9/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4

commit c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4
Author: Ben Pastene <bpastene@chromium.org>
Date: Fri Apr 13 16:49:20 2018

Revert "Reland: Use PostTask to schedule cross-process postMessage forwarding."

This reverts commit 7c3d1d13f940e88ef6706fd8b5c257a81d340ed9.

Reason for revert: WebviewLoginTest.Basic is still flaky on linux-chromeos-rel
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6886
https://ci.chromium.org/buildbot/chromium.chromiumos/linux-chromeos-rel/6887


Original change's description:
> Reland: Use PostTask to schedule cross-process postMessage forwarding.
> 
> Changes from original attempt at https://crrev.com/c/999182:
> - fix flakiness in two additional ChromeOS login tests
> - fix CSP WPT tests to not depend on ordering between iframe's onload
>   and postMessage - see  https://crbug.com/832319 .
> 
> Previously, we sent the IPC to forward a cross-process postMessage
> immediately.  This caused a behavioral difference from the
> same-process case where the postMessage is always scheduled.  Namely,
> in a scenario like this:
> 
>   frame.postMessage(...);
>   doSomethingThatSendsIPCsToFrame(frame);
> 
> the IPCs from JS following the postMessage would've been ordered
> incorrectly, causing |frame| to see their side effects after the
> postMessage dispatch in the cross-process case, whereas they would be
> seen before the postMessage dispatch in the same-process case.  One
> example of this is frame.focus(), and another is a frame element
> onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
> a postMessage dispatched from an inline script while the frame was
> still loading.
> 
> To resolve these ordering concerns, this CL changes cross-process
> postMessage to do a PostTask on the sender side before sending the
> message to the browser process.  This improves the current state of
> the world, but does not yet achieve a perfect match for the IPC
> ordering in the same-process case - see discussion on the bug.
> 
> Bug: 828529
> Change-Id: I62a627c501526d09900be4f5bd2c899acf4d1e07
> Reviewed-on: https://chromium-review.googlesource.com/999182
> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#550284}
> Reviewed-on: https://chromium-review.googlesource.com/1011287
> Cr-Commit-Position: refs/heads/master@{#550621}

TBR=xiyuan@chromium.org,dcheng@chromium.org,alexmos@chromium.org

Change-Id: Ic0637a6038bed6e5334a26e1934bee81faad3b9e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 828529
Reviewed-on: https://chromium-review.googlesource.com/1012138
Reviewed-by: Ben Pastene <bpastene@chromium.org>
Commit-Queue: Ben Pastene <bpastene@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550649}
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/c3895d83d3b75b2258e3070ec1f1e79fb74c7bb4/third_party/blink/renderer/core/loader/empty_clients.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07a131898bfa3810ba928189472f45d4c1784a0e

commit 07a131898bfa3810ba928189472f45d4c1784a0e
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Fri Apr 13 23:02:18 2018

Reland 2: Use PostTask to schedule cross-process postMessage forwarding.

Changes from first reland attempt at https://crrev.com/c/1011287:
- fix an additional source of flakiness in ChromeOS login tests

Changes from original attempt at https://crrev.com/c/999182:
- fix flakiness in two additional ChromeOS login tests
- fix CSP WPT tests to not depend on ordering between iframe's onload
  and postMessage - see  https://crbug.com/832319 .

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

Bug: 828529
Tbr: dcheng@chromium.org
Change-Id: If2cc6591db31471adff0d84ec0b689905e21cdf1
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/master@{#550284}
Reviewed-on: https://chromium-review.googlesource.com/1011287
Cr-Original-Commit-Position: refs/heads/master@{#550621}
Reviewed-on: https://chromium-review.googlesource.com/1012472
Cr-Commit-Position: refs/heads/master@{#550769}
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/07a131898bfa3810ba928189472f45d4c1784a0e/third_party/blink/renderer/core/loader/empty_clients.h

The second reland from #36 has been in a few canaries and seems to be sticking, so requesting merge of r550769 to M67.  (Note that bugdroid comments 37-41 seem to duplicate earlier ones - not sure why they were made, maybe by mistake?)

I've verified this on Mac canary 68.0.3398.0 using the page and steps in c#26.  The postMessages do fire after the iframe load event now, though interestingly the canary behavior with --site-per-process now matches Edge - not only the frame load event, but also the setTimeout fires before any of the postMessages get there.  I suppose that's still ok for our goals.  It's interesting that the timing in a local build was a bit different.  Changing the setTimeout from 0ms to 1ms on my test page makes setTimeout fire after all the postMessages, as in the same-process case.

I have some trouble playing the video to verify issue 822958 in Mac canary at the moment, with or without site isolation, and I was seeing this also on 67.0.3393.0 (without this fix) before I restarted.  The video doesn't play at all, being stuck on the first frame, and clicking the pause button does nothing.  Is anyone else seeing this?  Manually scrolling the video to the beginning does trigger the fade-in ad that wasn't shown before with --site-per-process, though this repro is a bit different from the original one.

Comment 42: Thanks for verifying!  For testing issue 822958, did you set chrome://flags#autoplay-policy to "No user gesture is required"?  That was necessary for the video to autoplay.

Hmm, I just tested myself on Mac Canary 68.0.3398.0 with --site-per-process and chrome://flags#autoplay-policy set to "No user gesture is required."  I'm still seeing no video ad there...  That's odd to me, because my test in comment 16 did resolve it in a local build.  Can you check as well?

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Per comment #42, only r550769  needs to be merged to M67, correct? If yes, is the change well baked/verified in canary and safe to merge?

#46: correct, this merge request is only for r550769, and it's been verified in canary (see #42).  It looks like issue 822958 might require additional work, but this fix fixes an important class of problems by itself, so we probably want to merge it regardless of issue 822958.

#43-44: Hmm, I thought I posted a reply to that yesterday, but I don't see it now.  In any case, indeed I forgot to update chrome://flags#autoplay-policy in the profile where I was testing in #42, and with the updated flag I also don't see the video ad on on the URL for issue 822958 on Mac or Win canary 68.0.3398.0.

I tried this out in a local official build, since the local build's timing might be different, as I found out in #42.  Interestingly, my local build with r550769 is *also* not showing the video ad.  However, adding an explicit UpdateStyleAndLayout() call on the |source| document before sending out the postMessage does show the video ad.  So I think we have to solve the sizing aspects of this to fix issue 822958, i.e., either force layout before sending the postMessage or delay postMessage until the next layout (suggested in #15 and #17).

Approving merge to r550769 to M67 branch 3396 based on comment #47. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb65b4fb4244a976ec36a78e47b06a0539c5ada4

commit fb65b4fb4244a976ec36a78e47b06a0539c5ada4
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Thu Apr 19 18:04:27 2018

Reland 2: Use PostTask to schedule cross-process postMessage forwarding (Merge to M67).

Changes from first reland attempt at https://crrev.com/c/1011287:
- fix an additional source of flakiness in ChromeOS login tests

Changes from original attempt at https://crrev.com/c/999182:
- fix flakiness in two additional ChromeOS login tests
- fix CSP WPT tests to not depend on ordering between iframe's onload
  and postMessage - see  https://crbug.com/832319 .

Previously, we sent the IPC to forward a cross-process postMessage
immediately.  This caused a behavioral difference from the
same-process case where the postMessage is always scheduled.  Namely,
in a scenario like this:

  frame.postMessage(...);
  doSomethingThatSendsIPCsToFrame(frame);

the IPCs from JS following the postMessage would've been ordered
incorrectly, causing |frame| to see their side effects after the
postMessage dispatch in the cross-process case, whereas they would be
seen before the postMessage dispatch in the same-process case.  One
example of this is frame.focus(), and another is a frame element
onload event (dispatched via FrameHostMsg_DispatchLoad) arriving after
a postMessage dispatched from an inline script while the frame was
still loading.

To resolve these ordering concerns, this CL changes cross-process
postMessage to do a PostTask on the sender side before sending the
message to the browser process.  This improves the current state of
the world, but does not yet achieve a perfect match for the IPC
ordering in the same-process case - see discussion on the bug.

TBR=alexmos@chromium.org

(cherry picked from commit 07a131898bfa3810ba928189472f45d4c1784a0e)

Bug: 828529
Tbr: dcheng@chromium.org
Change-Id: If2cc6591db31471adff0d84ec0b689905e21cdf1
Reviewed-on: https://chromium-review.googlesource.com/999182
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#550284}
Reviewed-on: https://chromium-review.googlesource.com/1011287
Cr-Original-Original-Commit-Position: refs/heads/master@{#550621}
Reviewed-on: https://chromium-review.googlesource.com/1012472
Cr-Original-Commit-Position: refs/heads/master@{#550769}
Reviewed-on: https://chromium-review.googlesource.com/1019608
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#134}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/chrome/browser/chromeos/login/test/oobe_base_test.cc
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/chrome/browser/chromeos/login/test/oobe_base_test.h
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/chrome/browser/chromeos/login/webview_login_browsertest.cc
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/chrome/test/data/extensions/platform_apps/web_view/edit_commands_no_menu/embedder.js
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/chrome/test/data/extensions/platform_apps/web_view/focus/embedder.js
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/WebKit/LayoutTests/external/wpt/content-security-policy/embedded-enforcement/support/testharness-helper.sub.js
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/frame/remote_dom_window.cc
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/frame/remote_dom_window.h
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/frame/remote_frame_client.h
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/frame/remote_frame_client_impl.cc
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/frame/remote_frame_client_impl.h
[modify] https://crrev.com/fb65b4fb4244a976ec36a78e47b06a0539c5ada4/third_party/blink/renderer/core/loader/empty_clients.h

Charlie and I looked at this yesterday, and it turns out Charlie also had a call to UpdateStyleAndLayout() after the PostTask fired and before forwarding the postMessage in his version of the fix.

As the next step, we'd like to measure the impact of delaying postMessage until the end of layout, if there's a layout pending.  That seems slightly better than forcing UpdateStyleAndLayout(), which might unnecessarily increase the number of layouts, as noted above.

A couple of questions to folks that know painting and layout:
- what's the right way to check if the layout is dirty when we need to send the postMessage?  For the sizing repro in this bug, source->NeedsLayoutTreeUpdate() seems to give me what I want, but I'm not sure if this covers all cases.  We'd want to use this after the postMessage PostTask fires to decide whether to forward the postMessage immediately or delay until next layout.

- if layout is needed, I'd like to add an UMA for how much we'd be delaying the postMessage if we wait for next layout.  What's a good way to post a task that will fire once the next layout is complete?  Ken's suggestion of RenderWidget::QueueMessage() seems to be tailored to delaying sending of IPC messages, whereas here I just need to log an UMA.

I think (Document::NeedsLayoutTreeUpdate() || LocalFrameView::NeedsLayout()) as the former checks for an up-to-date box tree and the latter if the layout is clean.

#52: Thanks!  I tried this out and used QueueSwapPromise to wait for layout to complete (don't know if there's a better way -- this is what seems to be used by RenderWidget::QueueMessage, which kenrb@ suggested).  I've realized that we can't just unconditionally wait for next layout because it seems that layout might be delayed indefinitely.  E.g. if the postMessage is sent while the tab is in background, I don't get the cc::SwapPromise resolved until the tab becomes visible again.  

Since we can't delay postMessages indefinitely, it seems we'd have to set a timeout for some threshold (e.g., 5ms), and if layout hasn't happened yet by that point, we'd have to force it and then send the message - this is a hybrid option Charlie and I also discussed.  I'll see how feasible that is.  We'd only need to do this if layout is actually dirty after the initial PostTask fires.

One question is whether this is restricted to ancestor->descendant postMessages, or are there other cases where the target frame can observe layout-induced effects in the source frame?  Maybe we can restrict this fix to just ancestor->descendant cases?

Assuming we can do this, another idea is, what if instead of checking that the whole document needs layout, we checked that only the target's HTMLFrameOwnerElement in the source document is dirty?  In practice, that's what seems to happen in the video ad repro from issue 822958 - it seems to be modifying "width" and "height" iframe attributes just like the repro in this bug.  I've just tried this out, by checking source->NeedsLayoutTreeUpdateForNode(frame_owner_element) and only forcing layout in that case (via source->UpdateStyleAndLayout()), and that is enough to fix the bug in issue 822958.  Maybe this case is rare enough that we can just force layout as a short-term fix for issue 822958, without being too concerned about extra layouts?

Unfortunately, I think that, even if only done when layout is dirty, delaying until the next natural layout is going to be a performance problem, because it could delay the message for a potentially unbounded amount of time, since layout takes as long as it takes.

I don't have a great idea yet on how to solve this bug. Will think some more. 

Are there real sites that are hitting the issue in #0? If not then it would be better to just
do nothing about this issue, as it would avoid introducing a dependency between rendering updates
and messages. With the PostTask approach alexmos implemented, developers already have a way to
achieve what they want reliably, via code like:

/* parent frame*/

function rafCallback() {
  childFrame.postMessage("message"));
}

dirtyLayoutOfChildFrameSize();

requestAnimationFrame(rafCallback);

rafCallback happens during the event loop task that updates rendering. Other JS tasks cannot run between the execution of rafCallback and the Blink rendering update. The Blink rendering update will if necessary queue up a message the child frame renderer if its size changed. Therefore this update will be queued before the PostTask
introduced by commit fb65b4fb4244a976ec36a78e47b06a0539c5ada4, and all will be well.

"message to the child frame renderer"

Unfortunately, there are sites that are hitting this problem, such as in issue 822958.  It's possible they could deploy a workaround like comment 55, but we're concerned more sites could already have a similar dependency on the old layout order and face the same problem.

If delaying until layout happens could be arbitrarily long, what did you think about alexmos's proposal in comment 53 to force a layout, but only in the narrow case of when the target's HTMLFrameOwnerElement is dirty?  That would hopefully cause fewer forced layouts than a general "force layout on every postMessage" approach (which we could monitor with metrics), offer similar ordering/performance to the same-process case, and stay focused on the cases most likely to have message handlers that depend on the layout.

Regarding the idea of checking only whether the HTMLFrameOwnerElement is dirty: that wouldn't
work in cases the iframe element has a size relative to its ancestor and the ancestor's layout
is dirty. It might work in most of the cases, but is not a very clean solution..

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb9795395ef6fbdccde33f5fbcbf5e8c33010658

commit fb9795395ef6fbdccde33f5fbcbf5e8c33010658
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Sat May 19 22:01:50 2018

Force visual properties to be synced before cross-process postMessage.

This CL fixes a reordering problem between sizing changes and
cross-process postMessages.  Previously, it was possible for a frame
to resize its child frame (forcing layout to guarantee the sizing IPC
is sent), then send it a postMessage, and have the message delivered
before the sizing change.  This is because the browser process might
throttle resizes, in particular if there had been another pending
resize which hadn't been acked by the (child) renderer.  To fix this,
this CL flushes any pending sizing info to the child renderer before
forwarding the postMessage.

Bug: 822958, 828529
Change-Id: I49252ed6f91b5c25be96b210fd3e3c6248f14da6
Reviewed-on: https://chromium-review.googlesource.com/1063310
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560176}
[modify] https://crrev.com/fb9795395ef6fbdccde33f5fbcbf5e8c33010658/content/browser/frame_host/render_frame_proxy_host.cc
[modify] https://crrev.com/fb9795395ef6fbdccde33f5fbcbf5e8c33010658/content/browser/renderer_host/render_widget_host_impl.cc
[modify] https://crrev.com/fb9795395ef6fbdccde33f5fbcbf5e8c33010658/content/browser/renderer_host/render_widget_host_impl.h
[modify] https://crrev.com/fb9795395ef6fbdccde33f5fbcbf5e8c33010658/content/browser/site_per_process_browsertest.cc

Is CL listed at #60 need a merge to M67? If yes, how safe will it be to merge to M67 this late in release cycle?

Comment 61: I just posted an update here:
https://bugs.chromium.org/p/chromium/issues/detail?id=822958#c62

Bottom line, I'm guessing we won't need to merge it, but we'll know for sure tomorrow.

The case in issue 822958 is now resolved, but we should follow up here on whether any other Chrome changes or recommendations to developers about postMessage vs resize/layout expectations should be pursued.