Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2584953002:
exo::CompositorFrameSinkHolder's release callbacks hold ref (Closed)
DescriptionBuffer holds reference to CompositorFrameSinkHolder so the
CompositorFrameSinkHolder lives until resources are released or Buffer is
destroyed.
CompositorFrameSinkHolder no longer holds references to itself in the field
release_callback_.
BUG=675004
Committed: https://crrev.com/d40f036aa8e40227403313246418c4ce8b908368
Cr-Commit-Position: refs/heads/master@{#441269}
Patch Set 1 #Patch Set 2 : Added comments explaining the fix #
Total comments: 7
Patch Set 3 : NOT FOR COMMIT: use ThreadTaskRunnerHandle to run SingleReleaseCallback #Patch Set 4 : Removed unrelated changes #
Total comments: 9
Patch Set 5 : Successfully using PostTask() #Patch Set 6 : Callback holds a reference to CompositorFrameSinkHolder #Patch Set 7 : Use base::ScopedClosureRunner instead of ThreadTaskRunnerHandle #Patch Set 8 : Use local ref ptr #Patch Set 9 : Release callbakcs hold onto refs to CompositorFrameSinkHolder #Patch Set 10 : Updated comments #Patch Set 11 : Use ScopedClosureRunner #
Total comments: 10
Patch Set 12 : Addressed nits #Patch Set 13 : Added ReleaseTextureAndCompositorFrameSinkHolder() and ReleaseContentsTextureAndCompositorFrameSink… #
Total comments: 8
Patch Set 14 : Addressed comments #
Total comments: 2
Patch Set 15 : Use PostTask() #Patch Set 16 : Added wrapper callback to surface.cc #Patch Set 17 : NOT FOR COMMIT: Buffer references sink_holder #
Total comments: 7
Patch Set 18 : NOT FOR COMMIT: SEGV_MAPERR in Callback::Run() #
Total comments: 1
Patch Set 19 : All exo unit tests passed. #
Total comments: 20
Patch Set 20 : Addressed comments #
Total comments: 2
Messages
Total messages: 70 (26 generated)
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Please review the changes.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
staraz@chromium.org changed reviewers: + reveman@chromium.org
Please review my changes
I'm failing to see how this fixes the bug. Please explain and add a comment to the code that makes it clear.
On 2016/12/19 17:14:10, reveman wrote: > I'm failing to see how this fixes the bug. Please explain and add a comment to > the code that makes it clear. Done.
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, 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?
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 18:20:02, reveman wrote: > 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? 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.
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 at 18:34:03, StarAZ1 wrote: > On 2016/12/19 18:20:02, reveman wrote: > > 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? > > 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. 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.
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 19:30:02, reveman wrote: > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > On 2016/12/19 18:20:02, reveman wrote: > > > 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? > > > > 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. > > 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. 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)?
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 19:30:02, reveman wrote: > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > On 2016/12/19 18:20:02, reveman wrote: > > > 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? > > > > 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. > > 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. 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.
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 at 19:37:50, StarAZ1 wrote: > On 2016/12/19 19:30:02, reveman wrote: > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > On 2016/12/19 18:20:02, reveman wrote: > > > > 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? > > > > > > 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. > > > > 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. > > 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)? 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?
https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, On 2016/12/19 20:18:15, reveman wrote: > On 2016/12/19 at 19:37:50, StarAZ1 wrote: > > On 2016/12/19 19:30:02, reveman wrote: > > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > > On 2016/12/19 18:20:02, reveman wrote: > > > > > 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? > > > > > > > > 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. > > > > > > 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. > > > > 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)? > > 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? How do I post the task to the current message loop?
On 2016/12/19 at 20:54:07, staraz wrote: > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > File components/exo/compositor_frame_sink_holder.cc (right): > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a std::pair<scoped_refptr<CompositorFrameSinkHolder>, > On 2016/12/19 20:18:15, reveman wrote: > > On 2016/12/19 at 19:37:50, StarAZ1 wrote: > > > On 2016/12/19 19:30:02, reveman wrote: > > > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > > > On 2016/12/19 18:20:02, reveman wrote: > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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)? > > > > 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? > > How do I post the task to the current message loop? base::ThreadTaskRunnerHandle::Get()->PostTask(...)
On 2016/12/19 21:03:12, reveman wrote: > On 2016/12/19 at 20:54:07, staraz wrote: > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a > std::pair<scoped_refptr<CompositorFrameSinkHolder>, > > On 2016/12/19 20:18:15, reveman wrote: > > > On 2016/12/19 at 19:37:50, StarAZ1 wrote: > > > > On 2016/12/19 19:30:02, reveman wrote: > > > > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > > > > On 2016/12/19 18:20:02, reveman wrote: > > > > > > > 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? > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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)? > > > > > > 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? > > > > How do I post the task to the current message loop? > > base::ThreadTaskRunnerHandle::Get()->PostTask(...) Thanks. PostTask takes a Closure while the map contains cc::SingleReleaseCallback. cc::SingleReleaseCallback is a wrapper for a specific type of base::Callback. I can change CompositorFrameSinkHolder::release_callbacks_ to take base::Callback instead of cc::SingleReleaseCallback, but that'll be a slightly larger change. Do you want me to land this CL as a quick fix to the unit tests first?
On 2016/12/19 at 21:29:19, staraz wrote: > On 2016/12/19 21:03:12, reveman wrote: > > On 2016/12/19 at 20:54:07, staraz wrote: > > > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a > > std::pair<scoped_refptr<CompositorFrameSinkHolder>, > > > On 2016/12/19 20:18:15, reveman wrote: > > > > On 2016/12/19 at 19:37:50, StarAZ1 wrote: > > > > > On 2016/12/19 19:30:02, reveman wrote: > > > > > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > > > > > On 2016/12/19 18:20:02, reveman wrote: > > > > > > > > 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? > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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. > > > > > > > > > > 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)? > > > > > > > > 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? > > > > > > How do I post the task to the current message loop? > > > > base::ThreadTaskRunnerHandle::Get()->PostTask(...) > > Thanks. > PostTask takes a Closure while the map contains cc::SingleReleaseCallback. > cc::SingleReleaseCallback is a wrapper for a specific type of base::Callback. > I can change CompositorFrameSinkHolder::release_callbacks_ to take base::Callback > instead of cc::SingleReleaseCallback, but that'll be a slightly larger change. > Do you want me to land this CL as a quick fix to the unit tests first? Instead of changing CompositorFrameSinkHolder::release_callbacks_, can we just post a task that owns the single release callback and runs it? Maybe use ScopedClosureRunner to ensure it will always run.
On 2016/12/19 23:04:42, reveman wrote: > On 2016/12/19 at 21:29:19, staraz wrote: > > On 2016/12/19 21:03:12, reveman wrote: > > > On 2016/12/19 at 20:54:07, staraz wrote: > > > > > > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/20001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:87: // |callback_pair| is a > > > std::pair<scoped_refptr<CompositorFrameSinkHolder>, > > > > On 2016/12/19 20:18:15, reveman wrote: > > > > > On 2016/12/19 at 19:37:50, StarAZ1 wrote: > > > > > > On 2016/12/19 19:30:02, reveman wrote: > > > > > > > On 2016/12/19 at 18:34:03, StarAZ1 wrote: > > > > > > > > On 2016/12/19 18:20:02, reveman wrote: > > > > > > > > > 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? > > > > > > > > > > > > > > > > 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. > > > > > > > > > > > > > > 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. > > > > > > > > > > > > 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)? > > > > > > > > > > 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? > > > > > > > > How do I post the task to the current message loop? > > > > > > base::ThreadTaskRunnerHandle::Get()->PostTask(...) > > > > Thanks. > > PostTask takes a Closure while the map contains cc::SingleReleaseCallback. > > cc::SingleReleaseCallback is a wrapper for a specific type of base::Callback. > > I can change CompositorFrameSinkHolder::release_callbacks_ to take > base::Callback > > instead of cc::SingleReleaseCallback, but that'll be a slightly larger change. > > Do you want me to land this CL as a quick fix to the unit tests first? > > Instead of changing CompositorFrameSinkHolder::release_callbacks_, can we just > post a task that owns the single release callback and runs it? Maybe use > ScopedClosureRunner to ensure it will always run. I'm using a ThreadTaskRunnerHandle but it seems the SingleReleaseCallback never runs. Could you please have a look? Thanks!
How are you checking that this runs? You'll need to have a running message loop for that. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:13: void RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, Can you bind cc::SingleReleaseCallback::Run directly instead of creating this wrapper function? https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:91: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; This shouldn't be needed anymore, right? https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:96: auto closure = nit: no need for this temporary variable. pass the base::Bind return value directly to PostTask. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:97: base::Bind(&RunSingleReleaseCallback, it->second.second.get(), 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. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:99: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); 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.
On 2016/12/20 16:34:28, reveman wrote: > How are you checking that this runs? You'll need to have a running message loop > for that. > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > File components/exo/compositor_frame_sink_holder.cc (right): > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:13: void > RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, > Can you bind cc::SingleReleaseCallback::Run directly instead of creating this > wrapper function? > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:91: > scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; > This shouldn't be needed anymore, right? > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:96: auto closure = > nit: no need for this temporary variable. pass the base::Bind return value > directly to PostTask. > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:97: > base::Bind(&RunSingleReleaseCallback, it->second.second.get(), > 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. > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > components/exo/compositor_frame_sink_holder.cc:99: > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); > 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. Never mind that. I didn't realize that I need to use base::Passed() and std::move() to pass a unique_ptr to a task. It all works now. I'll address your other comments.
On 2016/12/20 16:43:55, StarAZ1 wrote: > On 2016/12/20 16:34:28, reveman wrote: > > How are you checking that this runs? You'll need to have a running message > loop > > for that. > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:13: void > > RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, > > Can you bind cc::SingleReleaseCallback::Run directly instead of creating this > > wrapper function? > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:91: > > scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; > > This shouldn't be needed anymore, right? > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:96: auto closure = > > nit: no need for this temporary variable. pass the base::Bind return value > > directly to PostTask. > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:97: > > base::Bind(&RunSingleReleaseCallback, it->second.second.get(), > > 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. > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > components/exo/compositor_frame_sink_holder.cc:99: > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); > > 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. > > Never mind that. I didn't realize that I need to use base::Passed() and > std::move() to pass a unique_ptr to a task. > It all works now. I'll address your other comments. Removing the scoped_refptr<CompositorFrameSinkHolder> causes tests to crash again. My guess is that SingleReleaseCallback::Run() itself post a task to run asynchronously. Letting the wrapper RunSingleReleaseCallback() hold a reference to CompositorFrameSinkHolder should solve this problem. Do you think this is better than holding the reference in ReclaimResources()?
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:13: void RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, On 2016/12/20 16:34:28, reveman wrote: > Can you bind cc::SingleReleaseCallback::Run directly instead of creating this > wrapper function? This wrapper is now needed for holding a reference to CompositorFrameSinkHolder. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:91: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; On 2016/12/20 16:34:28, reveman wrote: > This shouldn't be needed anymore, right? 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. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:96: auto closure = On 2016/12/20 16:34:28, reveman wrote: > nit: no need for this temporary variable. pass the base::Bind return value > directly to PostTask. Done. https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... components/exo/compositor_frame_sink_holder.cc:99: base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); On 2016/12/20 16:34:28, reveman wrote: > 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. 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.
On 2016/12/20 at 16:50:43, staraz wrote: > On 2016/12/20 16:43:55, StarAZ1 wrote: > > On 2016/12/20 16:34:28, reveman wrote: > > > How are you checking that this runs? You'll need to have a running message > > loop > > > for that. > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:13: void > > > RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, > > > Can you bind cc::SingleReleaseCallback::Run directly instead of creating this > > > wrapper function? > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:91: > > > scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; > > > This shouldn't be needed anymore, right? > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:96: auto closure = > > > nit: no need for this temporary variable. pass the base::Bind return value > > > directly to PostTask. > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:97: > > > base::Bind(&RunSingleReleaseCallback, it->second.second.get(), > > > 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. > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > components/exo/compositor_frame_sink_holder.cc:99: > > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); > > > 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. > > > > Never mind that. I didn't realize that I need to use base::Passed() and > > std::move() to pass a unique_ptr to a task. > > It all works now. I'll address your other comments. > > Removing the scoped_refptr<CompositorFrameSinkHolder> causes tests to crash again. > My guess is that SingleReleaseCallback::Run() itself post a task to run asynchronously. > Letting the wrapper RunSingleReleaseCallback() hold a reference to CompositorFrameSinkHolder > should solve this problem. Do you think this is better than holding the reference in > ReclaimResources()? Can you find out exactly what is happening? I'd rather not land code that is based on guesses. Without knowing exactly what is happening it's hard for me to review this code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/20 18:10:03, reveman wrote: > On 2016/12/20 at 16:50:43, staraz wrote: > > On 2016/12/20 16:43:55, StarAZ1 wrote: > > > On 2016/12/20 16:34:28, reveman wrote: > > > > How are you checking that this runs? You'll need to have a running message > > > loop > > > > for that. > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > File components/exo/compositor_frame_sink_holder.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:13: void > > > > RunSingleReleaseCallback(cc::SingleReleaseCallback* callback, > > > > Can you bind cc::SingleReleaseCallback::Run directly instead of creating > this > > > > wrapper function? > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:91: > > > > scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder; > > > > This shouldn't be needed anymore, right? > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:96: auto closure = > > > > nit: no need for this temporary variable. pass the base::Bind return value > > > > directly to PostTask. > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:97: > > > > base::Bind(&RunSingleReleaseCallback, it->second.second.get(), > > > > 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. > > > > > > > > > > > > https://codereview.chromium.org/2584953002/diff/60001/components/exo/composit... > > > > components/exo/compositor_frame_sink_holder.cc:99: > > > > base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, closure); > > > > 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. > > > > > > Never mind that. I didn't realize that I need to use base::Passed() and > > > std::move() to pass a unique_ptr to a task. > > > It all works now. I'll address your other comments. > > > > Removing the scoped_refptr<CompositorFrameSinkHolder> causes tests to crash > again. > > My guess is that SingleReleaseCallback::Run() itself post a task to run > asynchronously. > > Letting the wrapper RunSingleReleaseCallback() hold a reference to > CompositorFrameSinkHolder > > should solve this problem. Do you think this is better than holding the > reference in > > ReclaimResources()? > > Can you find out exactly what is happening? I'd rather not land code that is > based on guesses. Without knowing exactly what is happening it's hard for me to > review this code. It doesn't matter when the SingleReleaseCallbacks run, as long as they run at some point. The problem is that the last reference to CompositorFrameSinkHolder gets erased in the middle of CompositorFrameSinkHolder::ReclaimResources so we need something else to keep CompositorFrameSinkHolder alive until the method returns. It was the local scoped_refptr before and the scoped_reftptr passed to RunSingleReleaseCallback() now. PS: I uploaded another patch to use ScopedClosureRunner.
Please update the description of this CL to explain what the problem is and how you're solving it. The current version just looks like a complicated version of adding a scoped_refptr. If PostTask is not the correct solution then let's not use ScopedClosureRunner and base:Bind. I think the confusing part is that we have one SinkHolder instance that the method is invoked for and then we have N SinkHolders and N singlereleasecallbacks that gets called and released/destroyed. Question. Is the caller not holding a ref to the SinkHolder that it's invoking a method on? Or is it doing that but that doesn't make a difference in this case?
Description was changed from ========== Create a local variable to hold onto the <CompositorFrameSinkHolder, callback> pair in exo::CompositorFrameSinkHolder::ReclaimResources. BUG=675004 ========== to ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL creates a local scoped_refptr to hold a reference to CompositorFrameSinkHolder so that it'll at least live until the end of ReclaimResources(). BUG=675004 ==========
On 2016/12/21 03:38:59, reveman wrote: > Please update the description of this CL to explain what the problem is and how > you're solving it. > > The current version just looks like a complicated version of adding a > scoped_refptr. If PostTask is not the correct solution then let's not use > ScopedClosureRunner and base:Bind. > > I think the confusing part is that we have one SinkHolder instance that the > method is invoked for and then we have N SinkHolders and N > singlereleasecallbacks that gets called and released/destroyed. > > Question. Is the caller not holding a ref to the SinkHolder that it's invoking a > method on? Or is it doing that but that doesn't make a difference in this case? All the scoped_refptr in release_callbacks_ refer to the SinkHolder itself (there is no "N SinkHolders and N singlereleasecallbacks that gets called and released/destroyed"). To answer your question: The caller does not hold a ref to the SinkHolder. The caller (exo::CompositorFrameSink) talks to SinkHolder via a mojo interface pointer. This is intended since we want to eventually move exo::CompositorFrameSink to gpu process and merge it with GpuCompositorFrameSink. The SinkHolder also owns the exo::CompositorFrameSink (hence the "holder" name) right now but that is temporary. I also updated the description.
Description was changed from ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL creates a local scoped_refptr to hold a reference to CompositorFrameSinkHolder so that it'll at least live until the end of ReclaimResources(). BUG=675004 ========== to ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL creates a local scoped_refptr to hold a reference to CompositorFrameSinkHolder so that it'll at least live until the end of ReclaimResources(). BUG=675004 ==========
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
We need to fix the design of this. CompositorFrameSinkHolder owning a number of references to itself is bad. That calling a CompositorFrameSinkHolder member function can cause the instance to be deleted is also bad. Can we find a design that avoids both of these issues. Maybe the SingleReleaseCallback::ReleaseCallback own the reference to the sink holder instead? And then we post a task to the current message loop to avoid having an instance be delete in the middle of calling a member function?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/21 16:39:07, reveman wrote: > We need to fix the design of this. CompositorFrameSinkHolder owning a number of > references to itself is bad. That calling a CompositorFrameSinkHolder member > function can cause the instance to be deleted is also bad. Can we find a design > that avoids both of these issues. > > Maybe the SingleReleaseCallback::ReleaseCallback own the reference to the sink > holder instead? And then we post a task to the current message loop to avoid > having an instance be delete in the middle of calling a member function? The release callbacks now take a scoped_refptr<CompositorFrameSinkHolder>. The tests look good but I have one concern: SinkHolder::release_callbacks is now a std::map<int, SingleReleaseCallback>. It takes any SingleReleaseCallback, including those that do not take a ref to SinkHolder. I updated some comments to reflect this but will it be an issue? I'll update the description.
Description was changed from ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL creates a local scoped_refptr to hold a reference to CompositorFrameSinkHolder so that it'll at least live until the end of ReclaimResources(). BUG=675004 ========== to ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL changes the release callbacks themselves to hold refs to CompositorFrameSinkHolder. ReclaimResources() puts the callbacks to the current MessageLoop so the destruction only happens after ReclaimResources() returns. BUG=675004 ==========
https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.cc:467: (compositor_frame_sink_holder)))); s/(compositor_frame_sink_holder)/compositor_frame_sink_holder/ https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:62: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); nit: CompositorFrameSinkHolder* compositor_frame_sink_holder https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:90: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); these Release(Contents)Texture functions don't actually use the sink_holder so can your wrap the call to these functions in a function that handles the sink_holder reference part? e.g. ReleaseTextureAndCompositorFrameSinkHolder https://codereview.chromium.org/2584953002/diff/200001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:84: scoped_refptr<CompositorFrameSinkHolder> holder(this); Please remove this and use PostTask instead. Instance being destroyed when a member function is called is a bad pattern. Better to defer this I think.
https://codereview.chromium.org/2584953002/diff/200001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:84: scoped_refptr<CompositorFrameSinkHolder> holder(this); On 2016/12/21 20:05:42, reveman wrote: > Please remove this and use PostTask instead. Instance being destroyed when a > member function is called is a bad pattern. Better to defer this I think. Sorry about this. I forgot to remove the line after changing to ScopedClosureRunner. I noticed that after using ScopedClosureRunner, several GamepadTest became flaky. I'll look into it.
https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:90: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); On 2016/12/21 20:05:42, reveman wrote: > these Release(Contents)Texture functions don't actually use the sink_holder so > can your wrap the call to these functions in a function that handles the > sink_holder reference part? e.g. ReleaseTextureAndCompositorFrameSinkHolder I'm not sure if I understand. SinkHolder doesn't get explicitly released and destroyed. Do you want another function (ReleaseTextureAndCompositorFrameSinkHolder()) that takes a texture and a ref to SinkHolder to hold onto the ref and pass the texture to ReleaseTexture()? The function still doesn't "do" anything to release the SinkHolder.
https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.cc:467: (compositor_frame_sink_holder)))); On 2016/12/21 20:05:42, reveman wrote: > s/(compositor_frame_sink_holder)/compositor_frame_sink_holder/ Done.
https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:90: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); On 2016/12/21 at 21:53:20, StarAZ1 wrote: > On 2016/12/21 20:05:42, reveman wrote: > > these Release(Contents)Texture functions don't actually use the sink_holder so > > can your wrap the call to these functions in a function that handles the > > sink_holder reference part? e.g. ReleaseTextureAndCompositorFrameSinkHolder > > I'm not sure if I understand. SinkHolder doesn't get explicitly released and destroyed. Do you want another function (ReleaseTextureAndCompositorFrameSinkHolder()) that takes a texture and a ref to SinkHolder to hold onto the ref and pass the texture to ReleaseTexture()? The function still doesn't "do" anything to release the SinkHolder. Yes, it's confusing that a function named "ReleaseTexture" for some reason takes a sink holder reference but doesn't use it. If you add a function that is called ReleaseTextureAndCompositorFrameSinkHolder that calls ReleaseTexture and then implicitly or explicitly drops the sink holder reference then that makes it more clear that this reference being kept alive is critical. In the current patch, it just looks like we're passing an unnecessary argument and someone might think it's a good idea to clean that up by removing that argument.
https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:62: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); On 2016/12/21 20:05:42, reveman wrote: > nit: CompositorFrameSinkHolder* compositor_frame_sink_holder Done. https://codereview.chromium.org/2584953002/diff/200001/components/exo/buffer.... components/exo/buffer.h:90: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); On 2016/12/21 22:46:00, reveman wrote: > On 2016/12/21 at 21:53:20, StarAZ1 wrote: > > On 2016/12/21 20:05:42, reveman wrote: > > > these Release(Contents)Texture functions don't actually use the sink_holder > so > > > can your wrap the call to these functions in a function that handles the > > > sink_holder reference part? e.g. ReleaseTextureAndCompositorFrameSinkHolder > > > > I'm not sure if I understand. SinkHolder doesn't get explicitly released and > destroyed. Do you want another function > (ReleaseTextureAndCompositorFrameSinkHolder()) that takes a texture and a ref to > SinkHolder to hold onto the ref and pass the texture to ReleaseTexture()? The > function still doesn't "do" anything to release the SinkHolder. > > Yes, it's confusing that a function named "ReleaseTexture" for some reason takes > a sink holder reference but doesn't use it. If you add a function that is called > ReleaseTextureAndCompositorFrameSinkHolder that calls ReleaseTexture and then > implicitly or explicitly drops the sink holder reference then that makes it more > clear that this reference being kept alive is critical. In the current patch, it > just looks like we're passing an unnecessary argument and someone might think > it's a good idea to clean that up by removing that argument. Done.
https://codereview.chromium.org/2584953002/diff/240001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/buffer.... components/exo/buffer.h:62: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); nit: s/scoped_refptr<CompositorFrameSinkHolder>/CompositorFrameSinkHolder*/ https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: base::ScopedClosureRunner runner(base::Bind( You're still not using PostTask here so this sink holder instance might be destroyed before this function returns. That it might be destroyed in the middle of a loop seems especially bad. Please use PostTask or at least move all callbacks to a temporary variable and run them after the loop that access release_callbacks_. https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:82: // Each release callback should should take a Isn't this beyond the details of this class? If the callback needs a reference then it should take it and if not then there's no need to. I think it's better to explain why a reference is needed at the place where we take that reference.
https://codereview.chromium.org/2584953002/diff/240001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/buffer.... components/exo/buffer.h:62: scoped_refptr<CompositorFrameSinkHolder> compositor_frame_sink_holder); On 2016/12/22 17:00:53, reveman wrote: > nit: s/scoped_refptr<CompositorFrameSinkHolder>/CompositorFrameSinkHolder*/ Done. https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: base::ScopedClosureRunner runner(base::Bind( On 2016/12/22 17:00:53, reveman wrote: > You're still not using PostTask here so this sink holder instance might be > destroyed before this function returns. That it might be destroyed in the middle > of a loop seems especially bad. Please use PostTask or at least move all > callbacks to a temporary variable and run them after the loop that access > release_callbacks_. I thought you wanted the ScopedClosureRunner to protect the callback against the message loop shutting down before the callback gets to run? https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:82: // Each release callback should should take a On 2016/12/22 17:00:53, reveman wrote: > Isn't this beyond the details of this class? If the callback needs a reference > then it should take it and if not then there's no need to. I think it's better > to explain why a reference is needed at the place where we take that reference. Done.
https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: base::ScopedClosureRunner runner(base::Bind( On 2016/12/22 at 18:08:48, StarAZ1 wrote: > On 2016/12/22 17:00:53, reveman wrote: > > You're still not using PostTask here so this sink holder instance might be > > destroyed before this function returns. That it might be destroyed in the middle > > of a loop seems especially bad. Please use PostTask or at least move all > > callbacks to a temporary variable and run them after the loop that access > > release_callbacks_. > > I thought you wanted the ScopedClosureRunner to protect the callback against the message loop shutting down before the callback gets to run? Yes, the only way that can happen is if you post a task to the message loop. Without a PostTask you're still letting the callback run here and the sink holder instance to be destroyed in the middle of this loop inside a member function of the sink holder. https://codereview.chromium.org/2584953002/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2584953002/diff/260001/components/exo/surface... components/exo/surface.cc:660: void Surface::UpdateResource(bool client_usage) { Isn't this function the correct place to wrap the release callback? This is where we introduce the dependency on compositor_frame_sink_holder_. The Buffer class shouldn't need to know about that.
The CQ bit was checked by staraz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/240001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:87: base::ScopedClosureRunner runner(base::Bind( On 2016/12/22 20:29:49, reveman wrote: > On 2016/12/22 at 18:08:48, StarAZ1 wrote: > > On 2016/12/22 17:00:53, reveman wrote: > > > You're still not using PostTask here so this sink holder instance might be > > > destroyed before this function returns. That it might be destroyed in the > middle > > > of a loop seems especially bad. Please use PostTask or at least move all > > > callbacks to a temporary variable and run them after the loop that access > > > release_callbacks_. > > > > I thought you wanted the ScopedClosureRunner to protect the callback against > the message loop shutting down before the callback gets to run? > > Yes, the only way that can happen is if you post a task to the message loop. > Without a PostTask you're still letting the callback run here and the sink > holder instance to be destroyed in the middle of this loop inside a member > function of the sink holder. Done. https://codereview.chromium.org/2584953002/diff/260001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2584953002/diff/260001/components/exo/surface... components/exo/surface.cc:660: void Surface::UpdateResource(bool client_usage) { On 2016/12/22 20:29:50, reveman wrote: > Isn't this function the correct place to wrap the release callback? This is > where we introduce the dependency on compositor_frame_sink_holder_. The Buffer > class shouldn't need to know about that. Done.
This design is still wrong afaict. Here are some key questions that I think will help find the correct design. What instance needs the sink_holder to be alive and why does it need it to be alive? The surface or the buffer? The instance that needs it to be alive should keep a reference to it. When the instance that needs to keep a ref to the sink holder goes away, can the sink holder reference then be dropped? If not, why not?
On 2016/12/24 21:37:46, reveman wrote: > This design is still wrong afaict. Here are some key questions that I think will > help find the correct design. > > What instance needs the sink_holder to be alive and why does it need it to be > alive? The surface or the buffer? The instance that needs it to be alive should > keep a reference to it. > > When the instance that needs to keep a ref to the sink holder goes away, can the > sink holder reference then be dropped? If not, why not? I uploaded another patch set. In the new patch, a Buffer holds onto a reference to a sink_holder when ProduceTextureMailbox() is called. The refptr is reset in CheckReleaseCallback(). There's one problem with this approach: SinkHolder owns the release callbacks. Therefore, the callback could outlive the Buffer. When the Buffer gets destroyed, SinkHolder is destroyed and a crash occurs as we are destroying callbacks hasn't run. Is it possible to let Buffer own its release callback and SinkHolder stores weak pointers to callbacks in the map?
On 2017/01/02 at 17:17:59, staraz wrote: > On 2016/12/24 21:37:46, reveman wrote: > > This design is still wrong afaict. Here are some key questions that I think will > > help find the correct design. > > > > What instance needs the sink_holder to be alive and why does it need it to be > > alive? The surface or the buffer? The instance that needs it to be alive should > > keep a reference to it. > > > > When the instance that needs to keep a ref to the sink holder goes away, can the > > sink holder reference then be dropped? If not, why not? > > I uploaded another patch set. > > In the new patch, a Buffer holds onto a reference to a sink_holder when > ProduceTextureMailbox() is called. The refptr is reset in CheckReleaseCallback(). > > There's one problem with this approach: SinkHolder owns the release callbacks. > Therefore, the callback could outlive the Buffer. When the Buffer gets destroyed, > SinkHolder is destroyed and a crash occurs as we are destroying callbacks hasn't > run. > > Is it possible to let Buffer own its release callback and SinkHolder stores weak > pointers to callbacks in the map? I think this will be fixed after removing the use of cc::SingleReleaseCallback. See my other comments. FYI, the use of SingleReleaseCallback comes from a time when we used texture layers for contents. https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.... components/exo/buffer.cc:402: std::unique_ptr<cc::SingleReleaseCallback> Buffer::ProduceTextureMailbox( It doesn't make sense for this function to return a release callback anymore. Also doesn't make much sense to be producing a TextureMailbox anymore. Please have this return a bool and a cc::TransferableResource instead. The buffer is the instance that cares about the release callback and not the surface so this buffer code is also where a release callback should be set for the sink holder. Please change the signature to: bool Buffer::ProduceTransferableResource(CompositorFrameSinkHolder* compositor_frame_sink_holder, int resource_id, bool secure_output_only, bool client_usage, cc::TransferableResource* resource); https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.... components/exo/buffer.cc:441: compositor_frame_sink_holder_ = compositor_frame_sink_holder; nit: short comment here saying something like: "We need to keep a reference to the sink holder to receive a release callback. https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.... components/exo/buffer.cc:542: compositor_frame_sink_holder_ = nullptr; Do we really need this here? Removing it would solve the sink holder destruction problems afaict. https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/320001/components/exo/buffer.... components/exo/buffer.h:129: // The CompositorFrameSinkHolder that has the ReleaseCallback of this buffer please updated this comment after addressing my other comments. https://codereview.chromium.org/2584953002/diff/320001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:32: void CompositorFrameSinkHolder::AddResourceReleaseCallback( nit: SetResourceReleaseCallback https://codereview.chromium.org/2584953002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:86: bool will_run = base::ThreadTaskRunnerHandle::Get()->PostTask( If you remove the reset of the sink holder in Buffer::CheckReleaseCallback then I think we can finally do just this here: auto it = release_callbacks_.find(resource.id); DCHECK(it != release_callbacks_.end()); it->second->Run(resource.sync_token, resource.lost); release_callbacks_.erase(it); https://codereview.chromium.org/2584953002/diff/320001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2584953002/diff/320001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:84: std::map<int, std::unique_ptr<cc::SingleReleaseCallback>>; Do we need to replace the use of cc::SingleReleaseCallback with a simple base::Callback to allow it to not run when the buffer is destroyed? I think the DCHECK in cc::SingleReleaseCallback dtor will fail otherwise.
Description was changed from ========== The release_callback_.erase(it) in CompositorFrameSinkHolder::ReclaimResources() could erase the last reference to the CompositorFrameSinkHolder and cause the CompositorFrameSinkHolder to be garbage collected before ReclaimResources() returns. This causes the crash in the bug. This CL changes the release callbacks themselves to hold refs to CompositorFrameSinkHolder. ReclaimResources() puts the callbacks to the current MessageLoop so the destruction only happens after ReclaimResources() returns. BUG=675004 ========== to ========== Buffer holds reference to CompositorFrameSinkHolder so the CompositorFrameSinkHolder lives until resources are released or Buffer is destroyed. CompositorFrameSinkHolder no longer holds references to itself in the field release_callback_. BUG=675004 ==========
https://codereview.chromium.org/2584953002/diff/340001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/340001/components/exo/buffer.... components/exo/buffer.h:134: // The refptr is reset when the release callback is called. remove this line? https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:560: if (this) remove "if (this)". that's never a good idea and the call to this function is already protected by a weak ptr. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:566: if (this) { ditto https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... File components/exo/buffer_unittest.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:34: std::unique_ptr<Surface> surface(new Surface()); nit: s/new Surface()/new Surface/ https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:46: bool r = buffer->ProduceTransferableResource( nit: s/bool r/bool rv/ for consistency https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:71: std::unique_ptr<Surface> surface(new Surface()); nit: s/new Surface()/new Surface/ https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:79: bool r = buffer->ProduceTransferableResource( nit: s/bool r/bool rv/ https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:107: bool r2 = buffer->ProduceTransferableResource( nit: s/bool r2/bool rv/ or just "rv = ..." if rv is declared above https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:35: DCHECK(callback); nit: DCHECK(!callback.is_null()) https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:75: ~CompositorFrameSinkHolder() override; why change this? https://codereview.chromium.org/2584953002/diff/360001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/surface... components/exo/surface.cc:666: ++next_resource_id_; nit: move the increment above reverse the if-statement: if (!current_buffer_.buffer() || !current_buffer_.buffer()->ProduceTransferableResource( compositor_frame_sink_holder_.get(), next_resource_id_++, state_.only_visible_on_secure_output, client_usage, ¤t_resource_)) { current_resource_.id = 0; current_resource_.size = gfx::Size(); }
https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.cc File components/exo/buffer.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:560: if (this) On 2017/01/03 23:24:46, reveman wrote: > remove "if (this)". that's never a good idea and the call to this function is > already protected by a weak ptr. Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer.... components/exo/buffer.cc:566: if (this) { On 2017/01/03 23:24:46, reveman wrote: > ditto Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... File components/exo/buffer_unittest.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:34: std::unique_ptr<Surface> surface(new Surface()); On 2017/01/03 23:24:47, reveman wrote: > nit: s/new Surface()/new Surface/ Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:46: bool r = buffer->ProduceTransferableResource( On 2017/01/03 23:24:47, reveman wrote: > nit: s/bool r/bool rv/ for consistency Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:71: std::unique_ptr<Surface> surface(new Surface()); On 2017/01/03 23:24:47, reveman wrote: > nit: s/new Surface()/new Surface/ Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:79: bool r = buffer->ProduceTransferableResource( On 2017/01/03 23:24:47, reveman wrote: > nit: s/bool r/bool rv/ Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/buffer_... components/exo/buffer_unittest.cc:107: bool r2 = buffer->ProduceTransferableResource( On 2017/01/03 23:24:46, reveman wrote: > nit: s/bool r2/bool rv/ or just "rv = ..." if rv is declared above Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... File components/exo/compositor_frame_sink_holder.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... components/exo/compositor_frame_sink_holder.cc:35: DCHECK(callback); On 2017/01/03 23:24:47, reveman wrote: > nit: DCHECK(!callback.is_null()) Done. https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... File components/exo/compositor_frame_sink_holder.h (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/composi... components/exo/compositor_frame_sink_holder.h:75: ~CompositorFrameSinkHolder() override; On 2017/01/03 23:24:47, reveman wrote: > why change this? This was for the MockCompositorFrameSinkHolder. I'll revert the change. https://codereview.chromium.org/2584953002/diff/360001/components/exo/surface.cc File components/exo/surface.cc (right): https://codereview.chromium.org/2584953002/diff/360001/components/exo/surface... components/exo/surface.cc:666: ++next_resource_id_; On 2017/01/03 23:24:47, reveman wrote: > nit: move the increment above reverse the if-statement: > > if (!current_buffer_.buffer() || > !current_buffer_.buffer()->ProduceTransferableResource( > compositor_frame_sink_holder_.get(), next_resource_id_++, > state_.only_visible_on_secure_output, client_usage, > ¤t_resource_)) { > current_resource_.id = 0; > current_resource_.size = gfx::Size(); > } Done.
The CQ bit was checked by staraz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2584953002/#ps380001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This new design looks much better. Thanks! LGTM with two minor nits https://codereview.chromium.org/2584953002/diff/380001/components/exo/buffer.cc File components/exo/buffer.cc (left): https://codereview.chromium.org/2584953002/diff/380001/components/exo/buffer.... components/exo/buffer.cc:545: nit: keep this line https://codereview.chromium.org/2584953002/diff/380001/components/exo/buffer.h File components/exo/buffer.h (right): https://codereview.chromium.org/2584953002/diff/380001/components/exo/buffer.... components/exo/buffer.h:129: // The refptr is reset when the release callback is called. nit: ..reset after the release callback has been called
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1483486763579420,
"parent_rev": "b109d727ca85e767b954dea542e2800ad606fba7", "commit_rev":
"16cdf43dc0b01e5d2b5edc866bbc09a9f3564074"}
Message was sent while issue was closed.
Description was changed from ========== Buffer holds reference to CompositorFrameSinkHolder so the CompositorFrameSinkHolder lives until resources are released or Buffer is destroyed. CompositorFrameSinkHolder no longer holds references to itself in the field release_callback_. BUG=675004 ========== to ========== Buffer holds reference to CompositorFrameSinkHolder so the CompositorFrameSinkHolder lives until resources are released or Buffer is destroyed. CompositorFrameSinkHolder no longer holds references to itself in the field release_callback_. BUG=675004 Review-Url: https://codereview.chromium.org/2584953002 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Buffer holds reference to CompositorFrameSinkHolder so the CompositorFrameSinkHolder lives until resources are released or Buffer is destroyed. CompositorFrameSinkHolder no longer holds references to itself in the field release_callback_. BUG=675004 Review-Url: https://codereview.chromium.org/2584953002 ========== to ========== Buffer holds reference to CompositorFrameSinkHolder so the CompositorFrameSinkHolder lives until resources are released or Buffer is destroyed. CompositorFrameSinkHolder no longer holds references to itself in the field release_callback_. BUG=675004 Committed: https://crrev.com/d40f036aa8e40227403313246418c4ce8b908368 Cr-Commit-Position: refs/heads/master@{#441269} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/d40f036aa8e40227403313246418c4ce8b908368 Cr-Commit-Position: refs/heads/master@{#441269} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
