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}

content/browser/frame_host/render_frame_proxy_host.cc

@@ -341,6 +341,23 @@ void RenderFrameProxyHost::OnRouteMessageEvent(
     if (!source_rfh) {
       new_params.source_routing_id = MSG_ROUTING_NONE;
     } else {
+      // https://crbug.com/822958: If the postMessage is going to a descendant
+      // frame, ensure that any pending visual properties such as size are sent
+      // to the target frame before the postMessage, as sites might implicitly
+      // be relying on this ordering.
+      bool target_is_descendant_of_source = false;
+      for (FrameTreeNode* node = target_rfh->frame_tree_node(); node;
+           node = node->parent()) {
+        if (node == source_rfh->frame_tree_node()) {
+          target_is_descendant_of_source = true;
+          break;
+        }
+      }
+      if (target_is_descendant_of_source) {
+        target_rfh->GetRenderWidgetHost()
+            ->SynchronizeVisualPropertiesIgnoringPendingAck();
+      }
+
       // Ensure that we have a swapped-out RVH and proxy for the source frame
       // in the target SiteInstance. If it doesn't exist, create it on demand
       // and also create its opener chain, since that will also be accessible

content/browser/renderer_host/render_widget_host_impl.cc

@@ -891,6 +891,11 @@ bool RenderWidgetHostImpl::SynchronizeVisualProperties() {
   return SynchronizeVisualProperties(false);
 }
 
+void RenderWidgetHostImpl::SynchronizeVisualPropertiesIgnoringPendingAck() {
+  visual_properties_ack_pending_ = false;
+  SynchronizeVisualProperties();
+}
+
 bool RenderWidgetHostImpl::SynchronizeVisualProperties(
     bool scroll_focused_node_into_view) {
   // Skip if the |delegate_| has already been detached because

content/browser/renderer_host/render_widget_host_impl.h

@@ -576,6 +576,11 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   // focused node should be scrolled into view.
   bool SynchronizeVisualProperties(bool scroll_focused_node_into_view);
 
+  // Similar to SynchronizeVisualProperties(), but performed even if
+  // |visual_properties_ack_pending_| is set.  Used to guarantee that the
+  // latest visual properties are sent to the renderer before another IPC.
+  void SynchronizeVisualPropertiesIgnoringPendingAck();
+
   // Called when we receive a notification indicating that the renderer process
   // is gone. This will reset our state so that our state will be consistent if
   // a new renderer is created.
@@ -776,6 +781,8 @@ class CONTENT_EXPORT RenderWidgetHostImpl
                            ChildAllocationAcceptedInParent);
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewMacTest,
                            ConflictingAllocationsResolve);
+  FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
+                           ResizeAndCrossProcessPostMessagePreserveOrder);
   friend class MockRenderWidgetHost;
   friend class OverscrollNavigationOverlayTest;
   friend class TestRenderViewHost;

content/browser/site_per_process_browsertest.cc

@@ -12051,4 +12051,47 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
       CrossProcessFrameConnector::CrashVisibility::kShownAfterCrashing, 1);
 }
 
+// Check that when a frame changes a subframe's size twice and then sends a
+// postMessage to the subframe, the subframe's onmessage handler sees the new
+// size.  In particular, ensure that the postMessage won't get reordered with
+// the second resize, which might be throttled if the first resize is still in
+// progress. See https://crbug.com/828529.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       ResizeAndCrossProcessPostMessagePreserveOrder) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  FrameTreeNode* root = web_contents()->GetFrameTree()->root();
+
+  // Add an onmessage handler to the subframe to send back its width.
+  EXPECT_TRUE(ExecuteScript(root->child_at(0), R"(
+      window.addEventListener('message', function(event) {
+        domAutomationController.send(document.body.clientWidth);
+      });)"));
+
+  // Drop the visual properties ACKs from the child renderer.  To do this,
+  // unsubscribe the child's RenderWidgetHost from its
+  // RenderFrameMetadataProvider, which ensures that
+  // DidUpdateVisualProperties() won't be called on it, and the ACK won't be
+  // reset.  This simulates that the ACK for the first resize below does not
+  // arrive before the second resize IPC arrives from the
+  // parent, and that the second resize IPC early-exits in
+  // SynchronizeVisualProperties() due to the pending visual properties ACK.
+  RenderWidgetHostImpl* rwh =
+      root->child_at(0)->current_frame_host()->GetRenderWidgetHost();
+  rwh->render_frame_metadata_provider_.RemoveObserver(rwh);
+
+  // Now, resize the subframe twice from the main frame and send it a
+  // postMessage. The postMessage handler should see the second updated size.
+  int width = 0;
+  EXPECT_TRUE(ExecuteScriptAndExtractInt(root, R"(
+      var f = document.querySelector('iframe');
+      f.width = 500;
+      f.offsetTop; // force layout; this sends a resize IPC for width of 500.
+      f.width = 700;
+      f.offsetTop; // force layout; this sends a resize IPC for width of 700.
+      f.contentWindow.postMessage('foo', '*');)", &width));
+  EXPECT_EQ(width, 700);
+}
+
 }  // namespace content