Introduce WorkerThread::willShutdown()

Introduce WorkerThread::willShutdown()

Currently WorkerThreadTest is calling GCs in WorkerThread::willDestroyIsolate()
but it is too late because the WorkerScriptController is already gone.
It seems it is not causing any practical issue for some reason but is not nice.
(It starts causing crashes once I land https://codereview.chromium.org/1328653002/.)

This CL introduces WorkerThread::willShutdown() so that WorkerThreadTest can run
GCs before the WorkerScriptController gets destructed.

BUG=

[haraken, 2015-09-03 01:50:37]

haraken@chromium.org changed reviewers:
+ kinuko@chromium.org, sadrul@chromium.org, toyoshim@chromium.org

[haraken, 2015-09-03 01:50:43]

PTAL

[kinuko, 2015-09-03 04:54:41]

Looks reasonable, but I'm confused by multiple GC methods.  Would you probably
help clarifying some of them?  I have two questions:

1. What's the difference between RequestGarbageCollectionForTesting() and
V8GCController::collectGarbage()?
When I tried to fix the leak I had a similar problem, calling
V8GCController::collectGarbage(isolate) crashed because it's too late, while
RequestGarbageCollectionForTesting worked fine. To me calling the latter seems
to have made sense because I wanted to fix leaks in tests so I did, but now
we're replacing it. Why V8GCController::collectGarbage() is preferred, and when
RequestGarbageCollectionForTesting should be used if that's the case?

2. Given that these GC methods need to be explicitly called for each isolate
before it goes away I'm not entirely sure if we don't need to call gc in
non-test cases, should it be ok?

[haraken, 2015-09-03 05:07:28]

On 2015/09/03 04:54:41, kinuko wrote:
> Looks reasonable, but I'm confused by multiple GC methods.  Would you probably
> help clarifying some of them?  I have two questions:
> 
> 1. What's the difference between RequestGarbageCollectionForTesting() and
> V8GCController::collectGarbage()?

There is no difference.

> When I tried to fix the leak I had a similar problem, calling
> V8GCController::collectGarbage(isolate) crashed because it's too late, while
> RequestGarbageCollectionForTesting worked fine.

This is just because V8GCController::collectGarbage tries to create a new
ScriptState before calling gc() (which internally calls
RequestGarbageCollectionForTesting). Creating a new ScriptState is not allowed
after the WorkerScriptController dies.

> To me calling the latter seems
> to have made sense because I wanted to fix leaks in tests so I did, but now
> we're replacing it. Why V8GCController::collectGarbage() is preferred, and
when
> RequestGarbageCollectionForTesting should be used if that's the case?

I want to unify them into V8GCController::collect{,All}Garbage(). It's hard to
understand/use V8 APIs correctly, so I want to limit the usage only in bindings/
as much as possible.

> 2. Given that these GC methods need to be explicitly called for each isolate
> before it goes away I'm not entirely sure if we don't need to call gc in
> non-test cases, should it be ok?

I hope it should be OK (otherwise, we'll already be leaking a lot of objects
after worker shutdown), but I don't know either how V8 GC is collecting V8
objects after shutting down the isolate.

[kinuko, 2015-09-03 05:34:36]

On 2015/09/03 05:07:28, haraken wrote:
> On 2015/09/03 04:54:41, kinuko wrote:
> > Looks reasonable, but I'm confused by multiple GC methods.  Would you
probably
> > help clarifying some of them?  I have two questions:
> > 
> > 1. What's the difference between RequestGarbageCollectionForTesting() and
> > V8GCController::collectGarbage()?
> 
> There is no difference.
> 
> > When I tried to fix the leak I had a similar problem, calling
> > V8GCController::collectGarbage(isolate) crashed because it's too late, while
> > RequestGarbageCollectionForTesting worked fine.
> 
> This is just because V8GCController::collectGarbage tries to create a new
> ScriptState before calling gc() (which internally calls
> RequestGarbageCollectionForTesting). Creating a new ScriptState is not allowed
> after the WorkerScriptController dies.

Yup, I could see that from the stack trace in the crash, and I'm puzzled why we
want to create a new script state for running GC after the test body finishes.

> > To me calling the latter seems
> > to have made sense because I wanted to fix leaks in tests so I did, but now
> > we're replacing it. Why V8GCController::collectGarbage() is preferred, and
> when
> > RequestGarbageCollectionForTesting should be used if that's the case?
> 
> I want to unify them into V8GCController::collect{,All}Garbage(). It's hard to
> understand/use V8 APIs correctly, so I want to limit the usage only in
bindings/
> as much as possible.

Looking in to the other CL collectAllGarbage() is called only in test code, if
that'll be the case would it make sense to suffix the method with 'ForTesting'
and call RequestGarbageCollectForTesting internally? If some method's added for
testing having a suffix like 'ForTesting' usually helps people understand (and
presubmit checks).

(Rephrasing this, I'm probably a bit concerned about adding yet another new
virtual method only for testing to already complex worker shutdown sequence)

> > 2. Given that these GC methods need to be explicitly called for each isolate
> > before it goes away I'm not entirely sure if we don't need to call gc in
> > non-test cases, should it be ok?
> 
> I hope it should be OK (otherwise, we'll already be leaking a lot of objects
> after worker shutdown), but I don't know either how V8 GC is collecting V8
> objects after shutting down the isolate.

Yup... that's my hope too, and match what the tests we have run seem to
indicate.

[haraken, 2015-09-03 05:45:47]

On 2015/09/03 05:34:36, kinuko wrote:
> On 2015/09/03 05:07:28, haraken wrote:
> > On 2015/09/03 04:54:41, kinuko wrote:
> > > Looks reasonable, but I'm confused by multiple GC methods.  Would you
> probably
> > > help clarifying some of them?  I have two questions:
> > > 
> > > 1. What's the difference between RequestGarbageCollectionForTesting() and
> > > V8GCController::collectGarbage()?
> > 
> > There is no difference.
> > 
> > > When I tried to fix the leak I had a similar problem, calling
> > > V8GCController::collectGarbage(isolate) crashed because it's too late,
while
> > > RequestGarbageCollectionForTesting worked fine.
> > 
> > This is just because V8GCController::collectGarbage tries to create a new
> > ScriptState before calling gc() (which internally calls
> > RequestGarbageCollectionForTesting). Creating a new ScriptState is not
allowed
> > after the WorkerScriptController dies.
> 
> Yup, I could see that from the stack trace in the crash, and I'm puzzled why
we
> want to create a new script state for running GC after the test body finishes.
> 
> > > To me calling the latter seems
> > > to have made sense because I wanted to fix leaks in tests so I did, but
now
> > > we're replacing it. Why V8GCController::collectGarbage() is preferred, and
> > when
> > > RequestGarbageCollectionForTesting should be used if that's the case?
> > 
> > I want to unify them into V8GCController::collect{,All}Garbage(). It's hard
to
> > understand/use V8 APIs correctly, so I want to limit the usage only in
> bindings/
> > as much as possible.
> 
> Looking in to the other CL collectAllGarbage() is called only in test code, if
> that'll be the case would it make sense to suffix the method with 'ForTesting'
> and call RequestGarbageCollectForTesting internally? If some method's added
for
> testing having a suffix like 'ForTesting' usually helps people understand (and
> presubmit checks).
> 
> (Rephrasing this, I'm probably a bit concerned about adding yet another new
> virtual method only for testing to already complex worker shutdown sequence)

Makes sense. I'll do that in the other CL (i.e., close this CL).

Although I can use RequestGarbageCollectForTesting in
V8GCController::collectAllGarbage, I can't use it in
V8GCController::collectGarbage because it's called by non-testing code in
content/ via WebFrameImpl. (I think we should remove those call sites.)

> 
> > > 2. Given that these GC methods need to be explicitly called for each
isolate
> > > before it goes away I'm not entirely sure if we don't need to call gc in
> > > non-test cases, should it be ok?
> > 
> > I hope it should be OK (otherwise, we'll already be leaking a lot of objects
> > after worker shutdown), but I don't know either how V8 GC is collecting V8
> > objects after shutting down the isolate.
> 
> Yup... that's my hope too, and match what the tests we have run seem to
> indicate.

[kinuko, 2015-09-03 05:52:35]

On 2015/09/03 05:45:47, haraken wrote:
> On 2015/09/03 05:34:36, kinuko wrote:
> > On 2015/09/03 05:07:28, haraken wrote:
> > > On 2015/09/03 04:54:41, kinuko wrote:
> > > > Looks reasonable, but I'm confused by multiple GC methods.  Would you
> > probably
> > > > help clarifying some of them?  I have two questions:
> > > > 
> > > > 1. What's the difference between RequestGarbageCollectionForTesting()
and
> > > > V8GCController::collectGarbage()?
> > > 
> > > There is no difference.
> > > 
> > > > When I tried to fix the leak I had a similar problem, calling
> > > > V8GCController::collectGarbage(isolate) crashed because it's too late,
> while
> > > > RequestGarbageCollectionForTesting worked fine.
> > > 
> > > This is just because V8GCController::collectGarbage tries to create a new
> > > ScriptState before calling gc() (which internally calls
> > > RequestGarbageCollectionForTesting). Creating a new ScriptState is not
> allowed
> > > after the WorkerScriptController dies.
> > 
> > Yup, I could see that from the stack trace in the crash, and I'm puzzled why
> we
> > want to create a new script state for running GC after the test body
finishes.
> > 
> > > > To me calling the latter seems
> > > > to have made sense because I wanted to fix leaks in tests so I did, but
> now
> > > > we're replacing it. Why V8GCController::collectGarbage() is preferred,
and
> > > when
> > > > RequestGarbageCollectionForTesting should be used if that's the case?
> > > 
> > > I want to unify them into V8GCController::collect{,All}Garbage(). It's
hard
> to
> > > understand/use V8 APIs correctly, so I want to limit the usage only in
> > bindings/
> > > as much as possible.
> > 
> > Looking in to the other CL collectAllGarbage() is called only in test code,
if
> > that'll be the case would it make sense to suffix the method with
'ForTesting'
> > and call RequestGarbageCollectForTesting internally? If some method's added
> for
> > testing having a suffix like 'ForTesting' usually helps people understand
(and
> > presubmit checks).
> > 
> > (Rephrasing this, I'm probably a bit concerned about adding yet another new
> > virtual method only for testing to already complex worker shutdown sequence)
> 
> Makes sense. I'll do that in the other CL (i.e., close this CL).

Thanks!

> Although I can use RequestGarbageCollectForTesting in
> V8GCController::collectAllGarbage, I can't use it in
> V8GCController::collectGarbage because it's called by non-testing code in
> content/ via WebFrameImpl. (I think we should remove those call sites.)

Yup, makes sense.