Mus: GpuCompositorFrameSink uses WeakPtrFactory

Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org

Committed: https://crrev.com/029f79a574a4e032561ac44a72746c95814487dc
Cr-Commit-Position: refs/heads/master@{#434347}

[Fady Samuel, 2016-11-24 15:39:47]

fsamuel@chromium.org changed reviewers:
+ sadrul@chromium.org, yzshen@chromium.org

[Fady Samuel, 2016-11-24 15:39:48]

+yzshen@ who is cced on the bug.
+sadrul@ FYI.

I will TBR.

[Fady Samuel, 2016-11-24 15:40:12]

Description was changed from

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
==========

to

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org
==========

[Fady Samuel, 2016-11-24 15:40:15]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2016-11-24 15:40:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-11-24 15:46:14]

On 2016/11/24 15:40:31, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

LGTM

(Just out of curiosity) how did you figure this out? :)
The failed builds I looked at didn't log anything about GpuCompositorFrameSink.

And those builds with mash_browser_tests throwing exceptions all reported
"SUCCESS: all tests passed."

However, I did see a *successful* build which has a crash callstack:
(https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...)
==============================
Received signal 11 SEGV_MAPERR fffff6d7bb391351
#0 0x000002ce2fa7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7fc8c13becb0 <unknown>
#2 0x000001b3cad9 gpu::GpuChannelHost::Send()
#3 0x000001b3ccf9 gpu::GpuChannelHost::OrderingBarrier()
#4 0x000001b3a259 gpu::CommandBufferProxyImpl::Flush()
#5 0x000001b34d7a gpu::CommandBufferHelper::Flush()
#6 0x000004258e6d gpu::gles2::GLES2Implementation::FlushHelper()
#7 0x00000425653b
gpu::gles2::GLES2Implementation::SetAggressivelyFreeResources()
#8 0x000006b01133 cc::GLRenderer::~GLRenderer()
#9 0x000006b02a39 cc::GLRenderer::~GLRenderer()
#10 0x00000430e195 cc::Display::~Display()
#11 0x00000430e289 cc::Display::~Display()
#12 0x0000066b14ea ui::ws::GpuCompositorFrameSink::~GpuCompositorFrameSink()
#13 0x0000066b15d0 ui::ws::GpuCompositorFrameSink::~GpuCompositorFrameSink()
#14 0x0000066ae1f1 mojo::StrongBinding<>::OnConnectionError()
#15 0x000003abd7fd mojo::InterfaceEndpointClient::NotifyError()
#16 0x000003ac0461 mojo::internal::MultiplexRouter::ProcessTasks()
#17 0x000003abf59c mojo::internal::MultiplexRouter::OnPipeConnectionError()
#18 0x000003abb310 mojo::Connector::HandleError()
#19 0x000003ac62d3
mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()
#20 0x000002cfbd2f base::MessageLoop::~MessageLoop()
#21 0x000002cd7be8 (anonymous namespace)::StartChildApp()
#22 0x00000173aa34
_ZN4base8internal7InvokerINS0_9BindStateIPFvN4mojo16InterfaceRequestIN3ash5mojom15NewWindowClientEEEEJEEES9_E3RunEPNS0_13BindStateBaseEOS8_
#23 0x000002898973 service_manager::ChildProcessMainWithCallback()
#24 0x000002cd78d6 RunMashBrowserTests()

[Fady Samuel, 2016-11-24 15:54:22]

On 2016/11/24 15:46:14, yzshen1 wrote:
> On 2016/11/24 15:40:31, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> LGTM
> 
> (Just out of curiosity) how did you figure this out? :)
> The failed builds I looked at didn't log anything about
GpuCompositorFrameSink.
> 
> And those builds with mash_browser_tests throwing exceptions all reported
> "SUCCESS: all tests passed."
> 
> However, I did see a *successful* build which has a crash callstack:
>
(https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...)
> ==============================
> Received signal 11 SEGV_MAPERR fffff6d7bb391351
> #0 0x000002ce2fa7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #1 0x7fc8c13becb0 <unknown>
> #2 0x000001b3cad9 gpu::GpuChannelHost::Send()
> #3 0x000001b3ccf9 gpu::GpuChannelHost::OrderingBarrier()
> #4 0x000001b3a259 gpu::CommandBufferProxyImpl::Flush()
> #5 0x000001b34d7a gpu::CommandBufferHelper::Flush()
> #6 0x000004258e6d gpu::gles2::GLES2Implementation::FlushHelper()
> #7 0x00000425653b
> gpu::gles2::GLES2Implementation::SetAggressivelyFreeResources()
> #8 0x000006b01133 cc::GLRenderer::~GLRenderer()
> #9 0x000006b02a39 cc::GLRenderer::~GLRenderer()
> #10 0x00000430e195 cc::Display::~Display()
> #11 0x00000430e289 cc::Display::~Display()
> #12 0x0000066b14ea ui::ws::GpuCompositorFrameSink::~GpuCompositorFrameSink()
> #13 0x0000066b15d0 ui::ws::GpuCompositorFrameSink::~GpuCompositorFrameSink()
> #14 0x0000066ae1f1 mojo::StrongBinding<>::OnConnectionError()
> #15 0x000003abd7fd mojo::InterfaceEndpointClient::NotifyError()
> #16 0x000003ac0461 mojo::internal::MultiplexRouter::ProcessTasks()
> #17 0x000003abf59c mojo::internal::MultiplexRouter::OnPipeConnectionError()
> #18 0x000003abb310 mojo::Connector::HandleError()
> #19 0x000003ac62d3
> mojo::Watcher::MessageLoopObserver::WillDestroyCurrentMessageLoop()
> #20 0x000002cfbd2f base::MessageLoop::~MessageLoop()
> #21 0x000002cd7be8 (anonymous namespace)::StartChildApp()
> #22 0x00000173aa34
>
_ZN4base8internal7InvokerINS0_9BindStateIPFvN4mojo16InterfaceRequestIN3ash5mojom15NewWindowClientEEEEJEEES9_E3RunEPNS0_13BindStateBaseEOS8_
> #23 0x000002898973 service_manager::ChildProcessMainWithCallback()
> #24 0x000002cd78d6 RunMashBrowserTests()

Ahh I was fixing another stack trace:


Received signal 11 <unknown> 000000000000
#0 0x000002ce1ab7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f408c404cb0 <unknown>
#2 0x0000066ae121 [1121/163337:ERROR:wm_shell_mus.cc(405)] Not implemented
reached in virtual void
ash::mus::WmShellMus::RemoveDisplayObserver(ash::WmDisplayObserver *)
ui::ws::GpuCompositorFrameSink::DidReceiveCompositorFrameAck()
#3 0x000004311cc3 cc::Surface::RunDrawCallbacks()
#4 0x0000043134db cc::SurfaceAggregator::ProcessAddedAndRemovedSurfaces()
#5 0x000004317221 cc::SurfaceAggregator::Aggregate()
#6 0x00000430d7f6 cc::Display::DrawAndSwap()
#7 0x00000430f4e1 cc::DisplayScheduler::DrawAndSwap()
#8 0x00000430e966 cc::DisplayScheduler::OnBeginFrameDeadline()
#9 0x000002d57e5e base::debug::TaskAnnotator::RunTask()
#10 0x000002cf9f4c base::MessageLoop::RunTask()
#11 0x000002cfa1f8 base::MessageLoop::DeferOrRunPendingTask()
#12 0x000002cfa4db base::MessageLoop::DoWork()
#13 0x000002cfb52a base::MessagePumpDefault::Run()
#14 0x000002cf9cf8 base::MessageLoop::RunHandler()
#15 0x000002d14c30 base::RunLoop::Run()


Maybe there's another problem here? I don't know...

BTW, this is an old stack trace. The problem may have changed since.

[commit-bot: I haz the power, 2016-11-24 16:38:12]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1480002015112010, "parent_rev":
"9103f3ef34b16ac78dc1660bae29f14263193ed1", "commit_rev":
"655fbca9fecc3c5a60eeb56092944d3e2d79ae9e"}

[commit-bot: I haz the power, 2016-11-24 16:38:36]

Description was changed from

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org
==========

to

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org
==========

[commit-bot: I haz the power, 2016-11-24 16:38:37]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-24 16:41:08]

Description was changed from

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org
==========

to

==========
Mus: GpuCompositorFrameSink uses WeakPtrFactory

A Surface can outlive the GpuCompositorFrameSink that created it.
Submitting a CompositorFrame to a surface means that the draw callback
could get called after GpuCompositorFrameSink is destroyed. By binding
the draw callback to a weak ptr we ensure we don't call it if
GpuCompositorFrameSink has been destroyed.

BUG=668420
TBR=yzshen@chromium.org, sadrul@chromium.org

Committed: https://crrev.com/029f79a574a4e032561ac44a72746c95814487dc
Cr-Commit-Position: refs/heads/master@{#434347}
==========

[commit-bot: I haz the power, 2016-11-24 16:41:09]

Patchset 1 (id:??) landed as
https://crrev.com/029f79a574a4e032561ac44a72746c95814487dc
Cr-Commit-Position: refs/heads/master@{#434347}