Pass the v8::Context through the willDestroyServiceWorkerContextOnWorkerThread event.

Pass the v8::Context through the willDestroyServiceWorkerContextOnWorkerThread event.

Currently it only passes a URL, and this isn't enough to uniquely identify the worker.

BUG=501569
R=falken@chromium.org, adamk@chromium.org

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

[not at google - send to devlin, 2015-08-26 01:18:01]

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
File public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
public/web/modules/serviceworker/WebServiceWorkerContextClient.h:83: //
TODO(kalman): Finish migration to make only the version which takes a
I haven't done a patch like this in a long time, is this the idiomatic way to do
it?

[not at google - send to devlin, 2015-08-26 01:18:07]

Chromium side is https://codereview.chromium.org/1318643002/.

[falken, 2015-08-26 08:47:30]

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

[falken, 2015-08-26 08:47:30]

https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:228:
m_client.willDestroyWorkerContext(m_workerGlobalScope->script()->context());
This looks risky, WorkerThread::shutdown() does:

workerGlobalScope()->dispose();
workerReportingProxy().willDestroyWorkerGlobalScope();

dispose() destroys the script, so this might crash..?

I must admit I just read the code every time to understand the startup/shutdown
sequence. kinuko@ may have more insights.

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
File public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
public/web/modules/serviceworker/WebServiceWorkerContextClient.h:83: //
TODO(kalman): Finish migration to make only the version which takes a
On 2015/08/26 01:18:01, kalman wrote:
> I haven't done a patch like this in a long time, is this the idiomatic way to
do
> it?

Seems fine. I usually just say "Remove after chromium-side patch lands."

[kinuko, 2015-08-26 09:05:01]

On 2015/08/26 08:47:30, falken wrote:
>
https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
> File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):
> 
>
https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
> Source/web/ServiceWorkerGlobalScopeProxy.cpp:228:
> m_client.willDestroyWorkerContext(m_workerGlobalScope->script()->context());
> This looks risky, WorkerThread::shutdown() does:
> 
> workerGlobalScope()->dispose();
> workerReportingProxy().willDestroyWorkerGlobalScope();
> 
> dispose() destroys the script, so this might crash..?
> 
> I must admit I just read the code every time to understand the
startup/shutdown
> sequence. kinuko@ may have more insights.

I just added a related comment on chromium side patch.

>
https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
> File public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right):
> 
>
https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
> public/web/modules/serviceworker/WebServiceWorkerContextClient.h:83: //
> TODO(kalman): Finish migration to make only the version which takes a
> On 2015/08/26 01:18:01, kalman wrote:
> > I haven't done a patch like this in a long time, is this the idiomatic way
to
> do
> > it?
> 
> Seems fine. I usually just say "Remove after chromium-side patch lands."

[not at google - send to devlin, 2015-08-26 17:33:54]

https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1310843006/diff/1/Source/web/ServiceWorkerGlo...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:228:
m_client.willDestroyWorkerContext(m_workerGlobalScope->script()->context());
On 2015/08/26 08:47:30, falken wrote:
> This looks risky, WorkerThread::shutdown() does:
> 
> workerGlobalScope()->dispose();
> workerReportingProxy().willDestroyWorkerGlobalScope();
> 
> dispose() destroys the script, so this might crash..?
> 
> I must admit I just read the code every time to understand the
startup/shutdown
> sequence. kinuko@ may have more insights.

Yep, good find. I never thought to really test this with manually stopping the
service worker :\ I've updated the patch to make this work. See annotations.

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
File public/web/modules/serviceworker/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/1310843006/diff/1/public/web/modules/servicew...
public/web/modules/serviceworker/WebServiceWorkerContextClient.h:83: //
TODO(kalman): Finish migration to make only the version which takes a
On 2015/08/26 08:47:30, falken wrote:
> On 2015/08/26 01:18:01, kalman wrote:
> > I haven't done a patch like this in a long time, is this the idiomatic way
to
> do
> > it?
> 
> Seems fine. I usually just say "Remove after chromium-side patch lands."

Done.

https://codereview.chromium.org/1310843006/diff/20001/Source/core/workers/Wor...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1310843006/diff/20001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:280: workerGlobalScope()->dispose();
As you pointed out, this looks wrong. The comment above that says "this should
be called before we start the shutdown procedure" seems to contradict the code
before I changed it.

https://codereview.chromium.org/1310843006/diff/20001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1310843006/diff/20001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:229: v8::HandleScope
handleScope(m_workerGlobalScope->thread()->isolate());
Oops.

[not at google - send to devlin, 2015-08-27 16:08:48]

ping?

[falken, 2015-08-28 01:54:21]

lgtm, was holding off in case kinuko's question on the chromium patch resulted
in changes, but seems like it's not

[not at google - send to devlin, 2015-08-28 16:35:40]

kalman@chromium.org changed reviewers:
+ adamk@chromium.org
- japhet@chromium.org, kinuko@chromium.org

[not at google - send to devlin, 2015-08-28 16:36:00]

-falken +adamk as reviewer for files in Source/

[not at google - send to devlin, 2015-08-28 16:36:22]

-japhet, I mean. Hi falken :-)

[adamk, 2015-08-28 18:35:26]

What does the Chromium side of this look like? That is, what's going to be done
with the v8::Local<v8::Context>?

Also, you'll actually need someone else to review the public/ part...

[not at google - send to devlin, 2015-08-28 20:47:08]

On 2015/08/28 18:35:26, adamk wrote:
> What does the Chromium side of this look like? That is, what's going to be
done
> with the v8::Local<v8::Context>?
> 
> Also, you'll actually need someone else to review the public/ part...

falken is an owner of that, and there is a link to the chromium side in a
previous patchset (https://codereview.chromium.org/1318643002/).

[adamk, 2015-08-28 22:00:26]

Ah, thanks, lgtm. Looks like this doesn't change anything about the lifetime of
v8::Contexts held on the Chromium side (since they were already held as part of
the map).

[not at google - send to devlin, 2015-08-28 22:01:34]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-08-28 22:01:52]

On 2015/08/28 22:00:26, adamk wrote:
> Ah, thanks, lgtm. Looks like this doesn't change anything about the lifetime
of
> v8::Contexts held on the Chromium side (since they were already held as part
of
> the map).

Yep, and in fact in order to correctly manage their lifetime I need this.

[commit-bot: I haz the power, 2015-08-28 22:02:06]

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

[commit-bot: I haz the power, 2015-08-29 00:01:29]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201440