cc: Fix missing render surfaces with output requests

The main thread calculates needed render surfaces, but doesn't have all
of the information that it needs.  In particular, it may not know that
one of its twin layers on the pending or active tree still has an
unserviced copy output request that will live through the commit and
activation, and thus that twin will still need a render surface.

As render surface calculations will probably move to the compositor
thread, rather than adding in complicated tracking and counting of
outstanding copy requests to Layer and request callbacks, instead
just add some logic to check if there are remaining copy output requests
during commit and activation and take that into consideration when
setting the output surface.

This also reverts commit 1e447ad6,
which was clearly legit as it caught this mistake.

BUG=457217

Review URL: https://codereview.chromium.org/935193002

Cr-Commit-Position: refs/heads/master@{#317107}

cc/layers/layer.cc

@@ -909,7 +909,7 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
       draw_checkerboard_for_missing_tiles_);
   layer->SetDrawsContent(DrawsContent());
   layer->SetHideLayerAndSubtree(hide_layer_and_subtree_);
-  layer->SetHasRenderSurface(has_render_surface_);
+  layer->SetHasRenderSurface(has_render_surface_ || layer->HasCopyRequest());
   if (!layer->FilterIsAnimatingOnImplOnly() && !FilterIsAnimating())
     layer->SetFilters(filters_);
   DCHECK(!(FilterIsAnimating() && layer->FilterIsAnimatingOnImplOnly()));

cc/layers/layer_impl.cc

@@ -517,7 +517,7 @@ void LayerImpl::PushPropertiesTo(LayerImpl* layer) {
       draw_checkerboard_for_missing_tiles_);
   layer->SetDrawsContent(DrawsContent());
   layer->SetHideLayerAndSubtree(hide_layer_and_subtree_);
-  layer->SetHasRenderSurface(!!render_surface());
+  layer->SetHasRenderSurface(!!render_surface() || layer->HasCopyRequest());
   layer->SetFilters(filters());
   layer->SetBackgroundFilters(background_filters());
   layer->SetMasksToBounds(masks_to_bounds_);

cc/trees/layer_tree_host_common.cc

@@ -1203,12 +1203,30 @@ struct PreCalculateMetaInformationRecursiveData {
   }
 };
 
+static bool ValidateRenderSurface(LayerImpl* layer) {
+  // This test verifies that there are no cases where a LayerImpl needs
+  // a render surface, but doesn't have one.
+  if (layer->render_surface())
+    return true;
+
+  return layer->filters().IsEmpty() && layer->background_filters().IsEmpty() &&
+         !layer->mask_layer() && !layer->replica_layer() &&
+         !IsRootLayer(layer) && !layer->is_root_for_isolated_group() &&
+         !layer->HasCopyRequest();
+}
+
+static bool ValidateRenderSurface(Layer* layer) {
+  return true;
+}
+
 // Recursively walks the layer tree to compute any information that is needed
 // before doing the main recursion.
 template <typename LayerType>
 static void PreCalculateMetaInformation(
     LayerType* layer,
     PreCalculateMetaInformationRecursiveData* recursive_data) {
+  DCHECK(ValidateRenderSurface(layer));
+
   layer->draw_properties().sorted_for_recursion = false;
   layer->draw_properties().has_child_with_a_scroll_parent = false;
 

cc/trees/layer_tree_host_unittest.cc

@@ -6285,4 +6285,83 @@ class LayerTreeHostTestNoTasksBetweenWillAndDidCommit
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestNoTasksBetweenWillAndDidCommit);
 
+// Verify that if a LayerImpl holds onto a copy request for multiple
+// frames that it will continue to have a render surface through
+// multiple commits, even though the Layer itself has no reason
+// to have a render surface.
+class LayerPreserveRenderSurfaceFromOutputRequests : public LayerTreeHostTest {
+ protected:
+  void SetupTree() override {
+    scoped_refptr<Layer> root = Layer::Create();
+    root->CreateRenderSurface();
+    root->SetBounds(gfx::Size(10, 10));
+    child_ = Layer::Create();
+    child_->SetBounds(gfx::Size(20, 20));
+    root->AddChild(child_);
+
+    layer_tree_host()->SetRootLayer(root);
+    LayerTreeHostTest::SetupTree();
+  }
+
+  static void CopyOutputCallback(scoped_ptr<CopyOutputResult> result) {}
+
+  void BeginTest() override {
+    child_->RequestCopyOfOutput(
+        CopyOutputRequest::CreateBitmapRequest(base::Bind(CopyOutputCallback)));
+    EXPECT_TRUE(child_->HasCopyRequest());
+    PostSetNeedsCommitToMainThread();
+  }
+
+  void DidCommit() override { EXPECT_FALSE(child_->HasCopyRequest()); }
+
+  void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override {
+    LayerImpl* child_impl = host_impl->sync_tree()->LayerById(child_->id());
+
+    switch (host_impl->sync_tree()->source_frame_number()) {
+      case 0:
+        EXPECT_TRUE(child_impl->HasCopyRequest());
+        EXPECT_TRUE(child_impl->render_surface());
+        break;
+      case 1:
+        if (host_impl->proxy()->CommitToActiveTree()) {
+          EXPECT_TRUE(child_impl->HasCopyRequest());
+          EXPECT_TRUE(child_impl->render_surface());
+        } else {
+          EXPECT_FALSE(child_impl->HasCopyRequest());
+          EXPECT_FALSE(child_impl->render_surface());
+        }
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+  }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    LayerImpl* child_impl = host_impl->active_tree()->LayerById(child_->id());
+    EXPECT_TRUE(child_impl->HasCopyRequest());
+    EXPECT_TRUE(child_impl->render_surface());
+
+    switch (host_impl->active_tree()->source_frame_number()) {
+      case 0:
+        // Lose output surface to prevent drawing and cause another commit.
+        host_impl->DidLoseOutputSurface();
+        break;
+      case 1:
+        EndTest();
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+  }
+
+  void AfterTest() override {}
+
+ private:
+  scoped_refptr<Layer> child_;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(LayerPreserveRenderSurfaceFromOutputRequests);
+
 }  // namespace cc

cc/trees/thread_proxy.cc

@@ -154,9 +154,9 @@ bool ThreadProxy::IsStarted() const {
 }
 
 bool ThreadProxy::CommitToActiveTree() const {
-  // With ThreadProxy we use a pending tree and activate it once it's ready to
-  // draw.
-  return false;
+  // With ThreadProxy and impl-side painting, we use a pending tree and activate
+  // it once it's ready to draw.
+  return !impl().layer_tree_host_impl->settings().impl_side_painting;
 }
 
 void ThreadProxy::SetLayerTreeHostClientReady() {