Fix a crash in mus::GpuService

Fix a crash in mus::GpuService

BUG=620058
Committed: https://crrev.com/ff6aef2ca17c3f114d52b5ba30b912d12f6c3a5b
Cr-Commit-Position: refs/heads/master@{#400118}

[Peng, 2016-06-14 16:01:12]

penghuang@chromium.org changed reviewers:
+ ben@chromium.org, jam@chromium.org

[Peng, 2016-06-14 16:01:12]

Hi Ben & John, PTAL. Thanks.

[jam, 2016-06-14 17:03:52]

bug?

it would be helpful to see a stack trace.

is this happening because this code is called on the UI thread in the browser
process and we have thread restrictions turned on there?

I assume the assert is inside the bindings code? we have tradeoff. if we add the
scoped allow wait to the sync bindings code then we wouldn't need to add a
thread restriction class each time we call a new sync method. however that would
also mean that we can keep introducing sync calls on critical threads easily.

[Peng, 2016-06-14 17:43:21]

On 2016/06/14 17:03:52, jam wrote:
> bug?
> 
> it would be helpful to see a stack trace.
> 
> is this happening because this code is called on the UI thread in the browser
> process and we have thread restrictions turned on there?
> 
> I assume the assert is inside the bindings code? we have tradeoff. if we add
the
> scoped allow wait to the sync bindings code then we wouldn't need to add a
> thread restriction class each time we call a new sync method. however that
would
> also mean that we can keep introducing sync calls on critical threads easily.

Filed a bug https://bugs.chromium.org/p/chromium/issues/detail?id=619985

The stack trace is:

[21137:21137:0614/133710:FATAL:thread_restrictions.cc(72)] Waiting is not
allowed to be used on this thread to prevent jank and deadlock.
#0 0x7fc443bcc05e base::debug::StackTrace::StackTrace()
#1 0x7fc443c23fdc logging::LogMessage::~LogMessage()
#2 0x7fc443d6f4cb base::ThreadRestrictions::AssertWaitAllowed()
#3 0x7fc443d0c98f base::ConditionVariable::Wait()
#4 0x7fc4369d134d mojo::edk::Waiter::Wait()
#5 0x7fc4369790e4 mojo::edk::Core::WaitManyInternal()
#6 0x7fc436978b68 mojo::edk::Core::Wait()
#7 0x7fc4369d5a53 MojoWaitImpl
#8 0x7fc43b65a03f MojoWait
#9 0x7fc43612b40a mojo::Wait()
#10 0x7fc43615c138 mojo::internal::SyncHandleRegistry::WatchAllHandles()
#11 0x7fc436160094 mojo::internal::SyncHandleWatcher::SyncWatch()
#12 0x7fc43612ad13 mojo::internal::Connector::SyncWatch()
#13 0x7fc436153cd1 mojo::internal::Router::AcceptWithResponder()
#14 0x7fc4361c5eb3 mus::mojom::GpuServiceProxy::EstablishGpuChannel()
#15 0x7fc43611ded4 mus::GpuService::EstablishGpuChannel()
#16 0x7fc435aa1c40 mus::GLES2Context::Initialize()
#17 0x7fc435aa2211 mus::GLES2Context::CreateOffscreenContext()
#18 0x7fc435aa0f42 mus::ContextProvider::BindToCurrentThread()
#19 0x7fc43b9365f3 cc::OutputSurface::BindToClient()
#20 0x7fc435aa6504 mus::OutputSurface::BindToClient()
#21 0x7fc43babe80a cc::LayerTreeHostImpl::InitializeRenderer()
#22 0x7fc43bb61249 cc::SingleThreadProxy::SetOutputSurface()
#23 0x7fc43ba90f1a cc::LayerTreeHost::SetOutputSurface()
#24 0x7fc4388c347d ui::Compositor::SetOutputSurface()
#25 0x7fc435a90654 views::SurfaceContextFactory::CreateOutputSurface()
#26 0x7fc4388c4399 ui::Compositor::RequestNewOutputSurface()
#27 0x7fc43ba9138d cc::LayerTreeHost::RequestNewOutputSurface()
#28 0x7fc43bb60e15 cc::SingleThreadProxy::RequestNewOutputSurface()
#29 0x7fc43b86ab50
_ZN4base8internal15RunnableAdapterIMN2cc28ScrollbarAnimationControllerEFvvEE3RunIPS3_JEEEvOT_DpOT0_
#30 0x7fc43b86aaae
_ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRNS0_15RunnableAdapterIMN2cc28ScrollbarAnimationControllerEFvvEEENS_7WeakPtrIS6_EEJEEEvOT_T0_DpOT1_
#31 0x7fc43bb67fa1
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMN2cc17SingleThreadProxyEFvvEEEFvPS7_EJNS_7WeakPtrIS7_EEEEELb1EFvvEE3RunEPNS0_13BindStateBaseE
#32 0x7fc43b86abae base::Callback<>::Run()
#33 0x7fc43b86a829 base::CancelableCallback<>::Forward()
#34 0x7fc43b86ab50
_ZN4base8internal15RunnableAdapterIMN2cc28ScrollbarAnimationControllerEFvvEE3RunIPS3_JEEEvOT_DpOT0_
#35 0x7fc43b86aaae
_ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRNS0_15RunnableAdapterIMN2cc28ScrollbarAnimationControllerEFvvEEENS_7WeakPtrIS6_EEJEEEvOT_T0_DpOT1_
#36 0x7fc43b86aa51
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMNS_18CancelableCallbackIFvvEEEKFvvEEEFvPKS8_EJNS_7WeakPtrIS8_EEEEELb1ES7_E3RunEPNS0_13BindStateBaseE
#37 0x7fc443bb00ce base::Callback<>::Run()
#38 0x7fc443bd189e base::debug::TaskAnnotator::RunTask()
#39 0x7fc443c3ff91 base::MessageLoop::RunTask()
#40 0x7fc443c40218 base::MessageLoop::DeferOrRunPendingTask()
#41 0x7fc443c40472 base::MessageLoop::DoWork()
#42 0x7fc443c568fc base::MessagePumpLibevent::Run()
#43 0x7fc443c3fa0a base::MessageLoop::RunHandler()
#44 0x7fc443cd88f4 base::RunLoop::Run()
#45 0x7fc447f5b127 ChromeBrowserMainParts::MainMessageLoopRun()
#46 0x7fc43d5794f9 content::BrowserMainLoop::RunMainMessageLoopParts()
#47 0x7fc43d582a85 content::BrowserMainRunnerImpl::Run()
#48 0x7fc43d573aa6 content::BrowserMain()
#49 0x7fc43f073436 content::RunNamedProcessTypeMain()
#50 0x7fc43f075435 content::ContentMainRunnerImpl::Run()
#51 0x7fc43f0726e2 content::ContentMain()
#52 0x7fc4448d360e ChromeMain
#53 0x7fc4448d3572 main
#54 0x7fc4316eef45 __libc_start_main
#55 0x7fc4448d3454 <unknown>

[Ben Goodger (Google), 2016-06-14 19:04:01]

Is there an equivalent in non-Mus chrome you can point to, e.g. where the GPU
channel get set up in that case?

[Peng, 2016-06-14 19:11:55]

On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> Is there an equivalent in non-Mus chrome you can point to, e.g. where the GPU
> channel get set up in that case?

https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...

Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
asynchronously. But for non-Mus chrome, it has to launch the gpu process,
so it is very slow and has to be async.

[Peng, 2016-06-14 19:27:54]

On 2016/06/14 19:11:55, Peng wrote:
> On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > Is there an equivalent in non-Mus chrome you can point to, e.g. where the
GPU
> > channel get set up in that case?
> 
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> 
> Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
> asynchronously. But for non-Mus chrome, it has to launch the gpu process,
> so it is very slow and has to be async.

[1]
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
[2]
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
[3]
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
[4]

BTW, we actually don't use hardware rendering in mus chrome. The chrome uses the
BitmapUploader to upload the pixels to mus surface with GL commands.
The BitmapUploader creates an offscreen GL context[1] which may need establish
gpu channel synchronously[2][3] if the channel is not exist. (It is hard to use
async method in BitmapUploader)

And I am also working on another CL https://codereview.chromium.org/2056833002.
It enables the HW rendering. With it, we will not need BitmapUploader anymore,
and we can
establish the gpu channel asynchronously in GpuProcessTransportFactory [4].

[jam, 2016-06-14 19:38:55]

On 2016/06/14 19:27:54, Peng wrote:
> On 2016/06/14 19:11:55, Peng wrote:
> > On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > > Is there an equivalent in non-Mus chrome you can point to, e.g. where the
> GPU
> > > channel get set up in that case?
> > 
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> > 
> > Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
> > asynchronously. But for non-Mus chrome, it has to launch the gpu process,
> > so it is very slow and has to be async.
> 
> [1]
>
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
> [2]
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> [3]
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> [4]
> 
> BTW, we actually don't use hardware rendering in mus chrome. The chrome uses
the
> BitmapUploader to upload the pixels to mus surface with GL commands.
> The BitmapUploader creates an offscreen GL context[1] which may need establish
> gpu channel synchronously[2][3] if the channel is not exist. (It is hard to
use
> async method in BitmapUploader)
> 
> And I am also working on another CL
https://codereview.chromium.org/2056833002.
> It enables the HW rendering. With it, we will not need BitmapUploader anymore,
> and we can
> establish the gpu channel asynchronously in GpuProcessTransportFactory [4].

if the non-mus case does it asynchronously, how come the mus case needs it
synchronously?

[jam, 2016-06-14 19:39:51]

On 2016/06/14 19:38:55, jam wrote:
> On 2016/06/14 19:27:54, Peng wrote:
> > On 2016/06/14 19:11:55, Peng wrote:
> > > On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > > > Is there an equivalent in non-Mus chrome you can point to, e.g. where
the
> > GPU
> > > > channel get set up in that case?
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> > > 
> > > Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
> > > asynchronously. But for non-Mus chrome, it has to launch the gpu process,
> > > so it is very slow and has to be async.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > [3]
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > [4]
> > 
> > BTW, we actually don't use hardware rendering in mus chrome. The chrome uses
> the
> > BitmapUploader to upload the pixels to mus surface with GL commands.
> > The BitmapUploader creates an offscreen GL context[1] which may need
establish
> > gpu channel synchronously[2][3] if the channel is not exist. (It is hard to
> use
> > async method in BitmapUploader)
> > 
> > And I am also working on another CL
> https://codereview.chromium.org/2056833002.
> > It enables the HW rendering. With it, we will not need BitmapUploader
anymore,
> > and we can
> > establish the gpu channel asynchronously in GpuProcessTransportFactory [4].
> 
> if the non-mus case does it asynchronously, how come the mus case needs it
> synchronously?

oh ok on rereading I think I understand, it's a problem with BitmapUploader.

Given that you have another solution that's close, I think we shouldn't land
this and should just wait for it.

[Peng, 2016-06-14 19:59:00]

On 2016/06/14 19:38:55, jam wrote:
> On 2016/06/14 19:27:54, Peng wrote:
> > On 2016/06/14 19:11:55, Peng wrote:
> > > On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > > > Is there an equivalent in non-Mus chrome you can point to, e.g. where
the
> > GPU
> > > > channel get set up in that case?
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> > > 
> > > Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
> > > asynchronously. But for non-Mus chrome, it has to launch the gpu process,
> > > so it is very slow and has to be async.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > [3]
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > [4]
> > 
> > BTW, we actually don't use hardware rendering in mus chrome. The chrome uses
> the
> > BitmapUploader to upload the pixels to mus surface with GL commands.
> > The BitmapUploader creates an offscreen GL context[1] which may need
establish
> > gpu channel synchronously[2][3] if the channel is not exist. (It is hard to
> use
> > async method in BitmapUploader)
> > 
> > And I am also working on another CL
> https://codereview.chromium.org/2056833002.
> > It enables the HW rendering. With it, we will not need BitmapUploader
anymore,
> > and we can
> > establish the gpu channel asynchronously in GpuProcessTransportFactory [4].
> 
> if the non-mus case does it asynchronously, how come the mus case needs it
> synchronously?

Non-mus uses different code path. It uses the BitmapUploader to do the SW
rendering.
And the BitmapUploader is not implemented with async in mind. It is hard to
change the
BitmapUploader.

But we will get rid of the sync mojo method call, when the HW rendering is
enabled with CL
https://codereview.chromium.org/2056833002 .

[Peng, 2016-06-14 20:29:38]

On 2016/06/14 19:59:00, Peng wrote:
> On 2016/06/14 19:38:55, jam wrote:
> > On 2016/06/14 19:27:54, Peng wrote:
> > > On 2016/06/14 19:11:55, Peng wrote:
> > > > On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > > > > Is there an equivalent in non-Mus chrome you can point to, e.g. where
> the
> > > GPU
> > > > > channel get set up in that case?
> > > > 
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> > > > 
> > > > Here is the an equivalent in non-Mus chrome. It establishes Gpu channel
> > > > asynchronously. But for non-Mus chrome, it has to launch the gpu
process,
> > > > so it is very slow and has to be async.
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
> > > [2]
> > >
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > > [3]
> > >
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > > [4]
> > > 
> > > BTW, we actually don't use hardware rendering in mus chrome. The chrome
uses
> > the
> > > BitmapUploader to upload the pixels to mus surface with GL commands.
> > > The BitmapUploader creates an offscreen GL context[1] which may need
> establish
> > > gpu channel synchronously[2][3] if the channel is not exist. (It is hard
to
> > use
> > > async method in BitmapUploader)
> > > 
> > > And I am also working on another CL
> > https://codereview.chromium.org/2056833002.
> > > It enables the HW rendering. With it, we will not need BitmapUploader
> anymore,
> > > and we can
> > > establish the gpu channel asynchronously in GpuProcessTransportFactory
[4].
> > 
> > if the non-mus case does it asynchronously, how come the mus case needs it
> > synchronously?
> 
> Non-mus uses different code path. It uses the BitmapUploader to do the SW
> rendering.
> And the BitmapUploader is not implemented with async in mind. It is hard to
> change the
> BitmapUploader.
> 
> But we will get rid of the sync mojo method call, when the HW rendering is
> enabled with CL
> https://codereview.chromium.org/2056833002 .

Discussed with jam offline. We agreed to land this CL as a temporary solution.
And we will
remove the ScopedAllowWait when the HW rendering is enabled in the mus chrome.

[jam, 2016-06-14 20:30:39]

Description was changed from

==========
Fix a crash in mus::GpuService

BUG=None
==========

to

==========
Fix a crash in mus::GpuService

BUG=620058
==========

[jam, 2016-06-14 20:31:02]

On 2016/06/14 20:29:38, Peng wrote:
> On 2016/06/14 19:59:00, Peng wrote:
> > On 2016/06/14 19:38:55, jam wrote:
> > > On 2016/06/14 19:27:54, Peng wrote:
> > > > On 2016/06/14 19:11:55, Peng wrote:
> > > > > On 2016/06/14 19:04:01, Ben Goodger (Google) wrote:
> > > > > > Is there an equivalent in non-Mus chrome you can point to, e.g.
where
> > the
> > > > GPU
> > > > > > channel get set up in that case?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_channel_...
> > > > > 
> > > > > Here is the an equivalent in non-Mus chrome. It establishes Gpu
channel
> > > > > asynchronously. But for non-Mus chrome, it has to launch the gpu
> process,
> > > > > so it is very slow and has to be async.
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/bitmap_uploader/bitmap_upload...
> > > > [2]
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > > > [3]
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/components/mus/public/cpp/lib/gles2_cont...
> > > > [4]
> > > > 
> > > > BTW, we actually don't use hardware rendering in mus chrome. The chrome
> uses
> > > the
> > > > BitmapUploader to upload the pixels to mus surface with GL commands.
> > > > The BitmapUploader creates an offscreen GL context[1] which may need
> > establish
> > > > gpu channel synchronously[2][3] if the channel is not exist. (It is hard
> to
> > > use
> > > > async method in BitmapUploader)
> > > > 
> > > > And I am also working on another CL
> > > https://codereview.chromium.org/2056833002.
> > > > It enables the HW rendering. With it, we will not need BitmapUploader
> > anymore,
> > > > and we can
> > > > establish the gpu channel asynchronously in GpuProcessTransportFactory
> [4].
> > > 
> > > if the non-mus case does it asynchronously, how come the mus case needs it
> > > synchronously?
> > 
> > Non-mus uses different code path. It uses the BitmapUploader to do the SW
> > rendering.
> > And the BitmapUploader is not implemented with async in mind. It is hard to
> > change the
> > BitmapUploader.
> > 
> > But we will get rid of the sync mojo method call, when the HW rendering is
> > enabled with CL
> > https://codereview.chromium.org/2056833002 .
> 
> Discussed with jam offline. We agreed to land this CL as a temporary solution.
> And we will
> remove the ScopedAllowWait when the HW rendering is enabled in the mus chrome.

lgtm, deferring to ben

[Peng, 2016-06-15 19:47:14]

Ben, kindly ping.

[Peng, 2016-06-15 19:47:20]

Ben, kindly ping.

[Ben Goodger (Google), 2016-06-15 20:04:11]

On 2016/06/15 19:47:20, Peng wrote:
> Ben, kindly ping.

lgtm

[Peng, 2016-06-16 01:14:24]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-06-16 01:14:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067833002/20001

[commit-bot: I haz the power, 2016-06-16 02:44:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-16 02:44:27]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Peng, 2016-06-16 09:11:14]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2016-06-16 09:11:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2067833002/20001

[commit-bot: I haz the power, 2016-06-16 09:50:01]

Description was changed from

==========
Fix a crash in mus::GpuService

BUG=620058
==========

to

==========
Fix a crash in mus::GpuService

BUG=620058
==========

[commit-bot: I haz the power, 2016-06-16 09:50:03]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-16 09:50:11]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 09:51:49]

Description was changed from

==========
Fix a crash in mus::GpuService

BUG=620058
==========

to

==========
Fix a crash in mus::GpuService

BUG=620058

Committed: https://crrev.com/ff6aef2ca17c3f114d52b5ba30b912d12f6c3a5b
Cr-Commit-Position: refs/heads/master@{#400118}
==========

[commit-bot: I haz the power, 2016-06-16 09:51:50]

Patchset 2 (id:??) landed as
https://crrev.com/ff6aef2ca17c3f114d52b5ba30b912d12f6c3a5b
Cr-Commit-Position: refs/heads/master@{#400118}