Moving temporary reference logic 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 c4dcc5b46cf5eb58a3ee195ddbba829760429155..c9f12578b85e50f9fc879c90476f9c731104e046 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -15,6 +15,7 @@
 #include "cc/surfaces/surface.h"
 #include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_id_allocator.h"
+#include "cc/surfaces/surface_info.h"
 
 namespace cc {
 
@@ -41,6 +42,19 @@ SurfaceManager::SurfaceManager(LifetimeType lifetime_type)
 
 SurfaceManager::~SurfaceManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (lifetime_type_ == LifetimeType::REFERENCES) {
+    // Remove all temporary references on shutdown.
+    for (const auto& map_entry : temp_references_) {
+      const FrameSinkId& frame_sink_id = map_entry.first;
+      for (const auto& local_frame_id : map_entry.second) {
+        RemoveSurfaceReferenceImpl(GetRootSurfaceId(),
+                                   SurfaceId(frame_sink_id, local_frame_id));
+      }
+    }
+    GarbageCollectSurfaces();
+  }
+
   for (SurfaceDestroyList::iterator it = surfaces_to_destroy_.begin();
        it != surfaces_to_destroy_.end();
        ++it) {
@@ -115,18 +129,68 @@ void SurfaceManager::AddSurfaceReference(const SurfaceId& parent_id,
   // Check some conditions that should never happen. We don't want to crash on
   // bad input from a compromised client so just return early.
   if (parent_id == child_id) {
-    LOG(ERROR) << "Cannot add self reference for " << parent_id.ToString();
+    DLOG(ERROR) << "Cannot add self reference for " << parent_id.ToString();
     return;
   }
   if (parent_id != root_surface_id_ && surface_map_.count(parent_id) == 0) {
-    LOG(ERROR) << "No surface in map for " << parent_id.ToString();
+    DLOG(ERROR) << "No surface in map for " << parent_id.ToString();
     return;
   }
   if (surface_map_.count(child_id) == 0) {
-    LOG(ERROR) << "No surface in map for " << child_id.ToString();
+    DLOG(ERROR) << "No surface in map for " << child_id.ToString();
+    return;
+  }
+
+  // There could be a temporary reference to |child_id| which we should now
+  // remove because a real reference is being added to it. To find out whether
+  // or not a temporary reference exists, we need to first look up the
+  // FrameSinkId of |child_id| in |temp_references_|, which returns a vector of
+  // LocalFrameIds, and then search for the LocalFrameId of |child_id| in the
+  // said vector. If there is no temporary reference, we can immediately add the
+  // reference from |parent_id| and return.
+  auto refs_iter = temp_references_.find(child_id.frame_sink_id());
+  if (refs_iter == temp_references_.end()) {
+    AddSurfaceReferenceImpl(parent_id, child_id);
+    return;
+  }
+  std::vector<LocalFrameId>& refs = refs_iter->second;
+  auto temp_ref_iter =
+      std::find(refs.begin(), refs.end(), child_id.local_frame_id());
+  if (temp_ref_iter == refs.end()) {
+    AddSurfaceReferenceImpl(parent_id, child_id);
     return;
   }
 
+  // Temporary references are implemented by holding a reference from the top
+  // level root to the child. If |parent_id| is the top level root, we do
+  // nothing because the reference already exists.

[danakj, 2017/01/18 19:53:11]

nit: don't linebreak after a period

[Saman Sami, 2017/01/18 19:55:43]

Done.

+  // Otherwise remove the temporary reference and add the reference.
+  if (parent_id != GetRootSurfaceId()) {
+    AddSurfaceReferenceImpl(parent_id, child_id);
+    RemoveSurfaceReference(GetRootSurfaceId(), child_id);
+  }
+
+  // Remove temporary references for surfaces with the same FrameSinkId that
+  // were created before |child_id|. The earlier surfaces were never embedded in
+  // the parent and the parent is embedding a later surface, so we know the
+  // parent doesn't need them anymore.
+  for (auto iter = refs.begin(); iter != temp_ref_iter; ++iter) {
+    SurfaceId id = SurfaceId(child_id.frame_sink_id(), *iter);
+    RemoveSurfaceReference(GetRootSurfaceId(), id);
+  }
+
+  // Remove markers for temporary references up to |child_id|, as the temporary
+  // references they correspond to were removed above. If |temp_ref_iter| points
+  // at the last element in |refs| then we are removing all temporary references
+  // for the FrameSinkId and can remove the map entry entirely.
+  if (++temp_ref_iter == refs.end())
+    temp_references_.erase(child_id.frame_sink_id());
+  else
+    refs.erase(refs.begin(), temp_ref_iter);
+}
+
+void SurfaceManager::AddSurfaceReferenceImpl(const SurfaceId& parent_id,
+                                             const SurfaceId& child_id) {
   parent_to_child_refs_[parent_id].insert(child_id);
   child_to_parent_refs_[child_id].insert(parent_id);
 }
@@ -139,8 +203,8 @@ void SurfaceManager::RemoveSurfaceReference(const SurfaceId& parent_id,
   // want to crash on bad input from a compromised client so just return early.
   if (parent_to_child_refs_.count(parent_id) == 0 ||
       parent_to_child_refs_[parent_id].count(child_id) == 0) {
-    LOG(ERROR) << "No reference from " << parent_id.ToString() << " to "
-               << child_id.ToString();
+    DLOG(ERROR) << "No reference from " << parent_id.ToString() << " to "
+                << child_id.ToString();
     return;
   }
 
@@ -504,6 +568,19 @@ bool SurfaceManager::SurfaceModified(const SurfaceId& surface_id) {
 
 void SurfaceManager::SurfaceCreated(const SurfaceInfo& surface_info) {
   CHECK(thread_checker_.CalledOnValidThread());
+
+  if (lifetime_type_ == LifetimeType::REFERENCES) {
+    // We can get into a situation where multiple CompositorFrames arrive for a
+    // CompositorFrameSink before the client can add any references for the
+    // frame. When the second frame with a new size arrives, the first will be
+    // destroyed in SurfaceFactory and then if there are no references it will
+    // be deleted during surface GC. A temporary reference, removed when a real
+    // reference is received, is added to prevent this from happening.
+    AddSurfaceReferenceImpl(GetRootSurfaceId(), surface_info.id());
+    temp_references_[surface_info.id().frame_sink_id()].push_back(
+        surface_info.id().local_frame_id());
+  }
+
   for (auto& observer : observer_list_)
     observer.OnSurfaceCreated(surface_info);
 }