exo::CompositorFrameSinkHolder's release callbacks hold ref

components/exo/compositor_frame_sink_holder.cc

Index: components/exo/compositor_frame_sink_holder.cc
diff --git a/components/exo/compositor_frame_sink_holder.cc b/components/exo/compositor_frame_sink_holder.cc
index dd82aff4dc1dc915afc79434ccbb091dc0c6d476..aef99e6f759de4d909e2fb891bed1f0bd22a07b2 100644
--- a/components/exo/compositor_frame_sink_holder.cc
+++ b/components/exo/compositor_frame_sink_holder.cc
@@ -9,6 +9,14 @@
 
 namespace exo {
 
+namespace {
+void RunSingleReleaseCallback(cc::SingleReleaseCallback* callback,

[reveman, 2016/12/20 16:34:28]

Can you bind cc::SingleReleaseCallback::Run directly instead of creating this
wrapper function?

[Alex Z., 2016/12/20 17:54:01]

This wrapper is now needed for holding a reference to CompositorFrameSinkHolder.

+                              const gpu::SyncToken& sync_token,
+                              bool is_lost) {
+  callback->Run(sync_token, is_lost);
+}
+}  // namespace
+
 ////////////////////////////////////////////////////////////////////////////////
 // CompositorFrameSinkHolder, public:
 
@@ -80,13 +88,16 @@ void CompositorFrameSinkHolder::OnBeginFrame(const cc::BeginFrameArgs& args) {
 
 void CompositorFrameSinkHolder::ReclaimResources(
     const cc::ReturnedResourceArray& resources) {
+  scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder;

[reveman, 2016/12/20 16:34:28]

This shouldn't be needed anymore, right?

[Alex Z., 2016/12/20 17:54:01]

CompositorFrameSinkHolder still gets destroyed too early without it.
It seems that there has to be something holding a reference to
CompositorFrameSinkHolder other than the pairs that are getting erased from the
map.
I now let RunSingleReleaseCallback() hold a reference to
CompositorFrameSinkHolder.

   for (auto& resource : resources) {
     auto it = release_callbacks_.find(resource.id);
     DCHECK(it != release_callbacks_.end());
-    std::unique_ptr<cc::SingleReleaseCallback> callback =
-        std::move(it->second.second);
+    compositor_frame_sink_holder = std::move(it->second.first);
+    auto closure =

[reveman, 2016/12/20 16:34:28]

nit: no need for this temporary variable. pass the base::Bind return value
directly to PostTask.

[Alex Z., 2016/12/20 17:54:01]

Done.

+        base::Bind(&RunSingleReleaseCallback, it->second.second.get(),

[reveman, 2016/12/20 16:34:28]

it->second.second.get() shouldn't work. The callback needs to take ownership of
the value for this to work properly. Otherwise it will be deleted below and
callback will run with an invalid pointer value. You can use base::Owned here I
think.

+                   resource.sync_token, resource.lost);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure);

[reveman, 2016/12/20 16:34:28]

What if the message loop is shutdown before this gets a chance to run? I think
we might need a ScopedClosureRunner here to protect against that.

[Alex Z., 2016/12/20 17:54:01]

It seems no matter how we invoke the Callback::Run(), the actual run is
scheduled asynchronously.

As far as the crashing unit tests are concerned, I think our problem is that
calling erase() with |it| being the last pair in |release_callbacks_| destroys
CompositorFrameSinkHolder before ReclaimResources() returns, rather than the
callback doesn't run in time. To solve this, something other than the pairs
|release_callbacks_| has to hold onto references to CompositorFrameSinkHolder at
least until ReclaimResources() returns. Either having a local scoped_refptr or
letting RunSingleReleaseCallbck() hold a reference solves the issue.

     release_callbacks_.erase(it);
-    callback->Run(resource.sync_token, resource.lost);
   }
 }