content/ui[Android]: Remove ContextProviderFactory.

content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This makes sure that the factory doesn't add any more
requests during shutdown.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2686243002
Cr-Commit-Position: refs/heads/master@{#453738}
Committed: https://chromium.googlesource.com/chromium/src/+/31690fb03a1ca682147c4e1f81a7d8479c88a233

[Khushal, 2017-02-10 04:06:18]

Description was changed from

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This adds an enum specifying the reason for failure to
return a valid host and checks that no requests are made while the
factory is shutting down.

BUG=684072
==========

to

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This adds an enum specifying the reason for failure to
return a valid host and checks that no requests are made while the
factory is shutting down.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Khushal, 2017-02-10 04:06:19]

Description was changed from

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This adds an enum specifying the reason for failure to
return a valid host and checks that no requests are made while the
factory is shutting down.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This adds an enum specifying the reason for failure to
return a valid host and checks that no requests are made while the
factory is shutting down.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Khushal, 2017-02-10 04:35:13]

Description was changed from

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This adds an enum specifying the reason for failure to
return a valid host and checks that no requests are made while the
factory is shutting down.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This makes sure that the factory doesn't add any more
requests during shutdown.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Khushal, 2017-02-10 04:44:26]

The CQ bit was checked by khushalsagar@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 04:44:43]

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

[commit-bot: I haz the power, 2017-02-10 07:02:30]

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

[commit-bot: I haz the power, 2017-02-10 07:02:31]

Dry run: This issue passed the CQ dry run.

[Khushal, 2017-02-10 18:04:42]

khushalsagar@chromium.org changed reviewers:
+ boliu@chromium.org, dtrainor@chromium.org, jbauman@chromium.org

[Khushal, 2017-02-10 18:04:43]

+boliu for content/ android.
+jbauman for content/gpu. Does the BrowserGpuChannelHostFactory change look
reasonable?
+dtrainor for ui/android.

[Khushal, 2017-02-10 18:06:34]

khushalsagar@chromium.org changed reviewers:
+ sky@chromium.org

[Khushal, 2017-02-10 18:06:35]

+sky for content/ test and services/

[Khushal, 2017-02-10 18:07:48]

khushalsagar@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Khushal, 2017-02-10 18:07:49]

+asvitkine for histograms.xml.

[Alexei Svitkine (slow), 2017-02-10 20:11:16]

Don't delete things from histograms.xml as they're still needed to decode data
from old versions.

Instead, you can mark them with <obsolete> tags, if desired.

[sky, 2017-02-10 21:07:58]

sky@chromium.org changed reviewers:
+ fsamuel@chromium.org

[sky, 2017-02-10 21:07:59]

LGTM - but wait for fady to review changes to services/ui/public/cpp/gpu/gpu.cc.

[David Trainor- moved to gerrit, 2017-02-10 21:59:15]

ui/android lgtm

[Fady Samuel, 2017-02-10 22:17:29]

lgtm

[boliu, 2017-02-10 22:23:02]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in the
callbacks?

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:81:
base::LazyInstance<cc::SurfaceManager> g_surface_manager =
all the global state should be shoved into a single struct, so they can all sit
in a single Lazyinstance?

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:353: return
cc::FrameSinkId(0 /* client_id */, g_next_sink_id++);
copy over the comment about being unique

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:592:
BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(base::Bind(
the gpu timeout got dropped afaict?

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:636: bool
support_locking = false;
nit: constexpr

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:659:
static_cast<ui::ContextProviderCommandBuffer*>(
just declare context_provider as the subtype, so don't need this cast?

https://codereview.chromium.org/2686243002/diff/60001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/test/test_rende...
content/test/test_render_view_host.cc:63: frame_sink_id_ =
AllocateFrameSinkId();
nit: indent

[Khushal, 2017-02-10 22:42:39]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 22:23:02, boliu wrote:
> Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in the
> callbacks?

Another thought was to set |instance_| to null in Terminate before destroying
it. But then you have to do a null check for the factory at all those call
sites. I don't know if that would be better.

Tbh, while writing this I figured we could simply skip running the callbacks in
the dtor, since that is effectively what is happening now with the return here.

We could also add that enum from ContextProviderFactory specifying the reason
for a null host to the GpuChannelEstablishFactory interface itself.

[boliu, 2017-02-10 22:45:18]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 22:42:39, Khushal wrote:
> On 2017/02/10 22:23:02, boliu wrote:
> > Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in the
> > callbacks?
> 
> Another thought was to set |instance_| to null in Terminate before destroying
> it. But then you have to do a null check for the factory at all those call
> sites. I don't know if that would be better.
> 
> Tbh, while writing this I figured we could simply skip running the callbacks
in
> the dtor, since that is effectively what is happening now with the return
here.
> 
> We could also add that enum from ContextProviderFactory specifying the reason
> for a null host to the GpuChannelEstablishFactory interface itself.

I'm actually worried about accidental infinite loops that's outside shutdown.

[Khushal, 2017-02-10 22:51:30]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 22:45:18, boliu wrote:
> On 2017/02/10 22:42:39, Khushal wrote:
> > On 2017/02/10 22:23:02, boliu wrote:
> > > Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in the
> > > callbacks?
> > 
> > Another thought was to set |instance_| to null in Terminate before
destroying
> > it. But then you have to do a null check for the factory at all those call
> > sites. I don't know if that would be better.
> > 
> > Tbh, while writing this I figured we could simply skip running the callbacks
> in
> > the dtor, since that is effectively what is happening now with the return
> here.
> > 
> > We could also add that enum from ContextProviderFactory specifying the
reason
> > for a null host to the GpuChannelEstablishFactory interface itself.
> 
> I'm actually worried about accidental infinite loops that's outside shutdown.

Maye be we should always post to the message loop here instead of running the
callback synchronously?

[boliu, 2017-02-10 23:05:22]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 22:51:30, Khushal wrote:
> On 2017/02/10 22:45:18, boliu wrote:
> > On 2017/02/10 22:42:39, Khushal wrote:
> > > On 2017/02/10 22:23:02, boliu wrote:
> > > > Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in
the
> > > > callbacks?
> > > 
> > > Another thought was to set |instance_| to null in Terminate before
> destroying
> > > it. But then you have to do a null check for the factory at all those call
> > > sites. I don't know if that would be better.
> > > 
> > > Tbh, while writing this I figured we could simply skip running the
callbacks
> > in
> > > the dtor, since that is effectively what is happening now with the return
> > here.
> > > 
> > > We could also add that enum from ContextProviderFactory specifying the
> reason
> > > for a null host to the GpuChannelEstablishFactory interface itself.
> > 
> > I'm actually worried about accidental infinite loops that's outside
shutdown.
> 
> Maye be we should always post to the message loop here instead of running the
> callback synchronously?

All depends on what happens in those callbacks today I guess.

This is cross-platform code, so you probably need to check a whole bunch of
places

[Khushal, 2017-02-10 23:24:30]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 23:05:22, boliu wrote:
> On 2017/02/10 22:51:30, Khushal wrote:
> > On 2017/02/10 22:45:18, boliu wrote:
> > > On 2017/02/10 22:42:39, Khushal wrote:
> > > > On 2017/02/10 22:23:02, boliu wrote:
> > > > > Hmm, I guess this is ok. But how hard is it to just ban re-entrancy in
> the
> > > > > callbacks?
> > > > 
> > > > Another thought was to set |instance_| to null in Terminate before
> > destroying
> > > > it. But then you have to do a null check for the factory at all those
call
> > > > sites. I don't know if that would be better.
> > > > 
> > > > Tbh, while writing this I figured we could simply skip running the
> callbacks
> > > in
> > > > the dtor, since that is effectively what is happening now with the
return
> > > here.
> > > > 
> > > > We could also add that enum from ContextProviderFactory specifying the
> > reason
> > > > for a null host to the GpuChannelEstablishFactory interface itself.
> > > 
> > > I'm actually worried about accidental infinite loops that's outside
> shutdown.
> > 
> > Maye be we should always post to the message loop here instead of running
the
> > callback synchronously?
> 
> All depends on what happens in those callbacks today I guess.
> 
> This is cross-platform code, so you probably need to check a whole bunch of
> places

I had actually looked around when thinking about putting an enum on the
callback. So there are 3 call-sites for it:

1) GpuProcessTransportFactory, which I think is used for context creation in the
browser on all platforms except android. Quick glance looks like we would make
limited attempts to initialize the Gpu process and then fallback to software.

2) CompositorImpl: Context creation for android and we don't fallback to
software. But I think in practice this re-entrancy has never been an issue at
shutdown because we stop making more requests after becoming invisible. For the
case where the initialization fails, I think we'll keep looping here.

3) VideoCaptureGpuJpegDecoder, which fails silently if a null host is given in
the callback.

So ideally most places should re-try a few times and then fallback to software
if they can, except during termination. If software is not possible, then it
should be fatal. Only android doesn't have a fallback path, so we need to know
whether to re-try or not in the callback. And may be even just crash after a
couple of attempts.

[Khushal, 2017-02-11 02:10:22]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/10 23:24:30, Khushal wrote:
> On 2017/02/10 23:05:22, boliu wrote:
> > On 2017/02/10 22:51:30, Khushal wrote:
> > > On 2017/02/10 22:45:18, boliu wrote:
> > > > On 2017/02/10 22:42:39, Khushal wrote:
> > > > > On 2017/02/10 22:23:02, boliu wrote:
> > > > > > Hmm, I guess this is ok. But how hard is it to just ban re-entrancy
in
> > the
> > > > > > callbacks?
> > > > > 
> > > > > Another thought was to set |instance_| to null in Terminate before
> > > destroying
> > > > > it. But then you have to do a null check for the factory at all those
> call
> > > > > sites. I don't know if that would be better.
> > > > > 
> > > > > Tbh, while writing this I figured we could simply skip running the
> > callbacks
> > > > in
> > > > > the dtor, since that is effectively what is happening now with the
> return
> > > > here.
> > > > > 
> > > > > We could also add that enum from ContextProviderFactory specifying the
> > > reason
> > > > > for a null host to the GpuChannelEstablishFactory interface itself.
> > > > 
> > > > I'm actually worried about accidental infinite loops that's outside
> > shutdown.
> > > 
> > > Maye be we should always post to the message loop here instead of running
> the
> > > callback synchronously?
> > 
> > All depends on what happens in those callbacks today I guess.
> > 
> > This is cross-platform code, so you probably need to check a whole bunch of
> > places
> 
> I had actually looked around when thinking about putting an enum on the
> callback. So there are 3 call-sites for it:
> 
> 1) GpuProcessTransportFactory, which I think is used for context creation in
the
> browser on all platforms except android. Quick glance looks like we would make
> limited attempts to initialize the Gpu process and then fallback to software.
> 
> 2) CompositorImpl: Context creation for android and we don't fallback to
> software. But I think in practice this re-entrancy has never been an issue at
> shutdown because we stop making more requests after becoming invisible. For
the
> case where the initialization fails, I think we'll keep looping here.
> 
> 3) VideoCaptureGpuJpegDecoder, which fails silently if a null host is given in
> the callback.
> 
> So ideally most places should re-try a few times and then fallback to software
> if they can, except during termination. If software is not possible, then it
> should be fatal. Only android doesn't have a fallback path, so we need to know
> whether to re-try or not in the callback. And may be even just crash after a
> couple of attempts.

So I think the simplest solution would be to just not run the callbacks in the
dtor, since we do it with a null host and the call-sites can't know if it is a
persistent failure. The cases where we don't fallback to software (android and
MUS?), consecutive failures like this would crash. I don't think there is any
code assuming that for every request the callback will always run anyway.

The calling code should ensure that it does not go into an infinite loop by
tolerating a limited number of consecutive failures and stop making requests
after that. Afaict, all call sites other than android are already doing this, so
we should change that in CompositorImpl. How does this sound?
+jbauman for gpu stuff.

[jbauman, 2017-02-11 02:13:52]

On 2017/02/11 02:10:22, Khushal wrote:
>
https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
> File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):
> 
>
https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
> content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
> On 2017/02/10 23:24:30, Khushal wrote:
> > On 2017/02/10 23:05:22, boliu wrote:
> > > On 2017/02/10 22:51:30, Khushal wrote:
> > > > On 2017/02/10 22:45:18, boliu wrote:
> > > > > On 2017/02/10 22:42:39, Khushal wrote:
> > > > > > On 2017/02/10 22:23:02, boliu wrote:
> > > > > > > Hmm, I guess this is ok. But how hard is it to just ban
re-entrancy
> in
> > > the
> > > > > > > callbacks?
> > > > > > 
> > > > > > Another thought was to set |instance_| to null in Terminate before
> > > > destroying
> > > > > > it. But then you have to do a null check for the factory at all
those
> > call
> > > > > > sites. I don't know if that would be better.
> > > > > > 
> > > > > > Tbh, while writing this I figured we could simply skip running the
> > > callbacks
> > > > > in
> > > > > > the dtor, since that is effectively what is happening now with the
> > return
> > > > > here.
> > > > > > 
> > > > > > We could also add that enum from ContextProviderFactory specifying
the
> > > > reason
> > > > > > for a null host to the GpuChannelEstablishFactory interface itself.
> > > > > 
> > > > > I'm actually worried about accidental infinite loops that's outside
> > > shutdown.
> > > > 
> > > > Maye be we should always post to the message loop here instead of
running
> > the
> > > > callback synchronously?
> > > 
> > > All depends on what happens in those callbacks today I guess.
> > > 
> > > This is cross-platform code, so you probably need to check a whole bunch
of
> > > places
> > 
> > I had actually looked around when thinking about putting an enum on the
> > callback. So there are 3 call-sites for it:
> > 
> > 1) GpuProcessTransportFactory, which I think is used for context creation in
> the
> > browser on all platforms except android. Quick glance looks like we would
make
> > limited attempts to initialize the Gpu process and then fallback to
software.
> > 
> > 2) CompositorImpl: Context creation for android and we don't fallback to
> > software. But I think in practice this re-entrancy has never been an issue
at
> > shutdown because we stop making more requests after becoming invisible. For
> the
> > case where the initialization fails, I think we'll keep looping here.
> > 
> > 3) VideoCaptureGpuJpegDecoder, which fails silently if a null host is given
in
> > the callback.
> > 
> > So ideally most places should re-try a few times and then fallback to
software
> > if they can, except during termination. If software is not possible, then it
> > should be fatal. Only android doesn't have a fallback path, so we need to
know
> > whether to re-try or not in the callback. And may be even just crash after a
> > couple of attempts.
> 
> So I think the simplest solution would be to just not run the callbacks in the
> dtor, since we do it with a null host and the call-sites can't know if it is a
> persistent failure. The cases where we don't fallback to software (android and
> MUS?), consecutive failures like this would crash. I don't think there is any
> code assuming that for every request the callback will always run anyway.

Yeah, if this is in the destructor then the browser's shutting down, so we don't
want the callbacks to actually do anything on failure.
> 
> The calling code should ensure that it does not go into an infinite loop by
> tolerating a limited number of consecutive failures and stop making requests
> after that. Afaict, all call sites other than android are already doing this,
so
> we should change that in CompositorImpl. How does this sound?
> +jbauman for gpu stuff.

This makes sense to me.

[Khushal, 2017-02-13 21:07:14]

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:290: if (in_shutdown_)
On 2017/02/11 02:10:22, Khushal wrote:
> On 2017/02/10 23:24:30, Khushal wrote:
> > On 2017/02/10 23:05:22, boliu wrote:
> > > On 2017/02/10 22:51:30, Khushal wrote:
> > > > On 2017/02/10 22:45:18, boliu wrote:
> > > > > On 2017/02/10 22:42:39, Khushal wrote:
> > > > > > On 2017/02/10 22:23:02, boliu wrote:
> > > > > > > Hmm, I guess this is ok. But how hard is it to just ban
re-entrancy
> in
> > > the
> > > > > > > callbacks?
> > > > > > 
> > > > > > Another thought was to set |instance_| to null in Terminate before
> > > > destroying
> > > > > > it. But then you have to do a null check for the factory at all
those
> > call
> > > > > > sites. I don't know if that would be better.
> > > > > > 
> > > > > > Tbh, while writing this I figured we could simply skip running the
> > > callbacks
> > > > > in
> > > > > > the dtor, since that is effectively what is happening now with the
> > return
> > > > > here.
> > > > > > 
> > > > > > We could also add that enum from ContextProviderFactory specifying
the
> > > > reason
> > > > > > for a null host to the GpuChannelEstablishFactory interface itself.
> > > > > 
> > > > > I'm actually worried about accidental infinite loops that's outside
> > > shutdown.
> > > > 
> > > > Maye be we should always post to the message loop here instead of
running
> > the
> > > > callback synchronously?
> > > 
> > > All depends on what happens in those callbacks today I guess.
> > > 
> > > This is cross-platform code, so you probably need to check a whole bunch
of
> > > places
> > 
> > I had actually looked around when thinking about putting an enum on the
> > callback. So there are 3 call-sites for it:
> > 
> > 1) GpuProcessTransportFactory, which I think is used for context creation in
> the
> > browser on all platforms except android. Quick glance looks like we would
make
> > limited attempts to initialize the Gpu process and then fallback to
software.
> > 
> > 2) CompositorImpl: Context creation for android and we don't fallback to
> > software. But I think in practice this re-entrancy has never been an issue
at
> > shutdown because we stop making more requests after becoming invisible. For
> the
> > case where the initialization fails, I think we'll keep looping here.
> > 
> > 3) VideoCaptureGpuJpegDecoder, which fails silently if a null host is given
in
> > the callback.
> > 
> > So ideally most places should re-try a few times and then fallback to
software
> > if they can, except during termination. If software is not possible, then it
> > should be fatal. Only android doesn't have a fallback path, so we need to
know
> > whether to re-try or not in the callback. And may be even just crash after a
> > couple of attempts.
> 
> So I think the simplest solution would be to just not run the callbacks in the
> dtor, since we do it with a null host and the call-sites can't know if it is a
> persistent failure. The cases where we don't fallback to software (android and
> MUS?), consecutive failures like this would crash. I don't think there is any
> code assuming that for every request the callback will always run anyway.
> 
> The calling code should ensure that it does not go into an infinite loop by
> tolerating a limited number of consecutive failures and stop making requests
> after that. Afaict, all call sites other than android are already doing this,
so
> we should change that in CompositorImpl. How does this sound?
> +jbauman for gpu stuff.

Done.

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:81:
base::LazyInstance<cc::SurfaceManager> g_surface_manager =
On 2017/02/10 22:23:02, boliu wrote:
> all the global state should be shoved into a single struct, so they can all
sit
> in a single Lazyinstance?

Good idea. Done.

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:353: return
cc::FrameSinkId(0 /* client_id */, g_next_sink_id++);
On 2017/02/10 22:23:02, boliu wrote:
> copy over the comment about being unique

Done.

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:592:
BrowserGpuChannelHostFactory::instance()->EstablishGpuChannel(base::Bind(
On 2017/02/10 22:23:02, boliu wrote:
> the gpu timeout got dropped afaict?

Oops, sorry. Added back here.

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:636: bool
support_locking = false;
On 2017/02/10 22:23:02, boliu wrote:
> nit: constexpr

Done.

https://codereview.chromium.org/2686243002/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:659:
static_cast<ui::ContextProviderCommandBuffer*>(
On 2017/02/10 22:23:02, boliu wrote:
> just declare context_provider as the subtype, so don't need this cast?

Done.

https://codereview.chromium.org/2686243002/diff/60001/content/test/test_rende...
File content/test/test_render_view_host.cc (right):

https://codereview.chromium.org/2686243002/diff/60001/content/test/test_rende...
content/test/test_render_view_host.cc:63: frame_sink_id_ =
AllocateFrameSinkId();
On 2017/02/10 22:23:02, boliu wrote:
> nit: indent

Done.

https://codereview.chromium.org/2686243002/diff/80001/services/ui/public/cpp/...
File services/ui/public/cpp/gpu/gpu.cc (left):

https://codereview.chromium.org/2686243002/diff/80001/services/ui/public/cpp/...
services/ui/public/cpp/gpu/gpu.cc:49: for (const auto& callback :
establish_callbacks_)
+fsamuel, do you want to take another look? Keeping this consistent with the
factory in content/.

https://codereview.chromium.org/2686243002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2686243002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:112579: +      label="BlimpCompositor
compositor command buffer context">
+asvitkine, does this look correct now?

[boliu, 2017-02-13 22:00:46]

https://codereview.chromium.org/2686243002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2686243002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:652: LOG_IF(FATAL,
++num_successive_gpu_initialization_failures_ > 2)
this is new..

if you want to add it, do it in a different CL?

[Khushal, 2017-02-13 23:49:51]

https://codereview.chromium.org/2686243002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2686243002/diff/80001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:652: LOG_IF(FATAL,
++num_successive_gpu_initialization_failures_ > 2)
On 2017/02/13 22:00:46, boliu wrote:
> this is new..
> 
> if you want to add it, do it in a different CL?

Sure. I'll put it in a separate patch.

[boliu, 2017-02-13 23:58:08]

lgtm

[Khushal, 2017-02-14 00:06:46]

asvitkine for histograms.xml.

jbauman to take a look at content/browser/gpu.

fsamuel for another look at services/ui/public/cpp/gpu/gpu.cc.

[jbauman, 2017-02-14 00:08:03]

lgtm

[Fady Samuel, 2017-02-14 00:30:22]

lgtm

[Alexei Svitkine (slow), 2017-02-14 01:40:52]

lgtm

[Khushal, 2017-02-14 03:14:22]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-14 03:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2686243002/#ps100001
(title: "bo's comment")

[commit-bot: I haz the power, 2017-02-14 03:15:02]

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

[commit-bot: I haz the power, 2017-02-14 03:50:27]

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

[commit-bot: I haz the power, 2017-02-14 03:50:32]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Khushal, 2017-02-14 06:00:19]

Removing the test which was checking that pending callbacks in
BrowserGpuChannelHostFactory are run on shutdown.

[Khushal, 2017-02-14 06:00:28]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-14 06:00:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org, boliu@chromium.org,
asvitkine@chromium.org, jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2686243002/#ps120001
(title: "test")

[commit-bot: I haz the power, 2017-02-14 06:00:48]

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

[commit-bot: I haz the power, 2017-02-14 07:34:42]

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

[commit-bot: I haz the power, 2017-02-14 07:34:44]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Khushal, 2017-02-28 19:26:01]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-28 19:26:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org, boliu@chromium.org,
asvitkine@chromium.org, jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2686243002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-28 19:26:50]

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

[commit-bot: I haz the power, 2017-02-28 19:30:33]

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

[commit-bot: I haz the power, 2017-02-28 19:30:36]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Khushal, 2017-02-28 19:48:49]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-02-28 19:48:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sky@chromium.org, boliu@chromium.org,
asvitkine@chromium.org, jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2686243002/#ps140002
(title: "more rebase")

[commit-bot: I haz the power, 2017-02-28 19:49:31]

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

[commit-bot: I haz the power, 2017-02-28 21:51:36]

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

[commit-bot: I haz the power, 2017-02-28 21:51:38]

Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Khushal, 2017-02-28 22:01:48]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2017-02-28 22:02:58]

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

[commit-bot: I haz the power, 2017-02-28 23:23:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140002, "attempt_start_ts": 1488319308748830,
"parent_rev": "1a04e1cf3d4912300d625a57318bc6f2ba53ba79", "commit_rev":
"31690fb03a1ca682147c4e1f81a7d8479c88a233"}

[commit-bot: I haz the power, 2017-02-28 23:24:35]

Description was changed from

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This makes sure that the factory doesn't add any more
requests during shutdown.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
content/ui[Android]: Remove ContextProviderFactory.

The class was added in ui/ to expose the compositor dependencies needed
by blimp, which is no longer necessary. The change removes the interface
and the implementation from content with the functionality added to
CompositorImpl.

Also, the logic for shutdown in GpuChannelEstablishFactory can currently
be reentrant, since the factory loops over all pending callbacks, and
running the callbacks with a null host can have the caller retry and add
to this list. This makes sure that the factory doesn't add any more
requests during shutdown.

BUG=684072
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2686243002
Cr-Commit-Position: refs/heads/master@{#453738}
Committed:
https://chromium.googlesource.com/chromium/src/+/31690fb03a1ca682147c4e1f81a7...
==========

[commit-bot: I haz the power, 2017-02-28 23:24:37]

Committed patchset #9 (id:140002) as
https://chromium.googlesource.com/chromium/src/+/31690fb03a1ca682147c4e1f81a7...