Surface synchronization: Fix one source of transient invariants viola…

…tions

At the most basic level, the invariant we want to maintain for surface
synchronization is:

Change in device scale factor or Change Compositor Viewport Size =>
Change in LocalSurfaceId

Browser side we always start with a valid initial local surface ID even
before we have a valid physical backing size. We shouldn't send a valid
LocalSurfaceId to the renderer if we don't have a physical backing size.

Bug: 814690
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Idc7f391d511615309732f792ff620a9fb7073d07
Reviewed-on: https://chromium-review.googlesource.com/941736
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Saman Sami <samans@chromium.org>
Commit-Queue: Fady Samuel <fsamuel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540292}

cc/trees/layer_tree_host.cc

@@ -1050,6 +1050,10 @@ void LayerTreeHost::SetViewportSizeAndScale(
     const gfx::Size& device_viewport_size,
     float device_scale_factor,
     const viz::LocalSurfaceId& local_surface_id) {
+  DCHECK(!local_surface_id.is_valid() || !device_viewport_size.IsEmpty())
+      << "A valid LocalSurfaceId has been provided with an empty device "
+         "viewport size "
+      << local_surface_id;
   // TODO(ccameron): Add CHECKs here for surface invariants violations.
   if (settings_.enable_surface_synchronization)
     SetLocalSurfaceId(local_surface_id);

content/browser/renderer_host/render_widget_host_impl.cc

@@ -775,7 +775,13 @@ bool RenderWidgetHostImpl::GetResizeParams(ResizeParams* resize_params) {
     resize_params->visible_viewport_size = view_->GetVisibleViewportSize();
     // TODO(ccameron): GetLocalSurfaceId is not synchronized with the device
     // scale factor of the surface. Fix this.
-    viz::LocalSurfaceId local_surface_id = view_->GetLocalSurfaceId();
+    // We can allocate a LocalSurfaceId on navigation prior to giving the widget
+    // a size. We should only propagate a LocalSurfaceId here if the
+    // compositor's viewport has a non-empty size.
+    viz::LocalSurfaceId local_surface_id =
+        resize_params->physical_backing_size.IsEmpty()
+            ? viz::LocalSurfaceId()
+            : view_->GetLocalSurfaceId();
     if (local_surface_id.is_valid())
       resize_params->local_surface_id = local_surface_id;
   }
@@ -2508,8 +2514,10 @@ void RenderWidgetHostImpl::DetachDelegate() {
 
 void RenderWidgetHostImpl::DidAllocateLocalSurfaceIdForAutoResize(
     uint64_t sequence_number) {
-  if (!view_ || last_auto_resize_request_number_ != sequence_number)
+  if (!view_ || !sequence_number ||
+      last_auto_resize_request_number_ != sequence_number) {
     return;
+  }
 
   viz::LocalSurfaceId local_surface_id(view_->GetLocalSurfaceId());
   if (local_surface_id.is_valid()) {

content/renderer/render_widget.cc

@@ -647,6 +647,8 @@ void RenderWidget::SetLocalSurfaceIdForAutoResize(
     const content::ScreenInfo& screen_info,
     uint32_t content_source_id,
     const viz::LocalSurfaceId& local_surface_id) {
+  DCHECK(!size_.IsEmpty());
+
   bool screen_info_changed = screen_info_ != screen_info;
 
   screen_info_ = screen_info;
@@ -870,6 +872,8 @@ void RenderWidget::OnEnableDeviceEmulation(
     resize_params.screen_info = screen_info_;
     resize_params.new_size = size_;
     resize_params.physical_backing_size = physical_backing_size_;
+    resize_params.local_surface_id = local_surface_id_;
+    resize_params.content_source_id = current_content_source_id_;
     resize_params.visible_viewport_size = visible_viewport_size_;
     resize_params.is_fullscreen_granted = is_fullscreen_granted_;
     resize_params.display_mode = display_mode_;
@@ -1409,7 +1413,7 @@ void RenderWidget::Resize(const ResizeParams& params) {
   // |current_content_source_id_|, then the given LocalSurfaceId was generated
   // before the navigation. Continue with the resize but don't use the
   // LocalSurfaceId until the right one comes.
-  viz::LocalSurfaceId new_local_surface_id = local_surface_id_;
+  viz::LocalSurfaceId new_local_surface_id;
   if (params.local_surface_id &&
       params.content_source_id == current_content_source_id_) {
     new_local_surface_id = *params.local_surface_id;
@@ -1424,7 +1428,8 @@ void RenderWidget::Resize(const ResizeParams& params) {
     // it receives a valid surface ID. This is a no-op if surface
     // synchronization is disabled.
     DCHECK(!compositor_->IsSurfaceSynchronizationEnabled() ||
-           !params.needs_resize_ack || params.local_surface_id->is_valid());
+           !params.needs_resize_ack || !params.local_surface_id ||
+           params.local_surface_id->is_valid());
     compositor_->SetBrowserControlsHeight(
         params.top_controls_height, params.bottom_controls_height,
         params.browser_controls_shrink_blink_size);

content/renderer/render_widget_browsertest.cc

@@ -53,10 +53,9 @@ class RenderWidgetTest : public RenderViewTest {
 };
 
 TEST_F(RenderWidgetTest, OnResize) {
+  widget()->DidNavigate();
   // The initial bounds is empty, so setting it to the same thing should do
   // nothing.
-  viz::LocalSurfaceId local_surface_id;
-  viz::ParentLocalSurfaceIdAllocator local_surface_id_allocator;
   ResizeParams resize_params;
   resize_params.screen_info = ScreenInfo();
   resize_params.new_size = gfx::Size();
@@ -65,8 +64,7 @@ TEST_F(RenderWidgetTest, OnResize) {
   resize_params.browser_controls_shrink_blink_size = false;
   resize_params.is_fullscreen_granted = false;
   resize_params.needs_resize_ack = false;
-  local_surface_id = local_surface_id_allocator.GenerateId();
-  resize_params.local_surface_id = local_surface_id;
+  resize_params.content_source_id = 0u;
   OnResize(resize_params);
   EXPECT_EQ(resize_params.needs_resize_ack, next_paint_is_resize_ack());
 
@@ -77,9 +75,12 @@ TEST_F(RenderWidgetTest, OnResize) {
 
   // Setting the bounds to a "real" rect should send the ack.
   render_thread_->sink().ClearMessages();
+  viz::ParentLocalSurfaceIdAllocator local_surface_id_allocator;
   gfx::Size size(100, 100);
+  resize_params.local_surface_id = local_surface_id_allocator.GenerateId();
   resize_params.new_size = size;
   resize_params.physical_backing_size = size;
+  resize_params.content_source_id = 1u;
   resize_params.needs_resize_ack = true;
   OnResize(resize_params);
   EXPECT_EQ(resize_params.needs_resize_ack, next_paint_is_resize_ack());
@@ -96,6 +97,7 @@ TEST_F(RenderWidgetTest, OnResize) {
   // Resetting the rect to empty should not send the ack.
   resize_params.new_size = gfx::Size();
   resize_params.physical_backing_size = gfx::Size();
+  resize_params.local_surface_id = base::nullopt;
   OnResize(resize_params);
   EXPECT_EQ(resize_params.needs_resize_ack, next_paint_is_resize_ack());