Avoid destroying surface before the parent surface stops referencing it.

Add surface sequence numbers, which are used to schedule the
destruction of surfaces. The child surface's destruction can
wait on a set of sequence numbers, and the parent surface can
later queue a frame that satisfies those numbers, causing the
former child surface to be destroyed.

Also move ownership of the SurfaceIdAllocator to the
ui::Compositor, so that the surface id namespace for a
compositor will stay the same across all output surfaces it
ever uses.

BUG=411118

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

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

9 files changed, 169 insertions, 7 deletions

diff --git a/cc/surfaces/surface.cc b/cc/surfaces/surface.cc
index d58ed4c..4d32d72 100644
--- a/cc/surfaces/surface.cc
+++ b/cc/surfaces/surface.cc
@@ -7,6 +7,7 @@
 #include "cc/output/compositor_frame.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/surfaces/surface_factory.h"
+#include "cc/surfaces/surface_manager.h"
 
 namespace cc {
 
@@ -17,7 +18,7 @@ static const int kFrameIndexStart = 2;
 Surface::Surface(SurfaceId id, const gfx::Size& size, SurfaceFactory* factory)
     : surface_id_(id),
       size_(size),
-      factory_(factory),
+      factory_(factory->AsWeakPtr()),
       frame_index_(kFrameIndexStart) {
 }
 
@@ -28,7 +29,7 @@ Surface::~Surface() {
     (*it)->SendEmptyResult();
   }
   copy_requests_.clear();
-  if (current_frame_) {
+  if (current_frame_ && factory_) {
     ReturnedResourceArray current_resources;
     TransferableResource::ReturnResources(
         current_frame_->delegated_frame_data->resource_list,
@@ -39,6 +40,7 @@ Surface::~Surface() {
 
 void Surface::QueueFrame(scoped_ptr<CompositorFrame> frame,
                          const base::Closure& callback) {
+  DCHECK(factory_);
   for (ScopedPtrVector<CopyOutputRequest>::iterator it = copy_requests_.begin();
        it != copy_requests_.end();
        ++it) {
@@ -63,6 +65,8 @@ void Surface::QueueFrame(scoped_ptr<CompositorFrame> frame,
   if (!draw_callback_.is_null())
     draw_callback_.Run();
   draw_callback_ = callback;
+  factory_->manager()->DidSatisfySequences(
+      surface_id_, &current_frame_->metadata.satisfies_sequences);
 }
 
 void Surface::RequestCopyOfOutput(scoped_ptr<CopyOutputRequest> copy_request) {
diff --git a/cc/surfaces/surface.h b/cc/surfaces/surface.h
index 718fef4..0df5c17 100644
--- a/cc/surfaces/surface.h
+++ b/cc/surfaces/surface.h
@@ -11,6 +11,7 @@
 #include "base/containers/hash_tables.h"
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "cc/base/scoped_ptr_vector.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/surfaces/surface_id.h"
@@ -50,12 +51,12 @@ class CC_SURFACES_EXPORT Surface {
   void TakeLatencyInfo(std::vector<ui::LatencyInfo>* latency_info);
   void RunDrawCallbacks();
 
-  SurfaceFactory* factory() { return factory_; }
+  base::WeakPtr<SurfaceFactory> factory() { return factory_; }
 
  private:
   SurfaceId surface_id_;
   gfx::Size size_;
-  SurfaceFactory* factory_;
+  base::WeakPtr<SurfaceFactory> factory_;
   // TODO(jamesr): Support multiple frames in flight.
   scoped_ptr<CompositorFrame> current_frame_;
   int frame_index_;
diff --git a/cc/surfaces/surface_factory.cc b/cc/surfaces/surface_factory.cc
index ab7186a..dcf1130 100644
--- a/cc/surfaces/surface_factory.cc
+++ b/cc/surfaces/surface_factory.cc
@@ -29,17 +29,26 @@ void SurfaceFactory::Create(SurfaceId surface_id, const gfx::Size& size) {
 void SurfaceFactory::Destroy(SurfaceId surface_id) {
   OwningSurfaceMap::iterator it = surface_map_.find(surface_id);
   DCHECK(it != surface_map_.end());
-  DCHECK(it->second->factory() == this);
+  DCHECK(it->second->factory().get() == this);
   manager_->DeregisterSurface(surface_id);
   surface_map_.erase(it);
 }
 
+void SurfaceFactory::DestroyOnSequence(
+    SurfaceId surface_id,
+    const std::set<SurfaceSequence>& dependency_set) {
+  OwningSurfaceMap::iterator it = surface_map_.find(surface_id);
+  DCHECK(it != surface_map_.end());
+  DCHECK(it->second->factory().get() == this);
+  manager_->DestroyOnSequence(surface_map_.take_and_erase(it), dependency_set);
+}
+
 void SurfaceFactory::SubmitFrame(SurfaceId surface_id,
                                  scoped_ptr<CompositorFrame> frame,
                                  const base::Closure& callback) {
   OwningSurfaceMap::iterator it = surface_map_.find(surface_id);
   DCHECK(it != surface_map_.end());
-  DCHECK(it->second->factory() == this);
+  DCHECK(it->second->factory().get() == this);
   it->second->QueueFrame(frame.Pass(), callback);
   manager_->SurfaceModified(surface_id);
 }
@@ -52,7 +61,7 @@ void SurfaceFactory::RequestCopyOfSurface(
     copy_request->SendEmptyResult();
     return;
   }
-  DCHECK(it->second->factory() == this);
+  DCHECK(it->second->factory().get() == this);
   it->second->RequestCopyOfOutput(copy_request.Pass());
   manager_->SurfaceModified(surface_id);
 }
diff --git a/cc/surfaces/surface_factory.h b/cc/surfaces/surface_factory.h
index 2516015..3eb4f43 100644
--- a/cc/surfaces/surface_factory.h
+++ b/cc/surfaces/surface_factory.h
@@ -11,6 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "cc/surfaces/surface_id.h"
 #include "cc/surfaces/surface_resource_holder.h"
+#include "cc/surfaces/surface_sequence.h"
 #include "cc/surfaces/surfaces_export.h"
 
 namespace gfx {
@@ -37,6 +38,8 @@ class CC_SURFACES_EXPORT SurfaceFactory
 
   void Create(SurfaceId surface_id, const gfx::Size& size);
   void Destroy(SurfaceId surface_id);
+  void DestroyOnSequence(SurfaceId surface_id,
+                         const std::set<SurfaceSequence>& dependency_set);
   // A frame can only be submitted to a surface created by this factory,
   // although the frame may reference surfaces created by other factories.
   // The callback is called the first time this frame is used to draw.
@@ -52,6 +55,8 @@ class CC_SURFACES_EXPORT SurfaceFactory
   void RefResources(const TransferableResourceArray& resources);
   void UnrefResources(const ReturnedResourceArray& resources);
 
+  SurfaceManager* manager() { return manager_; }
+
  private:
   SurfaceManager* manager_;
   SurfaceFactoryClient* client_;
diff --git a/cc/surfaces/surface_factory_unittest.cc b/cc/surfaces/surface_factory_unittest.cc
index 5cb83fe..ba2a888 100644
--- a/cc/surfaces/surface_factory_unittest.cc
+++ b/cc/surfaces/surface_factory_unittest.cc
@@ -372,5 +372,32 @@ TEST_F(SurfaceFactoryTest, DestroyWithResourceRefs) {
   factory_.SubmitFrame(id, frame.Pass(), base::Closure());
 }
 
+TEST_F(SurfaceFactoryTest, DestroySequence) {
+  SurfaceId id2(5);
+  factory_.Create(id2, gfx::Size(5, 5));
+
+  // Check that waiting before the sequence is satisfied works.
+  std::set<SurfaceSequence> sequence;
+  sequence.insert(SurfaceSequence(0, 4));
+  factory_.DestroyOnSequence(id2, sequence);
+
+  scoped_ptr<DelegatedFrameData> frame_data(new DelegatedFrameData);
+  scoped_ptr<CompositorFrame> frame(new CompositorFrame);
+  frame->metadata.satisfies_sequences.push_back(6);
+  frame->metadata.satisfies_sequences.push_back(4);
+  frame->delegated_frame_data = frame_data.Pass();
+  DCHECK(manager_.GetSurfaceForId(id2));
+  factory_.SubmitFrame(surface_id_, frame.Pass(), base::Closure());
+  DCHECK(!manager_.GetSurfaceForId(id2));
+
+  // Check that waiting after the sequence is satisfied works.
+  factory_.Create(id2, gfx::Size(5, 5));
+  sequence.clear();
+  sequence.insert(SurfaceSequence(0, 6));
+  DCHECK(manager_.GetSurfaceForId(id2));
+  factory_.DestroyOnSequence(id2, sequence);
+  DCHECK(!manager_.GetSurfaceForId(id2));
+}
+
 }  // namespace
 }  // namespace cc
diff --git a/cc/surfaces/surface_id_allocator.h b/cc/surfaces/surface_id_allocator.h
index d410f06..96a241f 100644
--- a/cc/surfaces/surface_id_allocator.h
+++ b/cc/surfaces/surface_id_allocator.h
@@ -21,6 +21,8 @@ class CC_SURFACES_EXPORT SurfaceIdAllocator {
 
   static uint32_t NamespaceForId(SurfaceId id);
 
+  uint32_t id_namespace() const { return id_namespace_; }
+
  private:
   const uint32_t id_namespace_;
   uint32_t next_id_;
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index 1e3699d..920f464 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "cc/surfaces/surface.h"
+#include "cc/surfaces/surface_id_allocator.h"
 
 namespace cc {
 
@@ -15,6 +16,12 @@ SurfaceManager::SurfaceManager() {
 
 SurfaceManager::~SurfaceManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  for (SurfaceDestroyList::iterator it = surfaces_to_destroy_.begin();
+       it != surfaces_to_destroy_.end();
+       ++it) {
+    DeregisterSurface(it->first->surface_id());
+    delete it->first;
+  }
 }
 
 void SurfaceManager::RegisterSurface(Surface* surface) {
@@ -31,6 +38,51 @@ void SurfaceManager::DeregisterSurface(SurfaceId surface_id) {
   surface_map_.erase(it);
 }
 
+void SurfaceManager::DestroyOnSequence(
+    scoped_ptr<Surface> surface,
+    const std::set<SurfaceSequence>& dependency_set) {
+  surfaces_to_destroy_.push_back(make_pair(surface.release(), dependency_set));
+  SearchForSatisfaction();
+}
+
+void SurfaceManager::DidSatisfySequences(SurfaceId id,
+                                         std::vector<uint32_t>* sequence) {
+  for (std::vector<uint32_t>::iterator it = sequence->begin();
+       it != sequence->end();
+       ++it) {
+    satisfied_sequences_.insert(
+        SurfaceSequence(SurfaceIdAllocator::NamespaceForId(id), *it));
+  }
+  sequence->clear();
+  SearchForSatisfaction();
+}
+
+void SurfaceManager::SearchForSatisfaction() {
+  for (SurfaceDestroyList::iterator dest_it = surfaces_to_destroy_.begin();
+       dest_it != surfaces_to_destroy_.end();) {
+    std::set<SurfaceSequence>& dependency_set = dest_it->second;
+
+    for (std::set<SurfaceSequence>::iterator it = dependency_set.begin();
+         it != dependency_set.end();) {
+      if (satisfied_sequences_.count(*it) > 0) {
+        satisfied_sequences_.erase(*it);
+        std::set<SurfaceSequence>::iterator old_it = it;
+        ++it;
+        dependency_set.erase(old_it);
+      } else {
+        ++it;
+      }
+    }
+    if (dependency_set.empty()) {
+      scoped_ptr<Surface> surf(dest_it->first);
+      DeregisterSurface(surf->surface_id());
+      dest_it = surfaces_to_destroy_.erase(dest_it);
+    } else {
+      ++dest_it;
+    }
+  }
+}
+
 Surface* SurfaceManager::GetSurfaceForId(SurfaceId surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
diff --git a/cc/surfaces/surface_manager.h b/cc/surfaces/surface_manager.h
index 7515be5..66db9d9 100644
--- a/cc/surfaces/surface_manager.h
+++ b/cc/surfaces/surface_manager.h
@@ -5,12 +5,17 @@
 #ifndef CC_SURFACES_SURFACE_MANAGER_H_
 #define CC_SURFACES_SURFACE_MANAGER_H_
 
+#include <list>
+#include <set>
+#include <vector>
+
 #include "base/containers/hash_tables.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "cc/surfaces/surface_damage_observer.h"
 #include "cc/surfaces/surface_id.h"
+#include "cc/surfaces/surface_sequence.h"
 #include "cc/surfaces/surfaces_export.h"
 
 namespace cc {
@@ -25,6 +30,10 @@ class CC_SURFACES_EXPORT SurfaceManager {
   void RegisterSurface(Surface* surface);
   void DeregisterSurface(SurfaceId surface_id);
 
+  // Destroy the Surface once a set of sequence numbers has been satisfied.
+  void DestroyOnSequence(scoped_ptr<Surface> surface,
+                         const std::set<SurfaceSequence>& dependency_set);
+
   Surface* GetSurfaceForId(SurfaceId surface_id);
 
   void AddObserver(SurfaceDamageObserver* obs) {
@@ -37,12 +46,27 @@ class CC_SURFACES_EXPORT SurfaceManager {
 
   void SurfaceModified(SurfaceId surface_id);
 
+  // A frame for a surface satisfies a set of sequence numbers.
+  void DidSatisfySequences(SurfaceId id, std::vector<uint32_t>* sequence);
+
  private:
+  void SearchForSatisfaction();
+
   typedef base::hash_map<SurfaceId, Surface*> SurfaceMap;
   SurfaceMap surface_map_;
   ObserverList<SurfaceDamageObserver> observer_list_;
   base::ThreadChecker thread_checker_;
 
+  // List of surfaces to be destroyed, along with what sequences they're still
+  // waiting on.
+  typedef std::list<std::pair<Surface*, std::set<SurfaceSequence>>>
+      SurfaceDestroyList;
+  SurfaceDestroyList surfaces_to_destroy_;
+
+  // Set of SurfaceSequences that have been satisfied by a frame but not yet
+  // waited on.
+  std::set<SurfaceSequence> satisfied_sequences_;
+
   DISALLOW_COPY_AND_ASSIGN(SurfaceManager);
 };
 
diff --git a/cc/surfaces/surface_sequence.h b/cc/surfaces/surface_sequence.h
new file mode 100644
index 0000000..4c99e45
--- /dev/null
+++ b/cc/surfaces/surface_sequence.h
@@ -0,0 +1,38 @@
+// 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.
+
+#ifndef CC_SURFACES_SURFACE_SEQUENCE_H_
+#define CC_SURFACES_SURFACE_SEQUENCE_H_
+
+namespace cc {
+
+// A per-surface-namespace sequence number that's used to coordinate
+// dependencies between frames. A sequence number may be satisfied once, and
+// may be depended on once.
+struct SurfaceSequence {
+  SurfaceSequence() : id_namespace(0u), sequence(0u) {}
+  SurfaceSequence(uint32_t id_namespace, uint32_t sequence)
+      : id_namespace(id_namespace), sequence(sequence) {}
+
+  uint32_t id_namespace;
+  uint32_t sequence;
+};
+
+inline bool operator==(const SurfaceSequence& a, const SurfaceSequence& b) {
+  return a.id_namespace == b.id_namespace && a.sequence == b.sequence;
+}
+
+inline bool operator!=(const SurfaceSequence& a, const SurfaceSequence& b) {
+  return !(a == b);
+}
+
+inline bool operator<(const SurfaceSequence& a, const SurfaceSequence& b) {
+  if (a.id_namespace != b.id_namespace)
+    return a.id_namespace < b.id_namespace;
+  return a.sequence < b.sequence;
+}
+
+}  // namespace cc
+
+#endif  // CC_SURFACES_SURFACE_SEQUENCE_H_