LeakDetector: Terminate all WebEmbeddedWorkerImpl before counting

LeakDetector: Terminate all WebEmbeddedWorkerImpl before counting

Before this patch, the document held by ServiceWorker registered by
the LayoutTests were detected as leaks. The service worker is designed to live
longer than registerer document. The service worker implementation,
|WebEmbeddedWorkerImpl| carries a |WebFrame| to hold a resource loader.
The frame has its associating document, and it confused WebLeakDetector.

This patch work arounds the issue by terminating all |WebEmbeddedWorkerImpl|
instances before counting number of documents.

BUG=396493
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178996

[kouhei (in TOK), 2014-07-24 07:25:05]

Would you take a look?

[michaeln, 2014-07-25 00:07:09]

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.cpp
File Source/web/WebLeakDetector.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.c...
Source/web/WebLeakDetector.cpp:86: WebEmbeddedWorkerImpl::terminateAll();
i think it will take some time to unwind the background threads, do you need a
continuation (completioncallback) of some kind?

[kouhei (in TOK), 2014-07-25 01:33:10]

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.cpp
File Source/web/WebLeakDetector.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.c...
Source/web/WebLeakDetector.cpp:86: WebEmbeddedWorkerImpl::terminateAll();
On 2014/07/25 00:07:09, michaeln wrote:
> i think it will take some time to unwind the background threads, do you need a
> continuation (completioncallback) of some kind?

LeakDetector already flushes the task queue 2 times for oilpan GC, so I think
this is practically fine. Can I try landing as this first?

[michaeln, 2014-07-25 02:49:52]

https://codereview.chromium.org/413993002/diff/1/Source/web/WebEmbeddedWorker...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:149: static
HashSet<WebEmbeddedWorkerImpl*>& aliveWebEmbeddedWorkerImplInstances()
this is long'ish, runningWorkerInstances()?

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.cpp
File Source/web/WebLeakDetector.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.c...
Source/web/WebLeakDetector.cpp:86: WebEmbeddedWorkerImpl::terminateAll();
On 2014/07/25 01:33:09, kouhei wrote:
> On 2014/07/25 00:07:09, michaeln wrote:
> > i think it will take some time to unwind the background threads, do you need
a
> > continuation (completioncallback) of some kind?
> 
> LeakDetector already flushes the task queue 2 times for oilpan GC, so I think
> this is practically fine. Can I try landing as this first?

i doubt if the collection of running instances is empty in two turns of the
message loop, are you able to test prior to landing?

[kouhei (in TOK), 2014-07-25 05:00:39]

https://codereview.chromium.org/413993002/diff/1/Source/web/WebEmbeddedWorker...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebEmbeddedWorker...
Source/web/WebEmbeddedWorkerImpl.cpp:149: static
HashSet<WebEmbeddedWorkerImpl*>& aliveWebEmbeddedWorkerImplInstances()
On 2014/07/25 02:49:52, michaeln wrote:
> this is long'ish, runningWorkerInstances()?

Acknowledged.

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.cpp
File Source/web/WebLeakDetector.cpp (right):

https://codereview.chromium.org/413993002/diff/1/Source/web/WebLeakDetector.c...
Source/web/WebLeakDetector.cpp:86: WebEmbeddedWorkerImpl::terminateAll();
On 2014/07/25 02:49:52, michaeln wrote:
> On 2014/07/25 01:33:09, kouhei wrote:
> > On 2014/07/25 00:07:09, michaeln wrote:
> > > i think it will take some time to unwind the background threads, do you
need
> a
> > > continuation (completioncallback) of some kind?
> > 
> > LeakDetector already flushes the task queue 2 times for oilpan GC, so I
think
> > this is practically fine. Can I try landing as this first?
> 
> i doubt if the collection of running instances is empty in two turns of the
> message loop, are you able to test prior to landing?

Yes I confirmed that there are no false positive leaks after this patch is
applied.

[michaeln, 2014-07-25 19:54:00]

lgtm

https://codereview.chromium.org/413993002/diff/20001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/413993002/diff/20001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:60: static void terminateAll();
maybe add a comment about how upon return shutdown is in progress but the
threads are not joined

[kouhei (in TOK), 2014-07-28 00:34:21]

Thanks for review! I'll give this a try.

>
https://codereview.chromium.org/413993002/diff/20001/Source/web/WebEmbeddedWo...
> File Source/web/WebEmbeddedWorkerImpl.h (right):
> 
>
https://codereview.chromium.org/413993002/diff/20001/Source/web/WebEmbeddedWo...
> Source/web/WebEmbeddedWorkerImpl.h:60: static void terminateAll();
> maybe add a comment about how upon return shutdown is in progress but the
> threads are not joined

Done.

[kouhei (in TOK), 2014-07-28 00:34:45]

tkent: Would you do an owner review?

[tkent, 2014-07-28 00:42:48]

lgtm

https://codereview.chromium.org/413993002/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/413993002/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:61: // Note that this only schedules
termination and does not synchronously wait for it to complete.
nit:  I recommend to wrap code comments in 80 columns.

[kouhei (in TOK), 2014-07-28 00:44:33]

The CQ bit was checked by kouhei@chromium.org

[commit-bot: I haz the power, 2014-07-28 00:44:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kouhei@chromium.org/413993002/60001

[kouhei (in TOK), 2014-07-28 00:44:52]

Thanks for review! Comments wrapped.

[commit-bot: I haz the power, 2014-07-28 01:47:31]

Change committed as 178996