[Extension ServiceWorkers] Blink: Passing v8::context to extensions dispatcher

[Extension ServiceWorkers] Blink: Passing v8::context to extensions dispatcher

This patch is to address a reverted patch (https://codereview.chromium.org/1264913002/).  At line 253 in WorkerThread.cpp, the original check was script()->scriptState()->contextIsValid() which can crash if the scriptState is null.  This patch was reverted and is now changed to script()->isContextInitialized().  isContextInitialized was also changed from a private to a public function.

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

[annekao, 2015-08-12 18:12:54]

annekao@google.com changed reviewers:
+ falken@chromium.org, rdevlin.cronin@chromium.org

[annekao, 2015-08-12 18:33:40]

annekao@google.com changed reviewers:
+ dcheng@chromium.org, haraken@chromium.org

[falken, 2015-08-13 02:28:18]

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

[falken, 2015-08-13 02:28:19]

+kinuko
It'd be nice to have a test for the case where isContextInitialized returns
false, I don't see how to do it though. I'm not even sure how
isExecutionForbidden() could return true since we create the global scope and
script inside the mutex guarded section, so terminate() can't come in and kill
it.

Did you manually see what happens if script->initializeContextIfNeeded(); is
commented out?

[annekao, 2015-08-13 17:07:25]

On 2015/08/13 02:28:19, falken wrote:
> Did you manually see what happens if script->initializeContextIfNeeded(); is
> commented out?

The service worker still registers.
But, didInitializeWorkerContext() doesn't run.

[Devlin, 2015-08-13 17:40:11]

falken and kinuko know this better than me, so I'll let them handle it.  My only
nit would be that the CL description is rather long.

[falken, 2015-08-14 00:59:58]

OK lgtm. We'll think more how to test as part of
https://code.google.com/p/chromium/issues/detail?id=519111

[dcheng, 2015-08-14 01:10:27]

lgtm

[haraken, 2015-08-14 01:11:13]

bindings/ LGTM

[falken, 2015-08-14 01:19:03]

On 2015/08/13 17:07:25, annekao wrote:
> On 2015/08/13 02:28:19, falken wrote:
> > Did you manually see what happens if script->initializeContextIfNeeded(); is
> > commented out?
> 
> The service worker still registers.
> But, didInitializeWorkerContext() doesn't run.

Actually I think in the real situation the SW wouldn't register. The
isExecutionForbidden() inside evaluate() would cause evaluate to return false,
and we wouldn't register the worker. That sounds like the right behavior.

[kinuko, 2015-08-14 01:37:35]

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

https://codereview.chromium.org/1287103002/diff/20001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:211:
m_client.didInitializeWorkerContext(m_workerGlobalScope->script()->context(),
WebURL(m_document.url()));
KURL m_document.url() is not thread safe, I suspect we need to create an
isolated copy of the URL (by calling KURL::copy()) and store it in this proxy's
own field while we're on the main thread (i.e. in ctor), and use the isolated
copy here.

[annekao, 2015-08-14 19:43:49]

Patchset #3 (id:40001) has been deleted

[annekao, 2015-08-14 19:44:23]

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

https://codereview.chromium.org/1287103002/diff/20001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:211:
m_client.didInitializeWorkerContext(m_workerGlobalScope->script()->context(),
WebURL(m_document.url()));
On 2015/08/14 01:37:34, kinuko wrote:
> KURL m_document.url() is not thread safe, I suspect we need to create an
> isolated copy of the URL (by calling KURL::copy()) and store it in this
proxy's
> own field while we're on the main thread (i.e. in ctor), and use the isolated
> copy here.

Done.

[kinuko (google), 2015-08-17 10:27:19]

kinuko@google.com changed reviewers:
+ kinuko@google.com

[kinuko (google), 2015-08-17 10:27:20]

lgtm

https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.h:89: bool isContextInitialized()
{ return m_scriptState && !!m_scriptState->perContextData(); }
nit: this could probably be a const method

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

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:228: m_workerGlobalScope = 0;
It looks your patch's reverting newer changes, please make sure you only
apply/rebase your change on top of ToT

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.h:107: KURL m_url;
nit: I'd call this m_documentUrl and put it under m_document

[kinuko, 2015-08-17 10:54:58]

On 2015/08/17 10:27:20, kinuko (google) wrote:
> lgtm
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
> File Source/bindings/core/v8/WorkerScriptController.h (right):
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
> Source/bindings/core/v8/WorkerScriptController.h:89: bool
isContextInitialized()
> { return m_scriptState && !!m_scriptState->perContextData(); }
> nit: this could probably be a const method
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
> File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
> Source/web/ServiceWorkerGlobalScopeProxy.cpp:228: m_workerGlobalScope = 0;
> It looks your patch's reverting newer changes, please make sure you only
> apply/rebase your change on top of ToT
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
> File Source/web/ServiceWorkerGlobalScopeProxy.h (right):
> 
>
https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
> Source/web/ServiceWorkerGlobalScopeProxy.h:107: KURL m_url;
> nit: I'd call this m_documentUrl and put it under m_document

(oops... sent from wrong address)

[annekao, 2015-08-17 18:28:35]

Patchset #4 (id:80001) has been deleted

[annekao, 2015-08-17 19:47:41]

https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
File Source/bindings/core/v8/WorkerScriptController.h (right):

https://codereview.chromium.org/1287103002/diff/60001/Source/bindings/core/v8...
Source/bindings/core/v8/WorkerScriptController.h:89: bool isContextInitialized()
{ return m_scriptState && !!m_scriptState->perContextData(); }
On 2015/08/17 10:27:20, kinuko (google) wrote:
> nit: this could probably be a const method

Done.

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

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:228: m_workerGlobalScope = 0;
On 2015/08/17 10:27:20, kinuko (google) wrote:
> It looks your patch's reverting newer changes, please make sure you only
> apply/rebase your change on top of ToT

Done.

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1287103002/diff/60001/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.h:107: KURL m_url;
On 2015/08/17 10:27:20, kinuko (google) wrote:
> nit: I'd call this m_documentUrl and put it under m_document

Done.

[annekao, 2015-08-17 22:58:28]

The CQ bit was checked by annekao@google.com

[annekao, 2015-08-17 22:58:29]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
haraken@chromium.org, dcheng@chromium.org, kinuko@google.com
Link to the patchset: https://codereview.chromium.org/1287103002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-08-17 22:58:52]

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

[commit-bot: I haz the power, 2015-08-18 00:46:12]

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

[commit-bot: I haz the power, 2015-08-18 00:46:13]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[annekao, 2015-08-18 00:58:55]

The CQ bit was checked by annekao@google.com

[commit-bot: I haz the power, 2015-08-18 00:59:50]

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

[commit-bot: I haz the power, 2015-08-18 01:58:32]

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

[commit-bot: I haz the power, 2015-08-18 01:58:33]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[annekao, 2015-08-18 04:34:09]

The CQ bit was checked by annekao@google.com

[commit-bot: I haz the power, 2015-08-18 04:34:29]

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

[commit-bot: I haz the power, 2015-08-18 04:40:30]

Committed patchset #4 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200696