cc: Move ownership of surfaces to SurfaceManager

cc/surfaces/surface_manager.cc

Index: cc/surfaces/surface_manager.cc
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index 2b35c7463a963456a17d1d95dc350bc658ef5faa..5b3a11b88191de3059b09796d19c202074d329d0 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -38,22 +38,7 @@ SurfaceManager::SurfaceManager(LifetimeType lifetime_type)
       new DirectSurfaceReferenceFactory(weak_factory_.GetWeakPtr());
 }
 
-SurfaceManager::~SurfaceManager() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  // Remove all temporary references on shutdown.
-  temporary_references_.clear();
-  temporary_reference_ranges_.clear();
-
-  GarbageCollectSurfaces();
-
-  for (SurfaceDestroyList::iterator it = surfaces_to_destroy_.begin();
-       it != surfaces_to_destroy_.end();
-       ++it) {
-    UnregisterSurface((*it)->surface_id());
-  }
-  surfaces_to_destroy_.clear();
-}
+SurfaceManager::~SurfaceManager() = default;

[danakj, 2017/06/27 18:21:32]

should we dcheck that all surfaces are destroyed by now? how are we guarding
lifetimes? should this return some RAII object when CreateSurface that you have
to hand back with DeleteSurface, and it DCHECKs if its destroyed by anyone other
than SurfaceManager? Or how can we feel confident that we're destroying what we
create and not leaking surfaces?

[Saman Sami, 2017/06/28 14:19:43]

All Surfaces are owned by SurfaceManager. When it's destroyed, all Surfaces are
destroyed. There is no leak.

[danakj, 2017/06/28 15:59:45]

What about during the lifetime of the browser? I didn't mean at shutdown (tho we
could notice them for sure at shutdown)

[Saman Sami, 2017/06/28 18:01:00]

We can check if all the remaining surfaces in the surface map are marked for
destruction.

[danakj, 2017/06/28 18:16:24]

Ok thanks. That's better than nothing, though wouldn't catch anything until
shutdown at which point debugging where the leak came from would be very tricky.

 
 #if DCHECK_IS_ON()
 std::string SurfaceManager::SurfaceReferencesToString() {
@@ -77,7 +62,7 @@ void SurfaceManager::RequestSurfaceResolution(Surface* pending_surface) {
     dependency_tracker_->RequestSurfaceResolution(pending_surface);
 }
 
-std::unique_ptr<Surface> SurfaceManager::CreateSurface(
+Surface* SurfaceManager::CreateSurface(
     base::WeakPtr<CompositorFrameSinkSupport> compositor_frame_sink_support,
     const SurfaceInfo& surface_info) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -88,42 +73,32 @@ std::unique_ptr<Surface> SurfaceManager::CreateSurface(
 
   // If no surface with this SurfaceId exists, simply create the surface and
   // return.
-  auto surface_iter = surface_map_.find(surface_info.id());
-  if (surface_iter == surface_map_.end()) {
-    auto surface =
+  auto it = surface_map_.find(surface_info.id());
+  if (it == surface_map_.end()) {
+    surface_map_[surface_info.id()] =
         base::MakeUnique<Surface>(surface_info, compositor_frame_sink_support);
-    surface_map_[surface->surface_id()] = surface.get();
-    return surface;
+    return surface_map_[surface_info.id()].get();
   }
 
-  // If a surface with this SurfaceId exists and it's not marked as destroyed,
-  // we should not receive a request to create a new surface with the same
-  // SurfaceId.
-  DCHECK(surface_iter->second->destroyed());
-
-  // If a surface with this SurfaceId exists and it's marked as destroyed,
-  // it means it's in the garbage collector's queue. We simply take it out of
-  // the queue and reuse it.
-  auto it =
-      std::find_if(surfaces_to_destroy_.begin(), surfaces_to_destroy_.end(),
-                   [&surface_info](const std::unique_ptr<Surface>& surface) {
-                     return surface->surface_id() == surface_info.id();
-                   });
-  DCHECK(it != surfaces_to_destroy_.end());
-  std::unique_ptr<Surface> surface = std::move(*it);
-  surfaces_to_destroy_.erase(it);
-  surface->set_destroyed(false);
+  // If a surface with this SurfaceId exists, it must be marked as destroyed.
+  // Otherwise, we wouldn't receive a request to reuse the same SurfaceId.
+  // Remove the surface out of the garbage collector's queue and reuse it.
+  Surface* surface = it->second.get();
+  DCHECK(IsMarkedForDestruction(surface_info.id()));
+  surfaces_to_destroy_.erase(surface_info.id());
   DCHECK_EQ(compositor_frame_sink_support.get(),
             surface->compositor_frame_sink_support().get());
   return surface;
 }
 
-void SurfaceManager::DestroySurface(std::unique_ptr<Surface> surface) {
+void SurfaceManager::DestroySurface(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  surface->set_destroyed(true);
+  DCHECK(surface_map_.count(surface_id));
+  if (IsMarkedForDestruction(surface_id))

[danakj, 2017/06/27 18:21:32]

We want to be able to destroy a surface twice?

[Saman Sami, 2017/06/28 14:19:43]

I think I can get rid of this now. Earlier patch sets needed this.

+    return;
   for (auto& observer : observer_list_)
-    observer.OnSurfaceDestroyed(surface->surface_id());
-  surfaces_to_destroy_.push_back(std::move(surface));
+    observer.OnSurfaceDestroyed(surface_id);
+  surfaces_to_destroy_.insert(surface_id);
   GarbageCollectSurfaces();
 }
 
@@ -238,15 +213,13 @@ void SurfaceManager::GarbageCollectSurfaces() {
                                         ? GetLiveSurfacesForReferences()
                                         : GetLiveSurfacesForSequences();
 
-  std::vector<std::unique_ptr<Surface>> surfaces_to_delete;
+  std::vector<SurfaceId> surfaces_to_delete;
 
   // Delete all destroyed and unreachable surfaces.
   for (auto iter = surfaces_to_destroy_.begin();
        iter != surfaces_to_destroy_.end();) {
-    SurfaceId surface_id = (*iter)->surface_id();
-    if (reachable_surfaces.count(surface_id) == 0) {
-      UnregisterSurface(surface_id);
-      surfaces_to_delete.push_back(std::move(*iter));
+    if (reachable_surfaces.count(*iter) == 0) {
+      surfaces_to_delete.push_back(*iter);
       iter = surfaces_to_destroy_.erase(iter);
     } else {
       ++iter;
@@ -254,7 +227,8 @@ void SurfaceManager::GarbageCollectSurfaces() {
   }
 
   // ~Surface() draw callback could modify |surfaces_to_destroy_|.
-  surfaces_to_delete.clear();
+  for (const SurfaceId& surface_id : surfaces_to_delete)
+    DestroySurfaceInternal(surface_id);
 }
 
 SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForReferences() {
@@ -298,11 +272,12 @@ SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForSequences() {
   // their destruction dependencies satisfied.
   for (auto& map_entry : surface_map_) {
     const SurfaceId& surface_id = map_entry.first;
-    Surface* surface = map_entry.second;
+    Surface* surface = map_entry.second.get();
     surface->SatisfyDestructionDependencies(&satisfied_sequences_,
                                   framesink_manager_.GetValidFrameSinkIds());
 
-    if (!surface->destroyed() || surface->GetDestructionDependencyCount() > 0) {
+    if (!IsMarkedForDestruction(surface_id) ||
+        surface->GetDestructionDependencyCount() > 0) {
       live_surfaces_set.insert(surface_id);
       live_surfaces.push_back(surface_id);
     }
@@ -311,7 +286,7 @@ SurfaceManager::SurfaceIdSet SurfaceManager::GetLiveSurfacesForSequences() {
   // 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]];
+    Surface* surf = surface_map_[live_surfaces[i]].get();
     DCHECK(surf);
 
     const auto& children = GetSurfacesReferencedByParent(surf->surface_id());
@@ -468,7 +443,7 @@ Surface* SurfaceManager::GetSurfaceForId(const SurfaceId& surface_id) {
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   if (it == surface_map_.end())
     return nullptr;
-  return it->second;
+  return it->second.get();
 }
 
 bool SurfaceManager::SurfaceModified(const SurfaceId& surface_id,
@@ -526,10 +501,11 @@ void SurfaceManager::SurfaceDamageExpected(const SurfaceId& surface_id,
     observer.OnSurfaceDamageExpected(surface_id, args);
 }
 
-void SurfaceManager::UnregisterSurface(const SurfaceId& surface_id) {
+void SurfaceManager::DestroySurfaceInternal(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   DCHECK(it != surface_map_.end());
+  std::unique_ptr<Surface> doomed = std::move(it->second);

[danakj, 2017/06/27 18:21:31]

do you mean to keep this |doomed| alive until the end of the function? erase()
would delete it too, cuz the unique_ptr will.

[Saman Sami, 2017/06/28 14:19:44]

Yes, I want the surface to be removed from the map before its destructor is run
and an ack is sent.

[danakj, 2017/06/28 15:59:45]

Can you explain this in a comment? Is there a test that verifies this?

[Saman Sami, 2017/06/28 18:01:00]

Sure. CompositorFrameSinkSupportTest_AddDuringEviction should cover this.

   surface_map_.erase(it);
   RemoveAllSurfaceReferences(surface_id);
 }
@@ -544,7 +520,7 @@ void SurfaceManager::SurfaceReferencesToStringImpl(const SurfaceId& surface_id,
   Surface* surface = GetSurfaceForId(surface_id);
   if (surface) {
     *str << surface->surface_id().ToString();
-    *str << (surface->destroyed() ? " destroyed" : " live");
+    *str << (IsMarkedForDestruction(surface_id) ? " destroyed" : " live");
 
     if (surface->HasPendingFrame()) {
       // This provides the surface size from the root render pass.
@@ -571,4 +547,8 @@ void SurfaceManager::SurfaceReferencesToStringImpl(const SurfaceId& surface_id,
 }
 #endif  // DCHECK_IS_ON()
 
+bool SurfaceManager::IsMarkedForDestruction(const SurfaceId& surface_id) {
+  return surfaces_to_destroy_.count(surface_id) != 0;
+}
+
 }  // namespace cc