Suppress surface ID allocations during autoresize for child frames

This change makes the ResizeDueToAutoResize messages for child frames
transactional, with a begin/end. It also ensures that during a
transaction we don't allocate new LocalSurfaceIds, instead waiting
for the transaction to complete.

Bug: 789259,805073
Change-Id: I585ac6d14b9978280847f49205d03bf49b9bb4f8
Reviewed-on: https://chromium-review.googlesource.com/960981
Commit-Queue: Eric Karl <ericrk@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Chris Blume <cblume@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551114}

content/browser/frame_host/cross_process_frame_connector.cc

@@ -404,10 +404,14 @@ void CrossProcessFrameConnector::EmbedRendererWindowTreeClientInParent(
 }
 #endif
 
-void CrossProcessFrameConnector::ResizeDueToAutoResize(
-    const gfx::Size& new_size,
+void CrossProcessFrameConnector::BeginResizeDueToAutoResize() {
+  frame_proxy_in_parent_renderer_->Send(new FrameMsg_BeginResizeDueToAutoResize(
+      frame_proxy_in_parent_renderer_->GetRoutingID()));
+}
+
+void CrossProcessFrameConnector::EndResizeDueToAutoResize(
     uint64_t sequence_number) {
-  frame_proxy_in_parent_renderer_->Send(new FrameMsg_ResizeDueToAutoResize(
+  frame_proxy_in_parent_renderer_->Send(new FrameMsg_EndResizeDueToAutoResize(
       frame_proxy_in_parent_renderer_->GetRoutingID(), sequence_number));
 }
 

content/browser/frame_host/cross_process_frame_connector.h

@@ -111,8 +111,8 @@ class CONTENT_EXPORT CrossProcessFrameConnector
   void EmbedRendererWindowTreeClientInParent(
       ui::mojom::WindowTreeClientPtr window_tree_client) override;
 #endif
-  void ResizeDueToAutoResize(const gfx::Size& new_size,
-                             uint64_t sequence_number) override;
+  void BeginResizeDueToAutoResize() override;
+  void EndResizeDueToAutoResize(uint64_t sequence_number) override;
 
   // Set the visibility of immediate child views, i.e. views whose parent view
   // is |view_|.

content/browser/renderer_host/frame_connector_delegate.h

@@ -226,10 +226,14 @@ class CONTENT_EXPORT FrameConnectorDelegate {
       ui::mojom::WindowTreeClientPtr window_tree_client) {}
 #endif
 
-  // Called by RenderWidgetHostViewChildFrame when the child frame has resized
-  // to |new_size| because auto-resize is enabled.
-  virtual void ResizeDueToAutoResize(const gfx::Size& new_size,
-                                     uint64_t sequence_number) {}
+  // Called by RenderWidgetHostViewChildFrame when an auto-resize transaction
+  // starts.
+  virtual void BeginResizeDueToAutoResize() {}
+
+  // Called by RenderWidgetHostViewChildFrame when the child frame has finished
+  // an auto-resize transaction. Causes allocation of a new LocalSurfaceID
+  // associated with the new size.
+  virtual void EndResizeDueToAutoResize(uint64_t sequence_number) {}
 
   bool has_size() const { return has_size_; }
 

content/browser/renderer_host/render_widget_host_view_child_frame.cc

@@ -1036,13 +1036,12 @@ RenderWidgetHostViewChildFrame::ResizeDueToAutoResize(
     const gfx::Size& new_size,
     uint64_t sequence_number,
     const viz::LocalSurfaceId& local_surface_id) {
-  // TODO(cblume): This doesn't currently suppress allocation.
-  // It maintains existing behavior while using the suppression style.
-  // This will be addressed in a follow-up patch.
-  // See https://crbug.com/805073
+  if (frame_connector_)
+    frame_connector_->BeginResizeDueToAutoResize();
+
   base::OnceCallback<void()> allocation_task = base::BindOnce(
       &RenderWidgetHostViewChildFrame::OnResizeDueToAutoResizeComplete,
-      weak_factory_.GetWeakPtr(), new_size, sequence_number);
+      weak_factory_.GetWeakPtr(), sequence_number);
   return viz::ScopedSurfaceIdAllocator(std::move(allocation_task));
 }
 
@@ -1126,10 +1125,9 @@ bool RenderWidgetHostViewChildFrame::CanBecomeVisible() {
 }
 
 void RenderWidgetHostViewChildFrame::OnResizeDueToAutoResizeComplete(
-    const gfx::Size& new_size,
     uint64_t sequence_number) {
   if (frame_connector_)
-    frame_connector_->ResizeDueToAutoResize(new_size, sequence_number);
+    frame_connector_->EndResizeDueToAutoResize(sequence_number);
 }
 
 void RenderWidgetHostViewChildFrame::DidNavigate() {

content/browser/renderer_host/render_widget_host_view_child_frame.h

@@ -296,8 +296,7 @@ class CONTENT_EXPORT RenderWidgetHostViewChildFrame
   // using CSS.
   bool CanBecomeVisible();
 
-  void OnResizeDueToAutoResizeComplete(const gfx::Size& new_size,
-                                       uint64_t sequence_number);
+  void OnResizeDueToAutoResizeComplete(uint64_t sequence_number);
 
   std::vector<base::OnceClosure> frame_swapped_callbacks_;
 

content/browser/renderer_host/render_widget_host_view_child_frame_browsertest.cc

@@ -191,4 +191,55 @@ IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameTest, ChildFrameSinkId) {
                           base::Unretained(this)));
 }
 
+// Test that auto-resize messages only trigger a single allocation/response
+// from the child.
+IN_PROC_BROWSER_TEST_F(RenderWidgetHostViewChildFrameTest,
+                       ChildFrameAutoResizeMessages) {
+  EXPECT_TRUE(NavigateToURL(
+      shell(), embedded_test_server()->GetURL(
+                   "a.com", "/cross_site_iframe_factory.html?a(b)")));
+
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+
+  // Create our message filter to intercept messages.
+  scoped_refptr<UpdateResizeParamsMessageFilter> message_filter =
+      new UpdateResizeParamsMessageFilter();
+  root->current_frame_host()->GetProcess()->AddFilter(message_filter.get());
+
+  // Load cross-site page into iframe.
+  GURL cross_site_url(
+      embedded_test_server()->GetURL("foo.com", "/title2.html"));
+  // The child frame is created during this blocking call, on the UI thread.
+  // This is racing the IPC we are testing for, which arrives on the IO thread.
+  // Due to this we cannot get the pre-IPC value of the viz::FrameSinkId.
+  NavigateFrameToURL(root->child_at(0), cross_site_url);
+
+  RenderWidgetHostImpl* child_frame_impl =
+      root->child_at(0)->current_frame_host()->GetRenderWidgetHost();
+  child_frame_impl->SetAutoResize(true, gfx::Size(10, 10), gfx::Size(100, 100));
+
+  // Fake an auto-resize update from the parent renderer.
+  int routing_id = root->child_at(0)
+                       ->current_frame_host()
+                       ->GetRenderWidgetHost()
+                       ->GetRoutingID();
+  ViewHostMsg_ResizeOrRepaint_ACK_Params params;
+  params.view_size = gfx::Size(75, 75);
+  params.flags = 0;
+  params.sequence_number = 7;
+  viz::LocalSurfaceId current_id =
+      child_frame_impl->GetView()->GetLocalSurfaceId();
+  params.child_allocated_local_surface_id = viz::LocalSurfaceId(
+      current_id.parent_sequence_number(),
+      current_id.child_sequence_number() + 1, current_id.embed_token());
+  child_frame_impl->OnMessageReceived(
+      ViewHostMsg_ResizeOrRepaint_ACK(routing_id, params));
+
+  // The first UpdateResizeParams message received should have our new sequence
+  // number.
+  EXPECT_EQ(params.sequence_number, message_filter->WaitForSequenceNumber());
+}
+
 }  // namespace content

content/common/frame_messages.h

@@ -995,9 +995,14 @@ IPC_MESSAGE_ROUTED1(FrameMsg_PostMessageEvent, FrameMsg_PostMessage_Params)
 // Tells the RenderFrame to clear the focused element (if any).
 IPC_MESSAGE_ROUTED0(FrameMsg_ClearFocusedElement)
 
-// Informs the parent renderer that the child would like a new
-// viz::LocalSurfaceId in response to an auto-resize.
-IPC_MESSAGE_ROUTED1(FrameMsg_ResizeDueToAutoResize,
+// Informs the parent renderer that the child is beginning an autoresize
+// transaction.
+IPC_MESSAGE_ROUTED(FrameMsg_BeginResizeDueToAutoResize)
+
+// Informs the parent renderer that the child has completed an autoresize
+// transaction and that the child can now allocate a new viz::LocalSurfaceId
+// for its new size.
+IPC_MESSAGE_ROUTED1(FrameMsg_EndResizeDueToAutoResize,
                     uint64_t /* sequence_number */)
 
 // Requests a viz::LocalSurfaceId to enable auto-resize mode from the parent

content/renderer/render_frame_impl_browsertest.cc

@@ -29,6 +29,7 @@
 #include "content/renderer/mojo/blink_interface_registry_impl.h"
 #include "content/renderer/navigation_state_impl.h"
 #include "content/renderer/render_frame_impl.h"
+#include "content/renderer/render_frame_proxy.h"
 #include "content/renderer/render_view_impl.h"
 #include "content/test/fake_compositor_dependencies.h"
 #include "content/test/frame_host_test_interface.mojom.h"
@@ -1124,4 +1125,31 @@ TEST_F(RenderFrameRemoteInterfacesTest, ReusedOnSameDocumentNavigation) {
       {{GURL(kTestFirstURL), kFrameEventDidCommitSameDocumentLoad}});
 }
 
+// Verify that a RenderFrameProxy correctly handles autoresize.
+TEST_F(RenderFrameImplTest, ProxyAutoresize) {
+  auto* proxy = RenderFrameProxy::FromRoutingID(kFrameProxyRouteId);
+
+  // Send a ViewChanged message to initialize our proxy with a frame sink.
+  FrameMsg_ViewChanged_Params view_changed_params;
+  view_changed_params.frame_sink_id = viz::FrameSinkId(20, 21);
+  FrameMsg_ViewChanged view_changed(kFrameProxyRouteId, view_changed_params);
+  proxy->OnMessageReceived(view_changed);
+  render_thread_->sink().ClearMessages();
+
+  // Send a simulated sequence of messages representing an auto-resize
+  // transaction.
+  FrameMsg_BeginResizeDueToAutoResize begin_msg(kFrameProxyRouteId);
+  proxy->OnMessageReceived(begin_msg);
+  proxy->FrameRectsChanged(blink::WebRect(0, 0, 300, 300),
+                           blink::WebRect(0, 0, 300, 300));
+  FrameMsg_EndResizeDueToAutoResize end_msg(kFrameProxyRouteId, 7);
+  proxy->OnMessageReceived(end_msg);
+
+  // We should have exactly one UpdateResizeParams message.
+  const IPC::Message* msg1 = render_thread_->sink().GetUniqueMessageMatching(
+      FrameHostMsg_UpdateResizeParams::ID);
+  EXPECT_TRUE(msg1);
+  render_thread_->sink().ClearMessages();
+}
+
 }  // namespace content

content/renderer/render_frame_proxy.cc

@@ -417,7 +417,10 @@ bool RenderFrameProxy::OnMessageReceived(const IPC::Message& msg) {
                         OnSetFrameOwnerProperties)
     IPC_MESSAGE_HANDLER(FrameMsg_DidUpdateOrigin, OnDidUpdateOrigin)
     IPC_MESSAGE_HANDLER(InputMsg_SetFocus, OnSetPageFocus)
-    IPC_MESSAGE_HANDLER(FrameMsg_ResizeDueToAutoResize, OnResizeDueToAutoResize)
+    IPC_MESSAGE_HANDLER(FrameMsg_BeginResizeDueToAutoResize,
+                        OnBeginResizeDueToAutoResize)
+    IPC_MESSAGE_HANDLER(FrameMsg_EndResizeDueToAutoResize,
+                        OnEndResizeDueToAutoResize)
     IPC_MESSAGE_HANDLER(FrameMsg_EnableAutoResize, OnEnableAutoResize)
     IPC_MESSAGE_HANDLER(FrameMsg_DisableAutoResize, OnDisableAutoResize)
     IPC_MESSAGE_HANDLER(FrameMsg_SetFocusedFrame, OnSetFocusedFrame)
@@ -564,7 +567,14 @@ void RenderFrameProxy::OnScrollRectToVisible(
   web_frame_->ScrollRectToVisible(rect_to_scroll, params);
 }
 
-void RenderFrameProxy::OnResizeDueToAutoResize(uint64_t sequence_number) {
+void RenderFrameProxy::OnBeginResizeDueToAutoResize() {
+  DCHECK(!transaction_pending_);
+  transaction_pending_ = true;
+}
+
+void RenderFrameProxy::OnEndResizeDueToAutoResize(uint64_t sequence_number) {
+  DCHECK(transaction_pending_);
+  transaction_pending_ = false;
   pending_resize_params_.auto_resize_sequence_number = sequence_number;
   WasResized();
 }
@@ -590,7 +600,7 @@ void RenderFrameProxy::SetMusEmbeddedFrame(
 #endif
 
 void RenderFrameProxy::WasResized() {
-  if (!frame_sink_id_.is_valid() || crashed_)
+  if (!frame_sink_id_.is_valid() || crashed_ || transaction_pending_)
     return;
 
   bool synchronized_params_changed =

content/renderer/render_frame_proxy.h

@@ -123,6 +123,9 @@ class CONTENT_EXPORT RenderFrameProxy : public IPC::Listener,
   // IPC::Sender
   bool Send(IPC::Message* msg) override;
 
+  // IPC::Listener
+  bool OnMessageReceived(const IPC::Message& msg) override;
+
   // Out-of-process child frames receive a signal from RenderWidgetCompositor
   // when a compositor frame will begin.
   void WillBeginCompositorFrame();
@@ -213,9 +216,6 @@ class CONTENT_EXPORT RenderFrameProxy : public IPC::Listener,
 
   void SetChildFrameSurface(const viz::SurfaceInfo& surface_info);
 
-  // IPC::Listener
-  bool OnMessageReceived(const IPC::Message& msg) override;
-
   // IPC handlers
   void OnDeleteProxy();
   void OnChildFrameProcessGone();
@@ -250,7 +250,8 @@ class CONTENT_EXPORT RenderFrameProxy : public IPC::Listener,
   void OnSetHasReceivedUserGesture();
   void OnScrollRectToVisible(const gfx::Rect& rect_to_scroll,
                              const blink::WebScrollIntoViewParams& params);
-  void OnResizeDueToAutoResize(uint64_t sequence_number);
+  void OnBeginResizeDueToAutoResize();
+  void OnEndResizeDueToAutoResize(uint64_t sequence_number);
   void OnEnableAutoResize(const gfx::Size& min_size, const gfx::Size& max_size);
   void OnDisableAutoResize();
   void OnSetHasReceivedUserGestureBeforeNavigation(bool value);
@@ -300,6 +301,11 @@ class CONTENT_EXPORT RenderFrameProxy : public IPC::Listener,
   // |sent_resize_params_|.
   FrameResizeParams pending_resize_params_;
 
+  // Whether we are in the middle of a transaction which modifies
+  // |pending_resize_params_|. If so, we delay allocating a new LocalSurfaceId
+  // until the transaction completes.
+  bool transaction_pending_ = false;
+
   bool crashed_ = false;
 
   viz::FrameSinkId frame_sink_id_;

content/test/content_browser_test_utils_internal.cc

@@ -381,11 +381,17 @@ void UpdateResizeParamsMessageFilter::ResetRectRunLoop() {
 }
 
 viz::FrameSinkId UpdateResizeParamsMessageFilter::GetOrWaitForId() {
-  // No-opt if already quit.
+  // No-op if already quit.
   frame_sink_id_run_loop_.Run();
   return frame_sink_id_;
 }
 
+uint64_t UpdateResizeParamsMessageFilter::WaitForSequenceNumber() {
+  sequence_number_run_loop_.reset(new base::RunLoop);
+  sequence_number_run_loop_->Run();
+  return last_sequence_number_;
+}
+
 UpdateResizeParamsMessageFilter::~UpdateResizeParamsMessageFilter() {}
 
 void UpdateResizeParamsMessageFilter::OnUpdateResizeParams(
@@ -407,6 +413,13 @@ void UpdateResizeParamsMessageFilter::OnUpdateResizeParams(
       base::BindOnce(&UpdateResizeParamsMessageFilter::OnUpdatedFrameRectOnUI,
                      this, screen_space_rect_in_dip));
 
+  // Track each sequence number update.
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::BindOnce(
+          &UpdateResizeParamsMessageFilter::OnUpdatedSequenceNumberOnUI, this,
+          resize_params.auto_resize_sequence_number));
+
   // Record the received value. We cannot check the current state of the child
   // frame, as it can only be processed on the UI thread, and we cannot block
   // here.
@@ -441,6 +454,14 @@ void UpdateResizeParamsMessageFilter::OnUpdatedFrameSinkIdOnUI() {
   frame_sink_id_run_loop_.Quit();
 }
 
+void UpdateResizeParamsMessageFilter::OnUpdatedSequenceNumberOnUI(
+    uint64_t sequence_number) {
+  last_sequence_number_ = sequence_number;
+  if (sequence_number_run_loop_) {
+    sequence_number_run_loop_->QuitWhenIdle();
+  }
+}
+
 bool UpdateResizeParamsMessageFilter::OnMessageReceived(
     const IPC::Message& message) {
   IPC_BEGIN_MESSAGE_MAP(UpdateResizeParamsMessageFilter, message)

content/test/content_browser_test_utils_internal.h

@@ -208,6 +208,9 @@ class UpdateResizeParamsMessageFilter : public content::BrowserMessageFilter {
   // will return the new viz::FrameSinkId.
   viz::FrameSinkId GetOrWaitForId();
 
+  // Waits for the next sequence number to be received and returns it.
+  uint64_t WaitForSequenceNumber();
+
  protected:
   ~UpdateResizeParamsMessageFilter() override;
 
@@ -217,6 +220,7 @@ class UpdateResizeParamsMessageFilter : public content::BrowserMessageFilter {
   // |rect| is in DIPs.
   void OnUpdatedFrameRectOnUI(const gfx::Rect& rect);
   void OnUpdatedFrameSinkIdOnUI();
+  void OnUpdatedSequenceNumberOnUI(uint64_t sequence_number);
 
   bool OnMessageReceived(const IPC::Message& message) override;
 
@@ -227,6 +231,9 @@ class UpdateResizeParamsMessageFilter : public content::BrowserMessageFilter {
   bool screen_space_rect_received_;
   gfx::Rect last_rect_;
 
+  uint64_t last_sequence_number_ = 0;
+  std::unique_ptr<base::RunLoop> sequence_number_run_loop_;
+
   DISALLOW_COPY_AND_ASSIGN(UpdateResizeParamsMessageFilter);
 };