Force running GC on the worker thread before the unit test exits

Force running GC on the worker thread before the unit test exits

For the main thread we seem to always run
v8::Isolate::RequestGarbageCollectionForTesting but it's
not the case on the worker thread, which seems to cause leaks
on the context object.

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

[kinuko, 2015-09-01 13:57:21]

kinuko@chromium.org changed reviewers:
+ haraken@chromium.org

[kinuko, 2015-09-01 13:59:00]

PTL. Does this make sense?

[haraken, 2015-09-01 14:28:12]

On 2015/09/01 13:59:00, kinuko wrote:
> PTL. Does this make sense?

Does this really resolve the leak?

I'm suspecting that ScriptState::disposePerContextData() is not explicitly
called in the workers. Without calling ScriptState::disposePerContextData(), the
context will leak.

[kinuko, 2015-09-01 14:50:10]

On 2015/09/01 14:28:12, haraken wrote:
> On 2015/09/01 13:59:00, kinuko wrote:
> > PTL. Does this make sense?
> 
> Does this really resolve the leak?
> 
> I'm suspecting that ScriptState::disposePerContextData() is not explicitly
> called in the workers. Without calling ScriptState::disposePerContextData(),
the
> context will leak.

I first suspected the same, but in my very sloppy printf debugging it seems to
be called, but it doesn't end up with calling the context's weak callback, which
is necessary to free the ScriptState.  I haven't run this with LSan but with
this patch I could at least observe that the ScriptState gets freed.

[haraken, 2015-09-01 23:41:33]

> For the main thread we seem to always run
v8::Isolate::RequestGarbageCollectionForTesting

^^^ Would you help me find the code that is doing this?

[kinuko, 2015-09-02 01:00:45]

On 2015/09/01 23:41:33, haraken wrote:
> > For the main thread we seem to always run
> v8::Isolate::RequestGarbageCollectionForTesting
> 
> ^^^ Would you help me find the code that is doing this?

This is the one I spotted. (Seems to run once at the very end of the tests)
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[haraken, 2015-09-02 02:11:14]

On 2015/09/02 01:00:45, kinuko wrote:
> On 2015/09/01 23:41:33, haraken wrote:
> > > For the main thread we seem to always run
> > v8::Isolate::RequestGarbageCollectionForTesting
> > 
> > ^^^ Would you help me find the code that is doing this?
> 
> This is the one I spotted. (Seems to run once at the very end of the tests)
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Thanks. I don't fully understand how this CL fixes the context leak but I think
it's a good idea to have RequestGarbageCollectionForTesting at the end of the
worker test. Maybe it's better to create a helper method to encapsulate the
shutdown logic for worker tests somewhere though (just like runHelper in
WebUnitTests).

LGTM.

[kinuko, 2015-09-02 02:47:47]

On 2015/09/02 02:11:14, haraken wrote:
> On 2015/09/02 01:00:45, kinuko wrote:
> > On 2015/09/01 23:41:33, haraken wrote:
> > > > For the main thread we seem to always run
> > > v8::Isolate::RequestGarbageCollectionForTesting
> > > 
> > > ^^^ Would you help me find the code that is doing this?
> > 
> > This is the one I spotted. (Seems to run once at the very end of the tests)
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Thanks. I don't fully understand how this CL fixes the context leak but I
think
> it's a good idea to have RequestGarbageCollectionForTesting at the end of the
> worker test. Maybe it's better to create a helper method to encapsulate the
> shutdown logic for worker tests somewhere though (just like runHelper in
> WebUnitTests).
> 
> LGTM.

Thanks. I agree that it'be nicer to have a helper here. Let me land this as is
first to see if this really fixes the LSAN issue.

[kinuko, 2015-09-02 02:48:06]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2015-09-02 02:48:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1316233004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1316233004/20001

[commit-bot: I haz the power, 2015-09-02 02:52:59]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201601