exo::CompositorFrameSinkHolder's release callbacks hold ref

components/exo/compositor_frame_sink_holder.cc

 // Copyright 2016 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 "components/exo/compositor_frame_sink_holder.h"
 
 #include "cc/resources/returned_resource.h"
 #include "components/exo/surface.h"
 
 namespace exo {
@@ skipping 65 matching lines @@
     active_frame_callbacks_.pop_front();
   }
   begin_frame_source_->OnBeginFrame(args);
 }
 
 void CompositorFrameSinkHolder::ReclaimResources(
     const cc::ReturnedResourceArray& resources) {
   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);
   }
 }
 
 void CompositorFrameSinkHolder::WillDrawSurface() {
   if (surface_)
     surface_->WillDraw();
 
   UpdateNeedsBeginFrame();
 }
 
@@ skipping 33 matching lines @@
 
   bool needs_begin_frame = !active_frame_callbacks_.empty();
   if (needs_begin_frame == needs_begin_frame_)
     return;
 
   needs_begin_frame_ = needs_begin_frame;
   OnNeedsBeginFrames(needs_begin_frame_);
 }
 
 }  // namespace exo