Transmit FrameVisualProperties together with ViewportIntersectionState

Both of these structs contain information based on the geometry of an
iframe within its embedding document, and they are updated at the same
time, but they are transmitted to the remote render process via
independent IPC's. This creates a race condition in the remote render
process: if it happens to run a lifecycle update in between receiving
the two IPC's, it will have an inconsistent view of its frame geometry.

With this patch, the remote render process will receive both updated
structs via a single IPC.

Bug: 1135714
Change-Id: I93224e0a97c7f37896c925b6b97caa2328267710
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628746
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844297}

content/browser/renderer_host/cross_process_frame_connector.cc

@@ -150,7 +150,10 @@ void CrossProcessFrameConnector::SendIntrinsicSizingInfoToParent(
 }
 
 void CrossProcessFrameConnector::SynchronizeVisualProperties(
-    const blink::FrameVisualProperties& visual_properties) {
+    const blink::FrameVisualProperties& visual_properties,
+    bool propagate) {
+  last_received_zoom_level_ = visual_properties.zoom_level;
+  last_received_local_frame_size_ = visual_properties.local_frame_size;
   screen_info_ = visual_properties.screen_info;
   local_surface_id_ = visual_properties.local_surface_id;
 
@@ -176,7 +179,7 @@ void CrossProcessFrameConnector::SynchronizeVisualProperties(
       visual_properties.compositor_viewport,
       visual_properties.root_widget_window_segments);
 
-  render_widget_host->SynchronizeVisualProperties();
+  render_widget_host->UpdateVisualProperties(propagate);
 }
 
 void CrossProcessFrameConnector::UpdateCursor(const WebCursor& cursor) {
@@ -320,16 +323,25 @@ void CrossProcessFrameConnector::OnSynchronizeVisualProperties(
     return;
   }
 
-  last_received_zoom_level_ = visual_properties.zoom_level;
-  last_received_local_frame_size_ = visual_properties.local_frame_size;
   SynchronizeVisualProperties(visual_properties);
 }
 
 void CrossProcessFrameConnector::UpdateViewportIntersection(
+    const blink::mojom::ViewportIntersectionState& intersection_state,
+    const base::Optional<blink::FrameVisualProperties>& visual_properties) {
+  base::AutoReset<bool>(&is_processing_viewport_intersection_, true);
+  if (visual_properties.has_value())
+    SynchronizeVisualProperties(visual_properties.value(), false);
+  UpdateViewportIntersectionInternal(intersection_state);
+}
+
+void CrossProcessFrameConnector::UpdateViewportIntersectionInternal(
     const blink::mojom::ViewportIntersectionState& intersection_state) {
   intersection_state_ = intersection_state;
-  if (view_)
-    view_->UpdateViewportIntersection(intersection_state_);
+  if (view_) {
+    view_->UpdateViewportIntersection(
+        intersection_state_, view_->host()->LastComputedVisualProperties());
+  }
 
   if (IsVisible()) {
     // Record metrics if a crashed subframe became visible as a result of this

content/browser/renderer_host/cross_process_frame_connector.h

@@ -109,9 +109,11 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
   // its corresponding remote frame in the parent frame's renderer.
   void SendIntrinsicSizingInfoToParent(blink::mojom::IntrinsicSizingInfoPtr);
 
-  // Sends new resize parameters to the subframe's renderer.
+  // Record and apply new visual properties for the subframe. If 'propagate' is
+  // true, the new properties will be sent to the subframe's renderer process.
   void SynchronizeVisualProperties(
-      const blink::FrameVisualProperties& visual_properties);
+      const blink::FrameVisualProperties& visual_properties,
+      bool propagate = true);
 
   // Return the size of the CompositorFrame to use in the child renderer.
   const gfx::Size& local_frame_size_in_pixels() const {
@@ -283,7 +285,8 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
                                     bool subtree_throttled,
                                     bool display_locked);
   void UpdateViewportIntersection(
-      const blink::mojom::ViewportIntersectionState& intersection_state);
+      const blink::mojom::ViewportIntersectionState& intersection_state,
+      const base::Optional<blink::FrameVisualProperties>& visual_properties);
 
   // These enums back crashed frame histograms - see MaybeLogCrash() and
   // MaybeLogShownCrash() below.  Please do not modify or remove existing enum
@@ -331,8 +334,17 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
     use_zoom_for_device_scale_factor_ = use_zoom_for_device_scale_factor;
   }
 
+  // TODO(szager): This is a hack piled on top of a hack; see
+  // RenderWidgetHostViewChildFrame::WillSendScreenRects. We need a better way
+  // to initialize renderer process state after a frame migrates to a different
+  // process due to navigation.
+  bool IsProcessingViewportIntersection() const {
+    return is_processing_viewport_intersection_;
+  }
+
  protected:
   friend class MockCrossProcessFrameConnector;
+  friend class SitePerProcessBrowserTestBase;
 
   FRIEND_TEST_ALL_PREFIXES(RenderWidgetHostViewChildFrameZoomForDSFTest,
                            CompositorViewportPixelSize);
@@ -350,6 +362,9 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
   // Stability.ChildFrameCrash.Visibility.ShownAfterCrashing* metrics.
   void MaybeLogShownCrash(ShownAfterCrashingReason reason);
 
+  void UpdateViewportIntersectionInternal(
+      const blink::mojom::ViewportIntersectionState& intersection_state);
+
   // The RenderWidgetHostView for the frame. Initially nullptr.
   RenderWidgetHostViewChildFrame* view_ = nullptr;
 
@@ -407,6 +422,9 @@ class CONTENT_EXPORT CrossProcessFrameConnector {
   // shown after a crash. This is only used when recording renderer crashes.
   bool delegate_was_shown_after_crash_ = false;
 
+  // This is used to prevent re-entry into UpdateViewportIntersection.
+  bool is_processing_viewport_intersection_ = false;
+
   // The last pre-transform frame size received from the parent renderer.
   // |last_received_local_frame_size_| may be in DIP if use zoom for DSF is
   // off.

content/browser/renderer_host/render_frame_proxy_host.cc

@@ -745,9 +745,10 @@ void RenderFrameProxyHost::OpenURL(mojom::OpenURLParamsPtr params) {
 }
 
 void RenderFrameProxyHost::UpdateViewportIntersection(
-    blink::mojom::ViewportIntersectionStatePtr intersection_state) {
+    blink::mojom::ViewportIntersectionStatePtr intersection_state,
+    const base::Optional<blink::FrameVisualProperties>& visual_properties) {
   cross_process_frame_connector_->UpdateViewportIntersection(
-      *intersection_state);
+      *intersection_state, visual_properties);
 }
 
 void RenderFrameProxyHost::DidChangeOpener(

content/browser/renderer_host/render_frame_proxy_host.h

@@ -184,7 +184,9 @@ class CONTENT_EXPORT RenderFrameProxyHost
                                  int document_cookie) override;
   void Detach() override;
   void UpdateViewportIntersection(
-      blink::mojom::ViewportIntersectionStatePtr intersection_state) override;
+      blink::mojom::ViewportIntersectionStatePtr intersection_state,
+      const base::Optional<blink::FrameVisualProperties>& visual_properties)
+      override;
   void SynchronizeVisualProperties(
       const blink::FrameVisualProperties& frame_visual_properties) override;
 

content/browser/renderer_host/render_widget_host_impl.cc

@@ -1062,8 +1062,12 @@ blink::VisualProperties RenderWidgetHostImpl::GetVisualProperties() {
   return visual_properties;
 }
 
+bool RenderWidgetHostImpl::UpdateVisualProperties(bool propagate) {
+  return SynchronizeVisualProperties(false, propagate);
+}
+
 bool RenderWidgetHostImpl::SynchronizeVisualProperties() {
-  return SynchronizeVisualProperties(false);
+  return SynchronizeVisualProperties(false, true);
 }
 
 bool RenderWidgetHostImpl::SynchronizeVisualPropertiesIgnoringPendingAck() {
@@ -1072,7 +1076,8 @@ bool RenderWidgetHostImpl::SynchronizeVisualPropertiesIgnoringPendingAck() {
 }
 
 bool RenderWidgetHostImpl::SynchronizeVisualProperties(
-    bool scroll_focused_node_into_view) {
+    bool scroll_focused_node_into_view,
+    bool propagate) {
   // If the RenderViewHost is inactive, then there is no RenderWidget that can
   // receive visual properties yet, even though we are setting them on the
   // browser side. Wait until there is a local main frame with a RenderWidget
@@ -1125,7 +1130,8 @@ bool RenderWidgetHostImpl::SynchronizeVisualProperties(
   visual_properties->scroll_focused_node_into_view =
       scroll_focused_node_into_view;
 
-  blink_widget_->UpdateVisualProperties(*visual_properties);
+  if (propagate)
+    blink_widget_->UpdateVisualProperties(*visual_properties);
 
   bool width_changed =
       !old_visual_properties_ || old_visual_properties_->new_size.width() !=
@@ -2939,7 +2945,7 @@ RenderWidgetHostImpl::GetFrameWidgetInputHandler() {
 }
 
 base::Optional<blink::VisualProperties>
-RenderWidgetHostImpl::GetLastVisualPropertiesSentToRendererForTesting() {
+RenderWidgetHostImpl::LastComputedVisualProperties() const {
   if (!old_visual_properties_)
     return base::nullopt;
   return *old_visual_properties_;

content/browser/renderer_host/render_widget_host_impl.h

@@ -307,6 +307,10 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   void SetPopupBounds(const gfx::Rect& bounds,
                       SetPopupBoundsCallback callback) override;
 
+  // Update the stored set of visual properties for the renderer. If 'propagate'
+  // is true, the new properties will be sent to the renderer process.
+  bool UpdateVisualProperties(bool propagate);
+
   // Notification that the screen info has changed.
   void NotifyScreenInfoChanged();
 
@@ -665,7 +669,8 @@ class CONTENT_EXPORT RenderWidgetHostImpl
 
   // Pushes updated visual properties to the renderer as well as whether the
   // focused node should be scrolled into view.
-  bool SynchronizeVisualProperties(bool scroll_focused_node_into_view);
+  bool SynchronizeVisualProperties(bool scroll_focused_node_into_view,
+                                   bool propagate = true);
 
   // Similar to SynchronizeVisualProperties(), but performed even if
   // |visual_properties_ack_pending_| is set.  Used to guarantee that the
@@ -823,6 +828,8 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   base::Optional<blink::VisualProperties>
   GetLastVisualPropertiesSentToRendererForTesting();
 
+  base::Optional<blink::VisualProperties> LastComputedVisualProperties() const;
+
  protected:
   // |routing_id| must not be MSG_ROUTING_NONE.
   // If this object outlives |delegate|, DetachDelegate() must be called when

content/browser/renderer_host/render_widget_host_view_child_frame.cc

@@ -429,15 +429,16 @@ void RenderWidgetHostViewChildFrame::UnregisterFrameSinkId() {
 }
 
 void RenderWidgetHostViewChildFrame::UpdateViewportIntersection(
-    const blink::mojom::ViewportIntersectionState& intersection_state) {
+    const blink::mojom::ViewportIntersectionState& intersection_state,
+    const base::Optional<blink::VisualProperties>& visual_properties) {
   if (host()) {
     host()->SetIntersectsViewport(
         !intersection_state.viewport_intersection.IsEmpty());
 
     // Do not send viewport intersection to main frames.
     if (!host()->owner_delegate()) {
       host()->GetAssociatedFrameWidget()->SetViewportIntersection(
-          intersection_state.Clone());
+          intersection_state.Clone(), visual_properties);
     }
   }
 }
@@ -739,7 +740,10 @@ void RenderWidgetHostViewChildFrame::WillSendScreenRects() {
   // spammy way to do this, but triggering on SendScreenRects() is reasonable
   // until somebody figures that out. RWHVCF::Init() is too early.
   if (frame_connector_) {
-    UpdateViewportIntersection(frame_connector_->intersection_state());
+    if (!frame_connector_->IsProcessingViewportIntersection()) {
+      UpdateViewportIntersection(frame_connector_->intersection_state(),
+                                 base::nullopt);
+    }
     SetIsInert();
     UpdateInheritedEffectiveTouchAction();
     UpdateRenderThrottlingStatus();

content/browser/renderer_host/render_widget_host_view_child_frame.h

@@ -27,6 +27,7 @@
 #include "content/common/content_export.h"
 #include "content/public/browser/touch_selection_controller_client_manager.h"
 #include "services/viz/public/mojom/compositing/compositor_frame_sink.mojom.h"
+#include "third_party/blink/public/common/widget/visual_properties.h"
 #include "third_party/blink/public/mojom/frame/intrinsic_sizing_info.mojom-forward.h"
 #include "third_party/blink/public/mojom/frame/viewport_intersection_state.mojom-forward.h"
 #include "third_party/blink/public/mojom/input/input_event_result.mojom-shared.h"
@@ -181,7 +182,8 @@ class CONTENT_EXPORT RenderWidgetHostViewChildFrame
   void UnregisterFrameSinkId();
 
   void UpdateViewportIntersection(
-      const blink::mojom::ViewportIntersectionState& intersection_state);
+      const blink::mojom::ViewportIntersectionState& intersection_state,
+      const base::Optional<blink::VisualProperties>& visual_properties);
 
   // TODO(sunxd): Rename SetIsInert to UpdateIsInert.
   void SetIsInert();

content/browser/renderer_host/render_widget_host_view_child_frame_browsertest.cc

@@ -233,7 +233,7 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
     // Wait to see the size sent to the child RenderWidget.
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          child_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          child_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == initial_size)
         break;
       base::RunLoop().RunUntilIdle();
@@ -250,7 +250,7 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
     // Wait to see the size sent to the child RenderWidget.
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          nested_child_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          nested_child_rwh->LastComputedVisualProperties();
       if (properties &&
           properties->visible_viewport_size == nested_initial_size)
         break;
@@ -279,14 +279,14 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
     // Wait to see both RenderWidgets receive the message.
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          root_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          root_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == resize_to)
         break;
       base::RunLoop().RunUntilIdle();
     }
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          child_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          child_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == resize_to)
         break;
       base::RunLoop().RunUntilIdle();
@@ -310,14 +310,14 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
     // Wait to see both RenderWidgets receive the message.
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          nested_root_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          nested_root_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == resize_to)
         break;
       base::RunLoop().RunUntilIdle();
     }
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          nested_child_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          nested_child_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == resize_to)
         break;
       base::RunLoop().RunUntilIdle();
@@ -350,14 +350,14 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
     // waterfall to pass the new |visible_viewport_size| down.
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          root_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          root_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == auto_resize_to)
         break;
       base::RunLoop().RunUntilIdle();
     }
     while (true) {
       base::Optional<blink::VisualProperties> properties =
-          child_rwh->GetLastVisualPropertiesSentToRendererForTesting();
+          child_rwh->LastComputedVisualProperties();
       if (properties && properties->visible_viewport_size == auto_resize_to)
         break;
       base::RunLoop().RunUntilIdle();
@@ -556,15 +556,15 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
   base::Optional<blink::VisualProperties> properties =
       oopchild->current_frame_host()
           ->GetRenderWidgetHost()
-          ->GetLastVisualPropertiesSentToRendererForTesting();
+          ->LastComputedVisualProperties();
   EXPECT_TRUE(properties);
   EXPECT_TRUE(properties->local_surface_id);
   viz::LocalSurfaceId oopchild_initial_lsid =
       properties->local_surface_id.value();
 
   properties = oopdescendant->current_frame_host()
                    ->GetRenderWidgetHost()
-                   ->GetLastVisualPropertiesSentToRendererForTesting();
+                   ->LastComputedVisualProperties();
   EXPECT_TRUE(properties);
   EXPECT_TRUE(properties->local_surface_id);
   viz::LocalSurfaceId oopdescendant_initial_lsid =
@@ -583,7 +583,7 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
       base::Optional<blink::VisualProperties> properties =
           oopchild->current_frame_host()
               ->GetRenderWidgetHost()
-              ->GetLastVisualPropertiesSentToRendererForTesting();
+              ->LastComputedVisualProperties();
       if (properties && properties->local_surface_id &&
           oopchild_initial_lsid < properties->local_surface_id) {
         EXPECT_EQ(properties->root_widget_window_segments, expected_segments);
@@ -595,7 +595,7 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
       base::Optional<blink::VisualProperties> properties =
           oopdescendant->current_frame_host()
               ->GetRenderWidgetHost()
-              ->GetLastVisualPropertiesSentToRendererForTesting();
+              ->LastComputedVisualProperties();
       if (properties && properties->local_surface_id &&
           oopdescendant_initial_lsid < properties->local_surface_id) {
         EXPECT_EQ(properties->root_widget_window_segments, expected_segments);
@@ -617,7 +617,7 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameBrowserTest,
       base::Optional<blink::VisualProperties> properties =
           oopdescendant->current_frame_host()
               ->GetRenderWidgetHost()
-              ->GetLastVisualPropertiesSentToRendererForTesting();
+              ->LastComputedVisualProperties();
       // This check is needed, since we'll get an IPC originating from
       // RenderWidgetHostImpl immediately after the frame is added with the
       // incorrect value (the segments are cascaded from the parent renderer