cc: Move ownership of surfaces to SurfaceManager

cc/surfaces/surface_manager.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/surfaces/surface_manager.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 20 matching lines @@
 SurfaceManager::SurfaceManager(LifetimeType lifetime_type)
     : lifetime_type_(lifetime_type),
       root_surface_id_(FrameSinkId(0u, 0u),
                        LocalSurfaceId(1u, base::UnguessableToken::Create())),
       weak_factory_(this) {
   thread_checker_.DetachFromThread();
   reference_factory_ =
       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() {
   std::stringstream str;
   SurfaceReferencesToStringImpl(root_surface_id_, "", &str);
   // Temporary references will have an asterisk in front of them.
   for (auto& map_entry : temporary_references_)
     SurfaceReferencesToStringImpl(map_entry.first, "* ", &str);
 
   return str.str();
 }
 #endif
 
 void SurfaceManager::SetDependencyTracker(
     SurfaceDependencyTracker* dependency_tracker) {
   dependency_tracker_ = dependency_tracker;
 }
 
 void SurfaceManager::RequestSurfaceResolution(Surface* pending_surface) {
   if (dependency_tracker_)
     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());
   DCHECK(surface_info.is_valid());
   DCHECK(compositor_frame_sink_support);
   DCHECK_EQ(surface_info.id().frame_sink_id(),
             compositor_frame_sink_support->frame_sink_id());
 
   // 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();
 }
 
 void SurfaceManager::SurfaceWillDraw(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (auto& observer : observer_list_)
     observer.OnSurfaceWillDraw(surface_id);
 }
 
 void SurfaceManager::RequireSequence(const SurfaceId& surface_id,
@@ skipping 94 matching lines @@
 }
 
 void SurfaceManager::GarbageCollectSurfaces() {
   if (surfaces_to_destroy_.empty())
     return;
 
   SurfaceIdSet reachable_surfaces = using_surface_references()
                                         ? 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;
     }
   }
 
   // ~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() {
   DCHECK(using_surface_references());
 
   SurfaceIdSet reachable_surfaces;
 
   // Walk down from the root and mark each SurfaceId we encounter as reachable.
   std::queue<SurfaceId> surface_queue;
   surface_queue.push(root_surface_id_);
@@ skipping 23 matching lines @@
   // Simple mark and sweep GC.
   // TODO(jbauman): Reduce the amount of work when nothing needs to be
   // destroyed.
   std::vector<SurfaceId> live_surfaces;
   std::unordered_set<SurfaceId, SurfaceIdHash> 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_) {
     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);
     }
   }
 
   // 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());
     for (const SurfaceId& id : children) {
       if (live_surfaces_set.count(id))
         continue;
 
       Surface* surf2 = GetSurfaceForId(id);
       if (surf2) {
         live_surfaces.push_back(id);
@@ skipping 136 matching lines @@
     const FrameSinkId& child_frame_sink_id) {
   framesink_manager_.UnregisterFrameSinkHierarchy(parent_frame_sink_id,
                                                 child_frame_sink_id);
 }
 
 Surface* SurfaceManager::GetSurfaceForId(const SurfaceId& surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   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,
                                      const BeginFrameAck& ack) {
   CHECK(thread_checker_.CalledOnValidThread());
   bool changed = false;
   for (auto& observer : observer_list_)
     changed |= observer.OnSurfaceDamaged(surface_id, ack);
   return changed;
 }
@@ skipping 37 matching lines @@
     dependency_tracker_->OnSurfaceDiscarded(surface);
 }
 
 void SurfaceManager::SurfaceDamageExpected(const SurfaceId& surface_id,
                                            const BeginFrameArgs& args) {
   DCHECK(thread_checker_.CalledOnValidThread());
   for (auto& observer : observer_list_)
     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);
 }
 
 #if DCHECK_IS_ON()
 void SurfaceManager::SurfaceReferencesToStringImpl(const SurfaceId& surface_id,
                                                    std::string indent,
                                                    std::stringstream* str) {
   *str << indent;
 
   // Print the current line for |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.
       const CompositorFrame& frame = surface->GetPendingFrame();
       *str << " pending "
            << frame.render_pass_list.back()->output_rect.size().ToString();
     }
 
     if (surface->HasActiveFrame()) {
       // This provides the surface size from the root render pass.
       const CompositorFrame& frame = surface->GetActiveFrame();
       *str << " active "
            << frame.render_pass_list.back()->output_rect.size().ToString();
     }
   } else {
     *str << surface_id;
   }
   *str << "\n";
 
   // If the current surface has references to children, sort children and print
   // references for each child.
   for (const SurfaceId& child_id : GetSurfacesReferencedByParent(surface_id))
     SurfaceReferencesToStringImpl(child_id, indent + "  ", str);
 }
 #endif  // DCHECK_IS_ON()
 
+bool SurfaceManager::IsMarkedForDestruction(const SurfaceId& surface_id) {
+  return surfaces_to_destroy_.count(surface_id) != 0;
+}
+
 }  // namespace cc