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

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

Pass the v8::context from the worker thread up to the extensions dispatcher.  

In order to expose any kind of custom binding (i.e., javascript callback into C++ code) for the service worker context, we need to be alerted of when the context is created, so that we can inject them.  For most contexts, this is done through the blink notification (didCreateScriptContext), but that doesn't cover
worker contexts.

Please view https://codereview.chromium.org/1256323008/ for the continuation of this implementation in chrome.

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

[annekao, 2015-07-30 19:53:02]

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

[annekao, 2015-07-30 19:54:51]

Patchset #2 (id:20001) has been deleted

[annekao, 2015-07-30 21:13:04]

Patchset #1 (id:1) has been deleted

[annekao, 2015-07-30 21:13:09]

Patchset #1 (id:20002) has been deleted

[annekao, 2015-07-30 21:14:14]

Patchset #1 (id:50001) has been deleted

[Devlin, 2015-07-30 21:34:11]

LGTM, but we'll let a blink expert be the real authority.

[annekao, 2015-07-31 20:11:09]

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

[annekao, 2015-07-31 20:12:17]

Hi dcheng@, please take a look at the changes in these files.  The v8::context
for a service worker will be passed up from the worker thread into the
extensions dispatcher.  This patch continues into chrome here:
https://codereview.chromium.org/1256323008/.

[falken, 2015-08-03 01:55:13]

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

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

drive by

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

https://codereview.chromium.org/1264913002/diff/70001/Source/core/workers/Wor...
Source/core/workers/WorkerThread.cpp:259:
m_workerReportingProxy.didInitializeWorkerContext(script->context());
It seems to make sense to call didInitializeWorkerContext where
workerContextInitialized() is called in line 251.

As written this waits for evaluation to complete. So if the listener wants to
set some flag on the context as the the chromium patch seems to do, the initial
evaluation of the script wouldn't be under the flag (only events that fired
after would have it).

Also consider the while(true) {} case. If the script never terminates,
didInitializeWorkerContext() is never called in the current patch. In the case
of SW there's a browser-side timeout that sends a message to the renderer to
terminate execution and shutdown the thread. We should verify the sequence of
what happens in that case: I'm not sure whether script->context() is valid after
termination.

[dcheng, 2015-08-03 19:12:02]

Can you give me some background about why this is needed? I don't know enough to
understand how this fits into
https://docs.google.com/document/d/1iOpM0hl-dTUFRqg_iUVzvizcRXxf8zeUxWPkbNeuj....

[Devlin, 2015-08-03 20:03:51]

On 2015/08/03 19:12:02, dcheng wrote:
> Can you give me some background about why this is needed? I don't know enough
to
> understand how this fits into
>
https://docs.google.com/document/d/1iOpM0hl-dTUFRqg_iUVzvizcRXxf8zeUxWPkbNeuj....

In order to expose any kind of custom binding (i.e., javascript callback into
C++ code) for the service worker context, we need to be alerted of when the
context is created, so that we can inject them.  For most contexts, this is done
through the blink notification (didCreateScriptContext), but that doesn't cover
worker contexts.

[dcheng, 2015-08-04 00:27:36]

https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
public/web/WebServiceWorkerContextClient.h:97: virtual void
didInitializeWorkerContext(v8::Local<v8::Context> context, const blink::WebURL&
url) { }
Is it necessary to plumb |context| through like this? It seems like you could
get to this via |m_workerGlobalScope| (since that is set just a few lines before
this).

[dcheng, 2015-08-04 00:27:47]

Also, please update the CL description to explain why this patch is needed.

[annekao, 2015-08-04 18:43:39]

On 2015/08/03 01:55:14, falken wrote:
> drive by
> 
>
https://codereview.chromium.org/1264913002/diff/70001/Source/core/workers/Wor...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/1264913002/diff/70001/Source/core/workers/Wor...
> Source/core/workers/WorkerThread.cpp:259:
> m_workerReportingProxy.didInitializeWorkerContext(script->context());
> It seems to make sense to call didInitializeWorkerContext where
> workerContextInitialized() is called in line 251.
> 
> As written this waits for evaluation to complete. So if the listener wants to
> set some flag on the context as the the chromium patch seems to do, the
initial
> evaluation of the script wouldn't be under the flag (only events that fired
> after would have it).
> 
> Also consider the while(true) {} case. If the script never terminates,
> didInitializeWorkerContext() is never called in the current patch. In the case
> of SW there's a browser-side timeout that sends a message to the renderer to
> terminate execution and shutdown the thread. We should verify the sequence of
> what happens in that case: I'm not sure whether script->context() is valid
after
> termination.

Done.  Also added a script->context() is not empty check.

On 2015/08/04 00:27:36, dcheng wrote:
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> File public/web/WebServiceWorkerContextClient.h (right):
> 
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> public/web/WebServiceWorkerContextClient.h:97: virtual void
> didInitializeWorkerContext(v8::Local<v8::Context> context, const
blink::WebURL&
> url) { }
> Is it necessary to plumb |context| through like this? It seems like you could
> get to this via |m_workerGlobalScope| (since that is set just a few lines
before
> this).

I'm not entirely sure what you mean.  Are you suggesting that instead of
starting to pass the context up from the WorkerThread, start passing it from the
ServiceWorkerGlobalProxy?

On 2015/08/04 00:27:47, dcheng wrote:
> Also, please update the CL description to explain why this patch is needed.

Done.

[dcheng, 2015-08-04 18:48:33]

On 2015/08/04 at 18:43:39, annekao wrote:
> On 2015/08/03 01:55:14, falken wrote:
> > drive by
> > 
> >
https://codereview.chromium.org/1264913002/diff/70001/Source/core/workers/Wor...
> > File Source/core/workers/WorkerThread.cpp (right):
> > 
> >
https://codereview.chromium.org/1264913002/diff/70001/Source/core/workers/Wor...
> > Source/core/workers/WorkerThread.cpp:259:
> > m_workerReportingProxy.didInitializeWorkerContext(script->context());
> > It seems to make sense to call didInitializeWorkerContext where
> > workerContextInitialized() is called in line 251.
> > 
> > As written this waits for evaluation to complete. So if the listener wants
to
> > set some flag on the context as the the chromium patch seems to do, the
initial
> > evaluation of the script wouldn't be under the flag (only events that fired
> > after would have it).
> > 
> > Also consider the while(true) {} case. If the script never terminates,
> > didInitializeWorkerContext() is never called in the current patch. In the
case
> > of SW there's a browser-side timeout that sends a message to the renderer to
> > terminate execution and shutdown the thread. We should verify the sequence
of
> > what happens in that case: I'm not sure whether script->context() is valid
after
> > termination.
> 
> Done.  Also added a script->context() is not empty check.
> 
> On 2015/08/04 00:27:36, dcheng wrote:
> >
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > File public/web/WebServiceWorkerContextClient.h (right):
> > 
> >
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > public/web/WebServiceWorkerContextClient.h:97: virtual void
> > didInitializeWorkerContext(v8::Local<v8::Context> context, const
blink::WebURL&
> > url) { }
> > Is it necessary to plumb |context| through like this? It seems like you
could
> > get to this via |m_workerGlobalScope| (since that is set just a few lines
before
> > this).
> 
> I'm not entirely sure what you mean.  Are you suggesting that instead of
starting to pass the context up from the WorkerThread, start passing it from the
ServiceWorkerGlobalProxy?
> 

Isn't the v8::Context derivable from m_workerGlobalScope? So passing it as a
parameter is redundant, isn't it?

> On 2015/08/04 00:27:47, dcheng wrote:
> > Also, please update the CL description to explain why this patch is needed.
> 
> Done.

[annekao, 2015-08-04 19:53:33]

On 2015/08/04 18:48:33, dcheng wrote:
> > On 2015/08/04 00:27:36, dcheng wrote:
> > >
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > File public/web/WebServiceWorkerContextClient.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > public/web/WebServiceWorkerContextClient.h:97: virtual void
> > > didInitializeWorkerContext(v8::Local<v8::Context> context, const
> blink::WebURL&
> > > url) { }
> > > Is it necessary to plumb |context| through like this? It seems like you
> could
> > > get to this via |m_workerGlobalScope| (since that is set just a few lines
> before
> > > this).
> > 
> > I'm not entirely sure what you mean.  Are you suggesting that instead of
> starting to pass the context up from the WorkerThread, start passing it from
the
> ServiceWorkerGlobalProxy?
> > 
> 
> Isn't the v8::Context derivable from m_workerGlobalScope? So passing it as a
> parameter is redundant, isn't it?
> 

Well, it will have to be passed as a v8::Context when moving into the chrome
layer.  It is possible that in blink the context doesn't need to be passed
around, but when m_client gets called in the GlobalScopeProxy, it'll have to
pass the v8::Context to the client to avoid layering violations.

[dcheng, 2015-08-04 19:57:47]

On 2015/08/04 at 19:53:33, annekao wrote:
> On 2015/08/04 18:48:33, dcheng wrote:
> > > On 2015/08/04 00:27:36, dcheng wrote:
> > > >
> >
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > > File public/web/WebServiceWorkerContextClient.h (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > > public/web/WebServiceWorkerContextClient.h:97: virtual void
> > > > didInitializeWorkerContext(v8::Local<v8::Context> context, const
> > blink::WebURL&
> > > > url) { }
> > > > Is it necessary to plumb |context| through like this? It seems like you
> > could
> > > > get to this via |m_workerGlobalScope| (since that is set just a few
lines
> > before
> > > > this).
> > > 
> > > I'm not entirely sure what you mean.  Are you suggesting that instead of
> > starting to pass the context up from the WorkerThread, start passing it from
the
> > ServiceWorkerGlobalProxy?
> > > 
> > 
> > Isn't the v8::Context derivable from m_workerGlobalScope? So passing it as a
> > parameter is redundant, isn't it?
> > 
> 
> Well, it will have to be passed as a v8::Context when moving into the chrome
layer.  It is possible that in blink the context doesn't need to be passed
around, but when m_client gets called in the GlobalScopeProxy, it'll have to
pass the v8::Context to the client to avoid layering violations.

I'm only asking for the removal of the parameter from core->web.

[annekao, 2015-08-04 20:05:13]

On 2015/08/04 19:57:47, dcheng wrote:
> On 2015/08/04 at 19:53:33, annekao wrote:
> > On 2015/08/04 18:48:33, dcheng wrote:
> > > > On 2015/08/04 00:27:36, dcheng wrote:
> > > > >
> > >
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > > > File public/web/WebServiceWorkerContextClient.h (right):
> > > > > 
> > > > >
> > >
>
https://codereview.chromium.org/1264913002/diff/70001/public/web/WebServiceWo...
> > > > > public/web/WebServiceWorkerContextClient.h:97: virtual void
> > > > > didInitializeWorkerContext(v8::Local<v8::Context> context, const
> > > blink::WebURL&
> > > > > url) { }
> > > > > Is it necessary to plumb |context| through like this? It seems like
you
> > > could
> > > > > get to this via |m_workerGlobalScope| (since that is set just a few
> lines
> > > before
> > > > > this).
> > > > 
> > > > I'm not entirely sure what you mean.  Are you suggesting that instead of
> > > starting to pass the context up from the WorkerThread, start passing it
from
> the
> > > ServiceWorkerGlobalProxy?
> > > > 
> > > 
> > > Isn't the v8::Context derivable from m_workerGlobalScope? So passing it as
a
> > > parameter is redundant, isn't it?
> > > 
> > 
> > Well, it will have to be passed as a v8::Context when moving into the chrome
> layer.  It is possible that in blink the context doesn't need to be passed
> around, but when m_client gets called in the GlobalScopeProxy, it'll have to
> pass the v8::Context to the client to avoid layering violations.
> 
> I'm only asking for the removal of the parameter from core->web.

Done.

[dcheng, 2015-08-05 00:04:53]

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
Source/core/workers/WorkerReportingProxy.h:38: #include <v8.h>
Remove this #include.

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:253: if
(script->scriptState()->contextIsValid()) {
How come we didn't need to check if the context was valid before?

https://codereview.chromium.org/1264913002/diff/110001/Source/web/ServiceWork...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/web/ServiceWork...
Source/web/ServiceWorkerGlobalScopeProxy.h:40: #include <v8.h>
Remove this #include.

[annekao, 2015-08-05 16:43:02]

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
File Source/core/workers/WorkerReportingProxy.h (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
Source/core/workers/WorkerReportingProxy.h:38: #include <v8.h>
On 2015/08/05 00:04:53, dcheng wrote:
> Remove this #include.

Done.

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:253: if
(script->scriptState()->contextIsValid()) {
On 2015/08/05 00:04:53, dcheng wrote:
> How come we didn't need to check if the context was valid before?

I initially put this check in to address what falken said about how it may not
be valid.  But, after rereading his comment I realized I misunderstood it.  This
can be removed if there's no need for it.

https://codereview.chromium.org/1264913002/diff/110001/Source/web/ServiceWork...
File Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/web/ServiceWork...
Source/web/ServiceWorkerGlobalScopeProxy.h:40: #include <v8.h>
On 2015/08/05 00:04:53, dcheng wrote:
> Remove this #include.

Done.

[falken, 2015-08-06 03:37:20]

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:253: if
(script->scriptState()->contextIsValid()) {
On 2015/08/05 16:43:02, annekao wrote:
> On 2015/08/05 00:04:53, dcheng wrote:
> > How come we didn't need to check if the context was valid before?
> 
> I initially put this check in to address what falken said about how it may not
> be valid.  But, after rereading his comment I realized I misunderstood it. 
This
> can be removed if there's no need for it.

Hm, I was thinking of a different case, but it looks like it's still needed. In
the case of isExecutionForbidden(),  initializeContextIfNeeded() is not called,
so I think the context would be invalid. Probably it doesn't make sense to call
didInitializeWorkerContext() in that case.

[annekao, 2015-08-07 16:21:41]

On 2015/08/06 03:37:20, falken wrote:
>
https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
> Source/core/workers/WorkerThread.cpp:253: if
> (script->scriptState()->contextIsValid()) {
> On 2015/08/05 16:43:02, annekao wrote:
> > On 2015/08/05 00:04:53, dcheng wrote:
> > > How come we didn't need to check if the context was valid before?
> > 
> > I initially put this check in to address what falken said about how it may
not
> > be valid.  But, after rereading his comment I realized I misunderstood it. 
> This
> > can be removed if there's no need for it.
> 
> Hm, I was thinking of a different case, but it looks like it's still needed.
In
> the case of isExecutionForbidden(),  initializeContextIfNeeded() is not
called,
> so I think the context would be invalid. Probably it doesn't make sense to
call
> didInitializeWorkerContext() in that case.

dcheng@ thoughts?

[dcheng, 2015-08-07 17:34:03]

On 2015/08/07 at 16:21:41, annekao wrote:
> On 2015/08/06 03:37:20, falken wrote:
> >
https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
> > File Source/core/workers/WorkerThread.cpp (right):
> > 
> >
https://codereview.chromium.org/1264913002/diff/110001/Source/core/workers/Wo...
> > Source/core/workers/WorkerThread.cpp:253: if
> > (script->scriptState()->contextIsValid()) {
> > On 2015/08/05 16:43:02, annekao wrote:
> > > On 2015/08/05 00:04:53, dcheng wrote:
> > > > How come we didn't need to check if the context was valid before?
> > > 
> > > I initially put this check in to address what falken said about how it may
not
> > > be valid.  But, after rereading his comment I realized I misunderstood it.

> > This
> > > can be removed if there's no need for it.
> > 
> > Hm, I was thinking of a different case, but it looks like it's still needed.
In
> > the case of isExecutionForbidden(),  initializeContextIfNeeded() is not
called,
> > so I think the context would be invalid. Probably it doesn't make sense to
call
> > didInitializeWorkerContext() in that case.
> 
> dcheng@ thoughts?

I'm not an expert in service workers. That being said, if script execution is
forbidden, does it make sense to do any of the remaining work in the function?
What can a SW do if scripting is forbidden?

I defer to falken@ for final signoff on the WorkerThread.cpp changes.

[falken, 2015-08-10 04:10:56]

https://codereview.chromium.org/1264913002/diff/130001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1264913002/diff/130001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:255: }
This particular change lgtm.

The existing code is pretty weird as dcheng@ says: it's not clear why we
continue with everything even when isExecutionForbidden() is false and call
workerInspectorController()->workerContextInitialized() when the context may not
have been initialized.

For some clues, I see the initializeContextIfNeeded call was added in
https://codereview.chromium.org/270873005/ to fix a bug with DevTools.

blink-worker team should do some clean up/commenting here but it's not required
for this review.

[annekao, 2015-08-10 17:55:13]

Patchset #5 (id:150001) has been deleted

[annekao, 2015-08-10 20:27:36]

Patchset #5 (id:170001) has been deleted

[annekao, 2015-08-10 23:43:20]

The CQ bit was checked by annekao@google.com

[annekao, 2015-08-10 23:43:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1264913002/#ps190001
(title: " ")

[commit-bot: I haz the power, 2015-08-10 23:43:29]

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

[commit-bot: I haz the power, 2015-08-10 23:49:57]

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

[commit-bot: I haz the power, 2015-08-10 23:49:58]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[dcheng, 2015-08-10 23:51:43]

lgtm, but let's try to clean up this logic in a followup patch

[annekao, 2015-08-10 23:51:59]

The CQ bit was checked by annekao@google.com

[commit-bot: I haz the power, 2015-08-10 23:52:09]

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

[commit-bot: I haz the power, 2015-08-10 23:55:20]

Committed patchset #5 (id:190001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200277

[falken, 2015-08-11 01:49:28]

On 2015/08/10 23:51:43, dcheng wrote:
> lgtm, but let's try to clean up this logic in a followup patch

FYI filed https://code.google.com/p/chromium/issues/detail?id=519111

[sadrul, 2015-08-11 22:07:30]

sadrul@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-08-11 22:07:31]

https://codereview.chromium.org/1264913002/diff/190001/Source/core/workers/Wo...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/1264913002/diff/190001/Source/core/workers/Wo...
Source/core/workers/WorkerThread.cpp:253: if
(m_workerGlobalScope->script()->scriptState()->contextIsValid()) {
Do you think this should first check if
m_workerGlobalScope->script()->isContextIntialized() (or that
script()->scriptState() is non-nul)?

I ask since some related test seemed to start failing after this change (e.g.
the first failure in
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/?numbui...
coincides with this change:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...)

[falken, 2015-08-12 02:17:28]

On 2015/08/11 22:07:31, sadrul wrote:
>
https://codereview.chromium.org/1264913002/diff/190001/Source/core/workers/Wo...
> File Source/core/workers/WorkerThread.cpp (right):
> 
>
https://codereview.chromium.org/1264913002/diff/190001/Source/core/workers/Wo...
> Source/core/workers/WorkerThread.cpp:253: if
> (m_workerGlobalScope->script()->scriptState()->contextIsValid()) {
> Do you think this should first check if
> m_workerGlobalScope->script()->isContextIntialized() (or that
> script()->scriptState() is non-nul)?
> 
> I ask since some related test seemed to start failing after this change (e.g.
> the first failure in
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/?numbui...
> coincides with this change:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/...)

Yes this looks wrong. I manually ran with isExecutionForbidden set to false and
we crash because scriptState is null.

Sorry I didn't catch this, I assumed contextIsValid() is the way to check that
initialization happened.

Anne: I think we better to revert this and the chromium change so we can green
up the tree. Is it OK?

[falken, 2015-08-12 03:17:18]

A revert of this CL (patchset #5 id:190001) has been created in
https://codereview.chromium.org/1287903002/ by falken@chromium.org.

The reason for reverting is: Sorry to revert this change. It's suspected of
breaking webkit_unit_tests test
CompositorWorkerManagerTest.CreatingSecondDuringTerminationOfFirst:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/....

[falken, 2015-08-12 06:29:13]

On 2015/08/12 03:17:18, falken wrote:
> A revert of this CL (patchset #5 id:190001) has been created in
> https://codereview.chromium.org/1287903002/ by mailto:falken@chromium.org.
> 
> The reason for reverting is: Sorry to revert this change. It's suspected of
> breaking webkit_unit_tests test
> CompositorWorkerManagerTest.CreatingSecondDuringTerminationOfFirst:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.8/builds/....

Update: The test passes against after the revert. I think the solution is to
replace "if (m_workerGlobalScope->script()->scriptState()->contextIsValid()) {"
with "m_workerGlobalScope->script()->isContextInitialized()".

It'd be great if we can have a test for this, but seems difficult because of the
threading. I don't know how to force isExecutionForbidden() to return true in a
test. At least, testing the codepath manually by simulating isExecutionForbidden
returning false is a good idea.

[annekao, 2015-08-12 17:47:02]

Patchset #6 (id:210001) has been deleted

[annekao, 2015-08-12 18:15:27]

Updated patch can be found here: https://codereview.chromium.org/1287103002/