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 {
 
+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:
 
 CompositorFrameSinkHolder::CompositorFrameSinkHolder(
     Surface* surface,
     std::unique_ptr<CompositorFrameSink> frame_sink,
     cc::mojom::MojoCompositorFrameSinkClientRequest request)
     : surface_(surface),
       frame_sink_(std::move(frame_sink)),
       begin_frame_source_(base::MakeUnique<cc::ExternalBeginFrameSource>(this)),
@@ skipping 51 matching lines @@
 void CompositorFrameSinkHolder::OnBeginFrame(const cc::BeginFrameArgs& args) {
   while (!active_frame_callbacks_.empty()) {
     active_frame_callbacks_.front().Run(args.frame_time);
     active_frame_callbacks_.pop_front();
   }
   begin_frame_source_->OnBeginFrame(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);
   }
 }
 
 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