Forcibly clear worker ref counts on shutdown.

Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377, 608049
Committed: https://crrev.com/04a6912a1a2b77faf61e4b2514b9ecff74ee1f21
Cr-Commit-Position: refs/heads/master@{#420728}

[falken, 2016-09-14 05:41:57]

Description was changed from

==========
Forcibly clear worker ref counts on shutdown.

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it involves posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049
==========

to

==========
Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/ 

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049
==========

[falken, 2016-09-14 05:43:48]

falken@chromium.org changed reviewers:
+ horo@chromium.org

[falken, 2016-09-14 05:43:49]

horo PTAL

[falken, 2016-09-14 05:44:10]

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

[commit-bot: I haz the power, 2016-09-14 05:44:28]

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

[horo, 2016-09-14 06:31:57]

lgtm

[commit-bot: I haz the power, 2016-09-14 06:39:38]

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

[commit-bot: I haz the power, 2016-09-14 06:39:39]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-14 06:58:26]

falken@chromium.org changed reviewers:
+ jam@chromium.org

[falken, 2016-09-14 06:58:27]

jam: Can you please review?

[jam, 2016-09-14 17:10:50]

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:338: // on the UI thread anyway, so we can't
use the AllHostIterator.
how many tests is this for? can't we just add testbundles to them so that we
avoid adding this to production code? it is a mistake for this to be called on
non-UI thread for production code, so this statement can mask other bugs.

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:345: for
(content::RenderProcessHost::iterator host_iterator =
here and below, no "content::"

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1379:
is_worker_ref_count_disabled_ = true;
DCHECK(!is_worker_ref_count_disabled_); before this?

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:176: 
nit: undo this blank line (we don't add empty lines between overridden methods)

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
content/public/browser/render_process_host.h:316: virtual void
ForceReleaseWorkerRefCounts() = 0;
can you put these two methods on just RenderProcessHostImpl instead of the
public interface, if they're only called from content?

[alokp, 2016-09-16 04:47:52]

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

[alokp, 2016-09-16 04:47:53]

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1894: if
(worker_ref_count() != 0) {
should we still prevent fast shutdown for worker_ref_count > 0?

[falken, 2016-09-21 07:07:44]

Patchset #4 (id:60001) has been deleted

[falken, 2016-09-21 07:12:49]

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

[commit-bot: I haz the power, 2016-09-21 07:13: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-09-21 07:17:08]

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

[commit-bot: I haz the power, 2016-09-21 07:17:09]

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...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_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_...)

[falken, 2016-09-21 08:59:43]

Updated, PTAL. Sorry for the delay.

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:338: // on the UI thread anyway, so we can't
use the AllHostIterator.
On 2016/09/14 17:10:49, jam wrote:
> how many tests is this for? can't we just add testbundles to them so that we
> avoid adding this to production code? it is a mistake for this to be called on
> non-UI thread for production code, so this statement can mask other bugs.

Done, removed this and added testbundles.

https://codereview.chromium.org/2342523002/diff/1/content/browser/browser_con...
content/browser/browser_context.cc:345: for
(content::RenderProcessHost::iterator host_iterator =
On 2016/09/14 17:10:49, jam wrote:
> here and below, no "content::"

Done.

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1379:
is_worker_ref_count_disabled_ = true;
On 2016/09/14 17:10:49, jam wrote:
> DCHECK(!is_worker_ref_count_disabled_); before this?

Done.

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:1894: if
(worker_ref_count() != 0) {
On 2016/09/16 04:47:53, alokp wrote:
> should we still prevent fast shutdown for worker_ref_count > 0?

I think we still need to because fast shutdown won't necessarily lead to browser
shutdown. Other processes with unload handlers may be depending on a worker in
this process.

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/2342523002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:176: 
On 2016/09/14 17:10:49, jam wrote:
> nit: undo this blank line (we don't add empty lines between overridden
methods)

Who knew! Done.

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
content/public/browser/render_process_host.h:316: virtual void
ForceReleaseWorkerRefCounts() = 0;
On 2016/09/14 17:10:49, jam wrote:
> can you put these two methods on just RenderProcessHostImpl instead of the
> public interface, if they're only called from content?

I can't because the AllHostsIterator can include MockRenderProcessHosts, so they
can't be casted to RenderProcessHostImpl. Is there a way to tell the difference?

[falken, 2016-09-21 08:59:55]

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

[commit-bot: I haz the power, 2016-09-21 09:00:17]

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-09-21 09:48:54]

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

[commit-bot: I haz the power, 2016-09-21 09:48:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[falken, 2016-09-21 14:41:45]

Actually the review can wait, I need to fix some unit tests on other platforms
first.

[falken, 2016-09-21 15:01:58]

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

[commit-bot: I haz the power, 2016-09-21 15:02:26]

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

[falken, 2016-09-21 16:01:25]

Bots seem improved. PTAL.

https://codereview.chromium.org/2342523002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc
(right):

https://codereview.chromium.org/2342523002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/saml/saml_offline_signin_limiter_unittest.cc:109:
base::MessageLoop::current()->SetTaskRunner(runner_);
Question: Is it necessary to reverse this in TearDown()? (same question in other
tests) I figured it's not since the message loop should be tied to the thread
bundle which dies after the test.

[commit-bot: I haz the power, 2016-09-21 16:43:09]

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

[commit-bot: I haz the power, 2016-09-21 16:43:10]

Dry run: This issue passed the CQ dry run.

[falken, 2016-09-22 16:26:04]

jam: ping

[jam, 2016-09-23 19:14:28]

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/2342523002/diff/1/content/public/browser/rend...
content/public/browser/render_process_host.h:316: virtual void
ForceReleaseWorkerRefCounts() = 0;
On 2016/09/21 08:59:43, falken wrote:
> On 2016/09/14 17:10:49, jam wrote:
> > can you put these two methods on just RenderProcessHostImpl instead of the
> > public interface, if they're only called from content?
> 
> I can't because the AllHostsIterator can include MockRenderProcessHosts, so
they
> can't be casted to RenderProcessHostImpl. Is there a way to tell the
difference?

Is that code run during unittests though? i.e. we cast from RPH to RPHImpl for
code that we know it's run in unit tests.

[jam, 2016-09-23 19:15:52]

btw sorry for the delay, I missed this.

Please try putting this on the implementation only and not the public interface
first. if unit tests pass, then great. otherwise if that fails, the current
approach is fine. lgtm with that.

[falken, 2016-09-23 19:59:35]

Yep, the bad cast happened in unit tests. These tests were failing due to the
MockRenderProcessHosts:

  ChromeRuntimeAPIDelegateTest.RequestUpdateCheck
(../../chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:251)
    InstantSearchPrerendererTest.CanCommitQuery
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:269)
    InstantSearchPrerendererTest.CommitQuery
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:284)
    InstantSearchPrerendererTest.DoNotPrefetchSearchResults
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:257)
    InstantSearchPrerendererTest.PrefetchSearchResults
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:246)
    InstantSearchPrerendererTest.PrerenderingAllowed
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:313)
    ReuseInstantSearchBasePageTest.CanCommitQuery
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:442)
   
ReuseInstantSearchBasePageTest.CanCommitQuery_InstantSearchBasePageLoadInProgress
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:460)
    TestUsePrerenderPage.SetEmbeddedSearchRequestParams
(../../chrome/browser/ui/search/instant_search_prerenderer_unittest.cc:504)
    UpdateInstallGateTest.DelayInstallWhenInUse
(../../chrome/browser/extensions/update_install_gate_unittest.cc:263)
    UpdateInstallGateTest.InstallOnInstallImmediately
(../../chrome/browser/extensions/update_install_gate_unittest.cc:246)

[falken, 2016-09-23 19:59:56]

The CQ bit was checked by falken@chromium.org

[falken, 2016-09-23 19:59:57]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org,
jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2342523002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-23 20:00:27]

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-23 21:06:55]

Description was changed from

==========
Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/ 

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049
==========

to

==========
Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/ 

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049
==========

[commit-bot: I haz the power, 2016-09-23 21:06:57]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-09-23 21:10:56]

Description was changed from

==========
Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/ 

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049
==========

to

==========
Forcibly clear worker ref counts on shutdown.

Original patch author: horo@ at https://codereview.chromium.org/2248583003/

bug 608049 is a top crasher on stable channel. The cause is that render process
hosts are not reliably destructing on shutdown because shared workers keep them
alive.

Shared worker bookkeeping is expected to return the worker ref counts to zero
when documents close, but I suspect it often doesn't finish in time for
shutdown since it requires posting tasks between the IO thread and UI thread.
https://crrev.com/798a8b94726d3c439e6302998f31bc487f521ec8 tried to solve the
bug by removing waiting for an IPC from the renderer process, but the thread
hopping means shutdown is still racy.

This patch forcibly sets the ref counts to 0 on the UI thread when the browser
context is shutting down, similar to what we do for service workers.

BUG=636377,608049

Committed: https://crrev.com/04a6912a1a2b77faf61e4b2514b9ecff74ee1f21
Cr-Commit-Position: refs/heads/master@{#420728}
==========

[commit-bot: I haz the power, 2016-09-23 21:10:57]

Patchset 9 (id:??) landed as
https://crrev.com/04a6912a1a2b77faf61e4b2514b9ecff74ee1f21
Cr-Commit-Position: refs/heads/master@{#420728}