Avoid destroying webview Surface too early

Suppose a Surface W is referenced by surfaces A, B, and C. There's currently no mechanism to ensure that W will be deleted when all of A, B, and C are. This currently causes problems if a Surface containing an embedded renderer is resized, and then the previous embedded Surface is destroyed.

To fix this, allow each surface to keep track of what child surfaces it references. Then whenever those references change or when a surface is destroyed the SurfaceManager can do a GC to remove surfaces that aren't referenced anymore.

BUG=510826
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

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

8 files changed, 136 insertions, 13 deletions

diff --git a/cc/layers/surface_layer_impl.cc b/cc/layers/surface_layer_impl.cc
index f029335..0cd4f24 100644
--- a/cc/layers/surface_layer_impl.cc
+++ b/cc/layers/surface_layer_impl.cc
@@ -76,6 +76,7 @@ void SurfaceLayerImpl::AppendQuads(RenderPass* render_pass,
   SurfaceDrawQuad* quad =
       render_pass->CreateAndAppendDrawQuad<SurfaceDrawQuad>();
   quad->SetNew(shared_quad_state, quad_rect, visible_quad_rect, surface_id_);
+  render_pass->referenced_surfaces.push_back(surface_id_);
 }
 
 void SurfaceLayerImpl::GetDebugBorderProperties(SkColor* color,
diff --git a/cc/quads/render_pass.h b/cc/quads/render_pass.h
index 8c31d82..14caeba 100644
--- a/cc/quads/render_pass.h
+++ b/cc/quads/render_pass.h
@@ -14,6 +14,7 @@
 #include "cc/base/list_container.h"
 #include "cc/base/scoped_ptr_vector.h"
 #include "cc/quads/render_pass_id.h"
+#include "cc/surfaces/surface_id.h"
 #include "skia/ext/refptr.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/rect_f.h"
@@ -116,6 +117,10 @@ class CC_EXPORT RenderPass {
   QuadList quad_list;
   SharedQuadStateList shared_quad_state_list;
 
+  // This vector contains the complete set of SurfaceIds referenced by
+  // DrawQuads in quad_list.
+  std::vector<SurfaceId> referenced_surfaces;
+
  protected:
   explicit RenderPass(size_t num_layers);
   RenderPass();
diff --git a/cc/quads/render_pass_unittest.cc b/cc/quads/render_pass_unittest.cc
index 65183af..5ab8c31 100644
--- a/cc/quads/render_pass_unittest.cc
+++ b/cc/quads/render_pass_unittest.cc
@@ -30,6 +30,7 @@ struct RenderPassSize {
   gfx::Rect output_rect;
   gfx::Rect damage_rect;
   bool has_transparent_background;
+  std::vector<SurfaceId> referenced_surfaces;
   ScopedPtrVector<CopyOutputRequest> copy_callbacks;
 };
 
@@ -51,6 +52,7 @@ static void CompareRenderPassLists(const RenderPassList& expected_list,
     EXPECT_EQ(expected->shared_quad_state_list.size(),
               actual->shared_quad_state_list.size());
     EXPECT_EQ(expected->quad_list.size(), actual->quad_list.size());
+    EXPECT_EQ(expected->referenced_surfaces, actual->referenced_surfaces);
 
     for (auto exp_iter = expected->quad_list.cbegin(),
               act_iter = actual->quad_list.cbegin();
@@ -104,6 +106,7 @@ TEST(RenderPassTest, CopyShouldBeIdenticalExceptIdAndQuads) {
   EXPECT_EQ(pass->damage_rect, copy->damage_rect);
   EXPECT_EQ(pass->has_transparent_background, copy->has_transparent_background);
   EXPECT_EQ(0u, copy->quad_list.size());
+  EXPECT_EQ(0u, copy->referenced_surfaces.size());
 
   // The copy request should not be copied/duplicated.
   EXPECT_EQ(1u, pass->copy_requests.size());
diff --git a/cc/surfaces/surface.cc b/cc/surfaces/surface.cc
index bb3f282..16987cb 100644
--- a/cc/surfaces/surface.cc
+++ b/cc/surfaces/surface.cc
@@ -21,8 +21,8 @@ static const int kFrameIndexStart = 2;
 Surface::Surface(SurfaceId id, SurfaceFactory* factory)
     : surface_id_(id),
       factory_(factory->AsWeakPtr()),
-      frame_index_(kFrameIndexStart) {
-}
+      frame_index_(kFrameIndexStart),
+      destroyed_(false) {}
 
 Surface::~Surface() {
   ClearCopyRequests();
@@ -60,6 +60,16 @@ void Surface::QueueFrame(scoped_ptr<CompositorFrame> frame,
       !current_frame_->delegated_frame_data->render_pass_list.empty())
     ++frame_index_;
 
+  std::vector<SurfaceId> new_referenced_surfaces;
+  if (current_frame_) {
+    for (auto& render_pass :
+         current_frame_->delegated_frame_data->render_pass_list) {
+      new_referenced_surfaces.insert(new_referenced_surfaces.end(),
+                                     render_pass->referenced_surfaces.begin(),
+                                     render_pass->referenced_surfaces.end());
+    }
+  }
+
   if (previous_frame) {
     ReturnedResourceArray previous_resources;
     TransferableResource::ReturnResources(
@@ -71,10 +81,18 @@ void Surface::QueueFrame(scoped_ptr<CompositorFrame> frame,
     draw_callback_.Run(SurfaceDrawStatus::DRAW_SKIPPED);
   draw_callback_ = callback;
 
-  if (current_frame_) {
+  bool referenced_surfaces_changed =
+      (referenced_surfaces_ != new_referenced_surfaces);
+  referenced_surfaces_ = new_referenced_surfaces;
+  std::vector<uint32_t> satisfies_sequences;
+  if (current_frame_)
+    current_frame_->metadata.satisfies_sequences.swap(satisfies_sequences);
+  if (referenced_surfaces_changed || !satisfies_sequences.empty()) {
+    // Notify the manager that sequences were satisfied either if some new
+    // sequences were satisfied, or if the set of referenced surfaces changed
+    // to force a GC to happen.
     factory_->manager()->DidSatisfySequences(
-        SurfaceIdAllocator::NamespaceForId(surface_id_),
-        &current_frame_->metadata.satisfies_sequences);
+        SurfaceIdAllocator::NamespaceForId(surface_id_), &satisfies_sequences);
   }
 }
 
diff --git a/cc/surfaces/surface.h b/cc/surfaces/surface.h
index 73dade0..d9099f3 100644
--- a/cc/surfaces/surface.h
+++ b/cc/surfaces/surface.h
@@ -73,6 +73,13 @@ class CC_SURFACES_EXPORT Surface {
     return destruction_dependencies_.size();
   }
 
+  const std::vector<SurfaceId>& referenced_surfaces() const {
+    return referenced_surfaces_;
+  }
+
+  bool destroyed() const { return destroyed_; }
+  void set_destroyed(bool destroyed) { destroyed_ = destroyed; }
+
  private:
   void ClearCopyRequests();
 
@@ -81,8 +88,11 @@ class CC_SURFACES_EXPORT Surface {
   // TODO(jamesr): Support multiple frames in flight.
   scoped_ptr<CompositorFrame> current_frame_;
   int frame_index_;
+  bool destroyed_;
   std::vector<SurfaceSequence> destruction_dependencies_;
 
+  std::vector<SurfaceId> referenced_surfaces_;
+
   DrawCallback draw_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Surface);
diff --git a/cc/surfaces/surface_factory_unittest.cc b/cc/surfaces/surface_factory_unittest.cc
index 538dba9..c2951ae 100644
--- a/cc/surfaces/surface_factory_unittest.cc
+++ b/cc/surfaces/surface_factory_unittest.cc
@@ -458,5 +458,55 @@ TEST_F(SurfaceFactoryTest, InvalidIdNamespace) {
   EXPECT_FALSE(manager_.GetSurfaceForId(id));
 }
 
+TEST_F(SurfaceFactoryTest, DestroyCycle) {
+  SurfaceId id2(5);
+  factory_.Create(id2);
+
+  manager_.RegisterSurfaceIdNamespace(0);
+
+  manager_.GetSurfaceForId(id2)
+      ->AddDestructionDependency(SurfaceSequence(0, 4));
+
+  // Give id2 a frame that references surface_id_.
+  {
+    scoped_ptr<RenderPass> render_pass(RenderPass::Create());
+    render_pass->referenced_surfaces.push_back(surface_id_);
+    scoped_ptr<DelegatedFrameData> frame_data(new DelegatedFrameData);
+    frame_data->render_pass_list.push_back(render_pass.Pass());
+    scoped_ptr<CompositorFrame> frame(new CompositorFrame);
+    frame->delegated_frame_data = frame_data.Pass();
+    factory_.SubmitFrame(id2, frame.Pass(), SurfaceFactory::DrawCallback());
+  }
+  factory_.Destroy(id2);
+
+  // Give surface_id_ a frame that references id2.
+  {
+    scoped_ptr<RenderPass> render_pass(RenderPass::Create());
+    render_pass->referenced_surfaces.push_back(id2);
+    scoped_ptr<DelegatedFrameData> frame_data(new DelegatedFrameData);
+    frame_data->render_pass_list.push_back(render_pass.Pass());
+    scoped_ptr<CompositorFrame> frame(new CompositorFrame);
+    frame->delegated_frame_data = frame_data.Pass();
+    factory_.SubmitFrame(surface_id_, frame.Pass(),
+                         SurfaceFactory::DrawCallback());
+  }
+  factory_.Destroy(surface_id_);
+  EXPECT_TRUE(manager_.GetSurfaceForId(id2));
+  // surface_id_ should be retained by reference from id2.
+  EXPECT_TRUE(manager_.GetSurfaceForId(surface_id_));
+
+  // Satisfy last destruction dependency for id2.
+  std::vector<uint32_t> to_satisfy;
+  to_satisfy.push_back(4);
+  manager_.DidSatisfySequences(0, &to_satisfy);
+
+  // id2 and surface_id_ are in a reference cycle that has no surface
+  // sequences holding on to it, so they should be destroyed.
+  EXPECT_TRUE(!manager_.GetSurfaceForId(id2));
+  EXPECT_TRUE(!manager_.GetSurfaceForId(surface_id_));
+
+  surface_id_ = SurfaceId();
+}
+
 }  // namespace
 }  // namespace cc
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index 52e67d58..9ced46e 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -40,8 +40,9 @@ void SurfaceManager::DeregisterSurface(SurfaceId surface_id) {
 
 void SurfaceManager::Destroy(scoped_ptr<Surface> surface) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  surface->set_destroyed(true);
   surfaces_to_destroy_.push_back(surface.release());
-  SearchForSatisfaction();
+  GarbageCollectSurfaces();
 }
 
 void SurfaceManager::DidSatisfySequences(uint32_t id_namespace,
@@ -53,7 +54,7 @@ void SurfaceManager::DidSatisfySequences(uint32_t id_namespace,
     satisfied_sequences_.insert(SurfaceSequence(id_namespace, *it));
   }
   sequence->clear();
-  SearchForSatisfaction();
+  GarbageCollectSurfaces();
 }
 
 void SurfaceManager::RegisterSurfaceIdNamespace(uint32_t id_namespace) {
@@ -63,15 +64,50 @@ void SurfaceManager::RegisterSurfaceIdNamespace(uint32_t id_namespace) {
 
 void SurfaceManager::InvalidateSurfaceIdNamespace(uint32_t id_namespace) {
   valid_surface_id_namespaces_.erase(id_namespace);
-  SearchForSatisfaction();
+  GarbageCollectSurfaces();
 }
 
-void SurfaceManager::SearchForSatisfaction() {
+void SurfaceManager::GarbageCollectSurfaces() {
+  // Simple mark and sweep GC.
+  // TODO(jbauman): Reduce the amount of work when nothing needs to be
+  // destroyed.
+  std::vector<SurfaceId> live_surfaces;
+  std::set<SurfaceId> live_surfaces_set;
+
+  // GC roots are surfaces that have not been destroyed, or have not had all
+  // their destruction dependencies satisfied.
+  for (auto& map_entry : surface_map_) {
+    map_entry.second->SatisfyDestructionDependencies(
+        &satisfied_sequences_, &valid_surface_id_namespaces_);
+    if (!map_entry.second->destroyed() ||
+        map_entry.second->GetDestructionDependencyCount()) {
+      live_surfaces_set.insert(map_entry.first);
+      live_surfaces.push_back(map_entry.first);
+    }
+  }
+
+  // Mark all surfaces reachable from live surfaces by adding them to
+  // live_surfaces and live_surfaces_set.
+  for (size_t i = 0; i < live_surfaces.size(); i++) {
+    Surface* surf = surface_map_[live_surfaces[i]];
+    DCHECK(surf);
+
+    for (SurfaceId id : surf->referenced_surfaces()) {
+      if (live_surfaces_set.count(id))
+        continue;
+
+      Surface* surf2 = GetSurfaceForId(id);
+      if (surf2) {
+        live_surfaces.push_back(id);
+        live_surfaces_set.insert(id);
+      }
+    }
+  }
+
+  // Destroy all remaining unreachable surfaces.
   for (SurfaceDestroyList::iterator dest_it = surfaces_to_destroy_.begin();
        dest_it != surfaces_to_destroy_.end();) {
-    (*dest_it)->SatisfyDestructionDependencies(&satisfied_sequences_,
-                                               &valid_surface_id_namespaces_);
-    if (!(*dest_it)->GetDestructionDependencyCount()) {
+    if (!live_surfaces_set.count((*dest_it)->surface_id())) {
       scoped_ptr<Surface> surf(*dest_it);
       DeregisterSurface(surf->surface_id());
       dest_it = surfaces_to_destroy_.erase(dest_it);
diff --git a/cc/surfaces/surface_manager.h b/cc/surfaces/surface_manager.h
index aaaa444..8430ea6 100644
--- a/cc/surfaces/surface_manager.h
+++ b/cc/surfaces/surface_manager.h
@@ -56,7 +56,7 @@ class CC_SURFACES_EXPORT SurfaceManager {
   void InvalidateSurfaceIdNamespace(uint32_t id_namespace);
 
  private:
-  void SearchForSatisfaction();
+  void GarbageCollectSurfaces();
 
   typedef base::hash_map<SurfaceId, Surface*> SurfaceMap;
   SurfaceMap surface_map_;