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..aadd943861d80c8da24fb962e8926fa6bad94bda 100644
--- a/components/exo/compositor_frame_sink_holder.cc
+++ b/components/exo/compositor_frame_sink_holder.cc
@@ -83,10 +83,15 @@ void CompositorFrameSinkHolder::ReclaimResources(
   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);
+
+    // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>,

[reveman, 2016/12/19 18:20:02]

So why doesn't simply:

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

just work if the scoped_refptr in the pair keeps it alive?

[Alex Z., 2016/12/19 18:34:03]

The erase() triggers Release() of the scoped_refptr, which, in turns, starts
~CompositorFrameSinkHolder() inside ReclaimResources() since without
|callback_pair|, |it| could be the last thing referencing
CompositorFrameSinkHolder, hence the crash.

The |callback_pair| maintains a scoped_refptr until the end of the
ReclaimResources(). |callback_pair| gets destroyed when exiting from
ReclaimResources, thus it'll keep CompositorFrameSinkHolder alive until then.
|callback_pair| achieves the same objective as the |holder| in
SurfaceFacotryOwner::ReclaimResources() before my refactor.

[reveman, 2016/12/19 19:30:02]

Ok, thanks for explaining. Instead of "auto callback_pair", can you save a
reference to the holder alone to make it more clear what needs to be kept alive.
Something like this:

scoped_refptr<CompositorFrameSinkHolder> sink_holder(it->second.first);
it->second.second->Run(resource.sync_token, resource.lost);
release_callbacks_.erase(it);

then this lgtm.

[Alex Z., 2016/12/19 19:37:50]

With that, the code would look really similar to what it was before all the
refactor. (line 174 on the left:
https://codereview.chromium.org/2493223002/diff/1/components/exo/surface.cc)
Would you prefer creating the holder inside the loop or leave it at the
beginning of this method (like before)?

[Alex Z., 2016/12/19 20:05:39]

In addition, it could still crash if more code is added after the loop in the
future since the scoped_refptr only lives inside the loop.

[reveman, 2016/12/19 20:18:15]

Yes, except the "holder(this)" part that is surprising. I think this code is
fragile in general and maybe a better solution would be to post a task to the
current message loop that will run the callback?

[Alex Z., 2016/12/19 20:54:07]

How do I post the task to the current message loop?

+    // std::unique_ptr<cc::SingleReleaseCallback>>. The
+    // scoped_refptr<CompositorFrameSinkHolder> inside prevent
+    // CompositorFrameSinkHolder being destroyed before ReclaimResources()
+    // returns.
+    auto callback_pair = std::move(it->second);
     release_callbacks_.erase(it);
-    callback->Run(resource.sync_token, resource.lost);
+    callback_pair.second->Run(resource.sync_token, resource.lost);
   }
 }