Check that all shared workers are terminated.

Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049
Committed: https://crrev.com/529173d1bbeb53b3f9438648fad264b4db042721
Cr-Commit-Position: refs/heads/master@{#410730}

[alokp, 2016-08-08 18:20:02]

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

[commit-bot: I haz the power, 2016-08-08 18:20:24]

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-08 18:37:59]

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

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

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

[alokp, 2016-08-08 21:37:09]

Description was changed from

==========
Check that all shared workers are terminated.

BUG=608049
==========

to

==========
Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049
==========

[alokp, 2016-08-08 21:39:15]

alokp@chromium.org changed reviewers:
+ atwilson@chromium.org, horo@chromium.org, jam@chromium.org

[alokp, 2016-08-08 21:39:16]

atwilson/horo: shared_worker
jam: content

[horo, 2016-08-09 04:03:24]

lgtm with nits

https://codereview.chromium.org/2222063002/diff/1/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2222063002/diff/1/content/browser/browser_mai...
content/browser/browser_main_loop.cc:1109: // Temporary checks to verify that
all shared workers are terminated.
Nit: s/shared workers/shared workers and service workers/

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
checks to verify that all shared workers are terminated.
Nit: s/shared workers/shared workers and service workers/

|worker_ref_count_| is used for both shared workers and service workers.

So RenderProcessHostImpl::CheckAllWorkersTerminate() is verifying that all
shared workers and service workers are terminated.

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:248: // Temporary
checks to verify that all shared workers are terminated.
Nit: s/shared workers/shared workers and service workers/

[alokp, 2016-08-09 05:07:26]

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
checks to verify that all shared workers are terminated.
On 2016/08/09 04:03:24, horo wrote:
> Nit: s/shared workers/shared workers and service workers/
> 
> |worker_ref_count_| is used for both shared workers and service workers.
> 
> So RenderProcessHostImpl::CheckAllWorkersTerminate() is verifying that all
> shared workers and service workers are terminated.

Thanks for reviewing. I just commented about shared worker because in
BrowserProcessSubThread::IOThreadPreCleanUp, I am only checking shared workers.
Do you suggest a corresponding check for service worker as well?

[horo, 2016-08-09 06:01:26]

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
checks to verify that all shared workers are terminated.
On 2016/08/09 05:07:26, alokp wrote:
> On 2016/08/09 04:03:24, horo wrote:
> > Nit: s/shared workers/shared workers and service workers/
> > 
> > |worker_ref_count_| is used for both shared workers and service workers.
> > 
> > So RenderProcessHostImpl::CheckAllWorkersTerminate() is verifying that all
> > shared workers and service workers are terminated.
> 
> Thanks for reviewing. I just commented about shared worker because in
> BrowserProcessSubThread::IOThreadPreCleanUp, I am only checking shared
workers.
> Do you suggest a corresponding check for service worker as well?

I just suggested that you should change the comment of
RenderProcessHostImpl::CheckAllWorkersTerminated().

But...

Could you please explain why you want to check that all shared workers are
terminated for the bug?
This bug 608049 is caused by unclosed AudioOutputStream instance.

I thought that it is because shared workers can open AudioOutputStream.
But AudioContextConstructor is exposed only in window.
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/webaud...
So I think shared workers can't open AudioOutputStream.
And also, service workers can't open AudioOutputStream.

Is my understanding correct?

[alokp, 2016-08-09 06:35:27]

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
checks to verify that all shared workers are terminated.
 
> Could you please explain why you want to check that all shared workers are
> terminated for the bug?
> This bug 608049 is caused by unclosed AudioOutputStream instance.

For more context see crbug.com/628192

All audio streams will get closed if RenderProcessHostImpl::Cleanup is called.
In the past I added a few checks here https://codereview.chromium.org/2133813002
and discovered that worker_ref_count_ > 0 is preventing RenderProcessHostImpl(s)
from getting cleaned up. This patch seeks to further understand why
shared/service workers are not getting cleaned up.

I am not familiar with shared/service workers. I would appreciate any help with
diagnosing it further.

[horo, 2016-08-09 09:00:52]

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
checks to verify that all shared workers are terminated.
On 2016/08/09 06:35:27, alokp wrote:
>  
> > Could you please explain why you want to check that all shared workers are
> > terminated for the bug?
> > This bug 608049 is caused by unclosed AudioOutputStream instance.
> 
> For more context see crbug.com/628192
> 
> All audio streams will get closed if RenderProcessHostImpl::Cleanup is called.
> In the past I added a few checks here
https://codereview.chromium.org/2133813002
> and discovered that worker_ref_count_ > 0 is preventing
RenderProcessHostImpl(s)
> from getting cleaned up. This patch seeks to further understand why
> shared/service workers are not getting cleaned up.
> 
> I am not familiar with shared/service workers. I would appreciate any help
with
> diagnosing it further.

Ah I got it.

I checked ServiceWorkerProcessManager code now.
And I think ServiceWorkerProcessManager::Shutdown() is correctly calling
DecrementWorkerRefCount.

BrowserMainRunnerImpl::Shutdown()
-> BrowserMainLoop::ShutdownThreadsAndCleanUp()
 -> ChromeBrowserMainParts::PostMainMessageLoopRun()
  -> BrowserProcessImpl::StartTearDown()
   -> ProfileManager::~ProfileManager()
    -> ProfileInfo::~ProfileInfo()
     -> ProfileDestroyer::DestroyProfileWhenAppropriate()
      -> Profile::MaybeSendDestroyedNotification()
       -> BrowserContext::NotifyWillBeDestroyed()
        -> BrowserContext::ForEachStoragePartition()
         -> ShutdownServiceWorkerContext()
          -> ServiceWorkerProcessManager::Shutdown()
           -> RenderProcessHostImpl::DecrementWorkerRefCount()

But there may be edge cases that SharedWorkerServiceImpl doesn't correctly call
DecrementWorkerRefCount.

[jam, 2016-08-09 16:55:06]

lgtm

[alokp, 2016-08-09 16:58:44]

On 2016/08/09 09:00:52, horo wrote:
>
https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_process_host_impl.cc:717: // Temporary
> checks to verify that all shared workers are terminated.
> On 2016/08/09 06:35:27, alokp wrote:
> >  
> > > Could you please explain why you want to check that all shared workers are
> > > terminated for the bug?
> > > This bug 608049 is caused by unclosed AudioOutputStream instance.
> > 
> > For more context see crbug.com/628192
> > 
> > All audio streams will get closed if RenderProcessHostImpl::Cleanup is
called.
> > In the past I added a few checks here
> https://codereview.chromium.org/2133813002
> > and discovered that worker_ref_count_ > 0 is preventing
> RenderProcessHostImpl(s)
> > from getting cleaned up. This patch seeks to further understand why
> > shared/service workers are not getting cleaned up.
> > 
> > I am not familiar with shared/service workers. I would appreciate any help
> with
> > diagnosing it further.
> 
> Ah I got it.
> 
> I checked ServiceWorkerProcessManager code now.
> And I think ServiceWorkerProcessManager::Shutdown() is correctly calling
> DecrementWorkerRefCount.
> 
> BrowserMainRunnerImpl::Shutdown()
> -> BrowserMainLoop::ShutdownThreadsAndCleanUp()
>  -> ChromeBrowserMainParts::PostMainMessageLoopRun()
>   -> BrowserProcessImpl::StartTearDown()
>    -> ProfileManager::~ProfileManager()
>     -> ProfileInfo::~ProfileInfo()
>      -> ProfileDestroyer::DestroyProfileWhenAppropriate()
>       -> Profile::MaybeSendDestroyedNotification()
>        -> BrowserContext::NotifyWillBeDestroyed()
>         -> BrowserContext::ForEachStoragePartition()
>          -> ShutdownServiceWorkerContext()
>           -> ServiceWorkerProcessManager::Shutdown()
>            -> RenderProcessHostImpl::DecrementWorkerRefCount()
> 
> But there may be edge cases that SharedWorkerServiceImpl doesn't correctly
call
> DecrementWorkerRefCount.

That or DecrementWorkerRefCount tasks posted to the UI thread do not get a
chance to run. We should be able to find out with this patch.

[alokp, 2016-08-09 16:58:56]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-08-09 16:59:30]

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-09 17:40:20]

Description was changed from

==========
Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049
==========

to

==========
Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049
==========

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

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-09 17:43:22]

Description was changed from

==========
Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049
==========

to

==========
Check that all shared workers are terminated.

Adds temporary checks to determine whether shared workers
prevent RenderProcessHost(s) from cleaning up due to
RenderProcessHostImpl::worker_ref_count_ > 0.

BUG=608049

Committed: https://crrev.com/529173d1bbeb53b3f9438648fad264b4db042721
Cr-Commit-Position: refs/heads/master@{#410730}
==========

[commit-bot: I haz the power, 2016-08-09 17:43:23]

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

[manzagop (departed), 2016-08-17 20:13:15]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2254963002/ by manzagop@chromium.org.

The reason for reverting is: Reverting temporary instrumentation following
investigation

We want to get this in prior to dev branch.

BUG=636377.

[falken, 2016-09-02 07:47:44]

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

[falken, 2016-09-02 07:47:45]

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

https://codereview.chromium.org/2222063002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:725: CHECK_EQ(0,
host->worker_ref_count_);
This would infinitely loop, no? iter.Advance() should be called.