content: Fix Context creation logic in ContextProviderFactoryImpl.

content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So destroy the ContextProviderFactoryImpl
before destroying the BrowserGpuChannelHostFactory.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/becb5a6cf904faee4024a13e9760b71245e77b31
Cr-Commit-Position: refs/heads/master@{#415877}

[Khushal, 2016-08-16 20:20:31]

khushalsagar@chromium.org changed reviewers:
+ piman@chromium.org, sievers@chromium.org

[Khushal, 2016-08-16 20:20:37]

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

[commit-bot: I haz the power, 2016-08-16 20:21:05]

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, 2016-08-16 20:42:07]

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

[commit-bot: I haz the power, 2016-08-16 20:42:08]

Dry run: 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...)

[piman, 2016-08-16 22:04:52]

https://codereview.chromium.org/2250473005/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2250473005/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:220: delete instance_;
This would delete nullptr, i.e. leak - did you mean to keep intance_ into a
local variable and delete that?

[Khushal, 2016-08-16 22:11:58]

https://codereview.chromium.org/2250473005/diff/1/content/browser/gpu/browser...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/2250473005/diff/1/content/browser/gpu/browser...
content/browser/gpu/browser_gpu_channel_host_factory.cc:220: delete instance_;
On 2016/08/16 22:04:52, piman wrote:
> This would delete nullptr, i.e. leak - did you mean to keep intance_ into a
> local variable and delete that?

Umm, yes. :l Done.

[Khushal, 2016-08-16 22:13:05]

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

[commit-bot: I haz the power, 2016-08-16 22:13:25]

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

[piman, 2016-08-16 22:18:59]

LGTM for content/browser/gpu. I will let Daniel look at the Android-specific
side, because he understands it better.

[Khushal, 2016-08-16 22:45:58]

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

[Khushal, 2016-08-16 22:45:59]

Thanks piman!

+dtrainor for stamping ui/

[no sievers, 2016-08-17 18:30:10]

thanks!

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:52:
return factory->GetGpuChannel();
nit: This is maybe a bit verbose vs. just returning factory->GetGpuChannel().
Also maybe call the function GetGpuChannel() (the current name sounds like it
returns a bool)?

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:166: void
ContextProviderFactoryImpl::CheckCanHandlePendingRequests() {
can this outer logic be merged into HandlePendingRequests()?

https://codereview.chromium.org/2250473005/diff/40001/ui/android/context_prov...
File ui/android/context_provider_factory.h (right):

https://codereview.chromium.org/2250473005/diff/40001/ui/android/context_prov...
ui/android/context_provider_factory.h:40: FAILURE_NONE,
nit: how about |ContextCreationResult| and _SUCCESS here?

[Khushal, 2016-08-17 21:24:56]

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/context_provider_factory_impl_android.cc
(right):

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:52:
return factory->GetGpuChannel();
On 2016/08/17 18:30:10, sievers wrote:
> nit: This is maybe a bit verbose vs. just returning factory->GetGpuChannel().
> Also maybe call the function GetGpuChannel() (the current name sounds like it
> returns a bool)?

Yeah, that's better. Done.

https://codereview.chromium.org/2250473005/diff/40001/content/browser/rendere...
content/browser/renderer_host/context_provider_factory_impl_android.cc:166: void
ContextProviderFactoryImpl::CheckCanHandlePendingRequests() {
On 2016/08/17 18:30:10, sievers wrote:
> can this outer logic be merged into HandlePendingRequests()?

We call HandlePendingRequests from places where we know what the result is, for
instance when the GpuChannelHostFactory gives us a null host in the callback,
and we pass the result back to the caller who made the context request. I added
one to the dtor too so we clear out all the requests when the factory dies.

In CheckCanHandlePendingRequests, if we don't have a gpu channel already, we
would make a request to the factory to establish the channel instead.

https://codereview.chromium.org/2250473005/diff/40001/ui/android/context_prov...
File ui/android/context_provider_factory.h (right):

https://codereview.chromium.org/2250473005/diff/40001/ui/android/context_prov...
ui/android/context_provider_factory.h:40: FAILURE_NONE,
On 2016/08/17 18:30:10, sievers wrote:
> nit: how about |ContextCreationResult| and _SUCCESS here?

That's much better. Done.

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

https://codereview.chromium.org/2250473005/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:667: // Retry only if
we are visible.
Should there be a check for consecutive failures? The way we do for consecutive
failures to initialize the output surface.

[no sievers, 2016-08-17 21:34:39]

lgtm

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

https://codereview.chromium.org/2250473005/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:667: // Retry only if
we are visible.
On 2016/08/17 21:24:56, Khushal wrote:
> Should there be a check for consecutive failures? The way we do for
consecutive
> failures to initialize the output surface.

Yes, I think a LOG(FATAL) here would be good if this happens twice in a row or
so.

[Khushal, 2016-08-17 21:55:55]

Thanks Daniel.
Pingy to dtrainor@

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

https://codereview.chromium.org/2250473005/diff/60001/content/browser/rendere...
content/browser/renderer_host/compositor_impl_android.cc:667: // Retry only if
we are visible.
On 2016/08/17 21:34:39, sievers wrote:
> On 2016/08/17 21:24:56, Khushal wrote:
> > Should there be a check for consecutive failures? The way we do for
> consecutive
> > failures to initialize the output surface.
> 
> Yes, I think a LOG(FATAL) here would be good if this happens twice in a row or
> so.

Done.

[Khushal, 2016-08-17 22:07:16]

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

[commit-bot: I haz the power, 2016-08-17 22:07:40]

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, 2016-08-17 23:00:32]

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

[commit-bot: I haz the power, 2016-08-17 23:00:33]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2016-08-18 17:49:43]

lgtm!

https://codereview.chromium.org/2250473005/diff/80001/ui/android/context_prov...
File ui/android/context_provider_factory.h (right):

https://codereview.chromium.org/2250473005/diff/80001/ui/android/context_prov...
ui/android/context_provider_factory.h:37: // Used when the browser is shutting
down. No more requests should be made
Should we enforce/DCHECK that we don't get calls after sending this?

[Khushal, 2016-08-18 18:29:13]

https://codereview.chromium.org/2250473005/diff/80001/ui/android/context_prov...
File ui/android/context_provider_factory.h (right):

https://codereview.chromium.org/2250473005/diff/80001/ui/android/context_prov...
ui/android/context_provider_factory.h:37: // Used when the browser is shutting
down. No more requests should be made
On 2016/08/18 17:49:43, David Trainor wrote:
> Should we enforce/DCHECK that we don't get calls after sending this?

We can internally enforce that we are not getting any callbacks while we are
running the existing ones with a shutdown response. Should we also add a
DidShutdown() method to the factory that the caller should check before making a
request?

[Khushal, 2016-08-24 23:43:54]

I just changed the setup so the GpuChannelEstablishFactory always outlives the
ContextProviderFactoryImpl, and when you terminate it, it will let the callers
know that it is shutting down. That felt better than assuming that the browser
is shutting down when the factory is null.

sievers@, could you take a look again.

[Khushal, 2016-08-29 21:12:00]

On 2016/08/24 23:43:54, Khushal wrote:
> I just changed the setup so the GpuChannelEstablishFactory always outlives the
> ContextProviderFactoryImpl, and when you terminate it, it will let the callers
> know that it is shutting down. That felt better than assuming that the browser
> is shutting down when the factory is null.
> 
> sievers@, could you take a look again.

pingy to sievers. :)

[no sievers, 2016-08-30 20:51:00]

On 2016/08/29 21:12:00, Khushal wrote:
> On 2016/08/24 23:43:54, Khushal wrote:
> > I just changed the setup so the GpuChannelEstablishFactory always outlives
the
> > ContextProviderFactoryImpl, and when you terminate it, it will let the
callers
> > know that it is shutting down. That felt better than assuming that the
browser
> > is shutting down when the factory is null.
> > 
> > sievers@, could you take a look again.
> 
> pingy to sievers. :)

lgtm, thanks!

[Khushal, 2016-08-31 00:04:50]

Description was changed from

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So reset the instance_ before running
the pending callbacks and have callers cope with a null factory instance.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
==========

to

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So reset the instance_ before running
the pending callbacks and have callers cope with a null factory instance.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Khushal, 2016-08-31 00:06:15]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-31 00:06:16]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
dtrainor@chromium.org, sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/2250473005/#ps120001
(title: "rebase + test updates")

[commit-bot: I haz the power, 2016-08-31 00:06:46]

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, 2016-08-31 00:30:16]

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

[commit-bot: I haz the power, 2016-08-31 00:30:18]

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

[Khushal, 2016-08-31 17:30:25]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-31 17:30:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sievers@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2250473005/#ps140001
(title: "missing include")

[commit-bot: I haz the power, 2016-08-31 17:30:55]

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, 2016-08-31 19:13:55]

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

[commit-bot: I haz the power, 2016-08-31 19:13:57]

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

[Khushal, 2016-08-31 21:30:48]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-08-31 21:30:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sievers@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2250473005/#ps160001
(title: "missed one test harness")

[commit-bot: I haz the power, 2016-08-31 21:31:28]

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, 2016-08-31 21:39:32]

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

[commit-bot: I haz the power, 2016-08-31 21:39:33]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Khushal, 2016-08-31 21:40:52]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-08-31 21:41:29]

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, 2016-08-31 22:25:20]

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

[commit-bot: I haz the power, 2016-08-31 22:25:21]

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

[Khushal, 2016-08-31 22:32:11]

The CQ bit was checked by khushalsagar@chromium.org

[commit-bot: I haz the power, 2016-08-31 22:33:22]

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, 2016-08-31 23:17:56]

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

[commit-bot: I haz the power, 2016-08-31 23:17:57]

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

[Khushal, 2016-09-01 02:55:56]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2016-09-01 02:55:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, sievers@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2250473005/#ps180001
(title: "use mock factory")

[commit-bot: I haz the power, 2016-09-01 02:56:12]

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, 2016-09-01 04:09:50]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-01 04:35:06]

Description was changed from

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So reset the instance_ before running
the pending callbacks and have callers cope with a null factory instance.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So reset the instance_ before running
the pending callbacks and have callers cope with a null factory instance.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-09-01 04:35:07]

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

[Khushal, 2016-09-01 04:54:14]

Description was changed from

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So reset the instance_ before running
the pending callbacks and have callers cope with a null factory instance.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

to

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So destroy the ContextProviderFactoryImpl before
destroying the BrowserGpuChannelHostFactory.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[Khushal, 2016-09-01 04:54:37]

Description was changed from

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So destroy the ContextProviderFactoryImpl before
destroying the BrowserGpuChannelHostFactory.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

to

==========
content: Fix Context creation logic in ContextProviderFactoryImpl.

1) The BrowserGpuChannelHostFactory runs any pending callbacks during
shutdown. This can become re-entrant if the caller queued more requests
on getting a null GpuChannel. So destroy the ContextProviderFactoryImpl
before destroying the BrowserGpuChannelHostFactory.

2) Specify the reason for the context creation failure in the callback
from ContextProviderFactoryImpl so the callers can respond accordingly
and retry if necessary.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[Michael van Ouwerkerk, 2016-09-01 15:19:16]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2299183002/ by mvanouwerkerk@chromium.org.

The reason for reverting is:
https://bugs.chromium.org/p/chromium/issues/detail?id=643199

The following tests (and probably others) became flaky in the past 24 hours or
so:

BackgroundTracingManagerBrowserTest.ReactiveSecondTriggerIgnored
DownloadContentTest.ResumeRestoredDownload_ShortFile
DownloadContentTest.ResumeRestoredDownload_EtagMismatch
DragDownloadFileTest.DragDownloadFileTest_Complete
DragDownloadFileTest.DragDownloadFileTest_NetError
TracingControllerTest.NotWhitelistedMetadataStripped
UtilityProcessMojoClientBrowserTest.CallService
UtilityProcessMojoClientBrowserTest.ConnectionError
UtilityProcessHostImplBrowserTest.LaunchProcess


The common issue seems to be this DCHECK:
[FATAL:context_provider_factory_impl_android.cc(181)] Check failed:
!in_shutdown_. The factory is shutting down, can't handle new requests.

[weiliangc, 2016-09-01 17:53:02]

weiliangc@chromium.org changed reviewers:
+ weiliangc@chromium.org

[weiliangc, 2016-09-01 17:53:03]

Also responsible for failure on GPU.FYI Android bots gpu_process_launch_tests
GpuProcess.no_gpu_process.

[weiliangc, 2016-09-01 17:53:48]

weiliangc@chromium.org changed reviewers:
- weiliangc@chromium.org