ServiceWorker: Add null check of ExecutionContext in skipWaiting.

ServiceWorker: Add null check of ExecutionContext in skipWaiting.

skipWaiting has the same issue like https://codereview.chromium.org/535193002/.

BUG=443046
TEST=run_webkit_tests http/tests/serviceworker/skip-waiting.html

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

[xiang, 2015-01-06 09:03:34]

xiang.long@intel.com changed reviewers:
+ falken@chromium.org, jsbell@chromium.org

[xiang, 2015-01-06 09:03:35]

PTAL, thanks.
I also want to split the skip-waiting.html tests to separate files, how do you
think?

[falken, 2015-01-08 04:31:52]

cc yhirano@, haraken@, kinuko@, this seems like more TerminateExecution trouble

I'm a fan of splitting up tests into multiple files. These monolithic tests are
really hard to debug when they flake.

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:131: return
ScriptPromise();
I have some questions.

1. What happens if TerminateExecution is called after this, is still safe to use
this executionContext variable? +haraken@ mentioned in
https://codereview.chromium.org/535193002/ crashes could still occur in that
scenario.
2. Why is the TerminateExecution getting called in the first place? We schedule
stopping the worker 30 seconds after it's started, it shouldn't happen during
this layout test.

[xiang, 2015-01-08 08:16:13]

I will split the test. Thanks.

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:131: return
ScriptPromise();
On 2015/01/08 04:31:52, falken wrote:
> I have some questions.
> 
> 1. What happens if TerminateExecution is called after this, is still safe to
use
> this executionContext variable? +haraken@ mentioned in
> https://codereview.chromium.org/535193002/ crashes could still occur in that
> scenario.

I'm not sure about it in this case, from my code reading, it should be OK.
Anyway I added a "FIXME" comment.

> 2. Why is the TerminateExecution getting called in the first place? We
schedule
> stopping the worker 30 seconds after it's started, it shouldn't happen during
> this layout test.

I think the unregister() will trigger worker stop. 2 of 3 tests in this file
will unregister() without waiting the worker "complete" message.

[falken, 2015-01-08 08:28:05]

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

[falken, 2015-01-08 08:28:05]

haraken@: can you also have a look?

I think this LGTM as a short-term solution. Probably long-term we need to fix
TerminateExecution and SW interaction in a deeper way.

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right):

https://codereview.chromium.org/823583006/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:131: return
ScriptPromise();
On 2015/01/08 08:16:13, xiang wrote:
> On 2015/01/08 04:31:52, falken wrote:
> > I have some questions.
> > 
> > 1. What happens if TerminateExecution is called after this, is still safe to
> use
> > this executionContext variable? +haraken@ mentioned in
> > https://codereview.chromium.org/535193002/ crashes could still occur in that
> > scenario.
> 
> I'm not sure about it in this case, from my code reading, it should be OK.
> Anyway I added a "FIXME" comment.
> 
> > 2. Why is the TerminateExecution getting called in the first place? We
> schedule
> > stopping the worker 30 seconds after it's started, it shouldn't happen
during
> > this layout test.
> 
> I think the unregister() will trigger worker stop. 2 of 3 tests in this file
> will unregister() without waiting the worker "complete" message.

I see! That looks right.

[haraken, 2015-01-08 08:31:26]

On 2015/01/08 08:28:05, falken wrote:
> haraken@: can you also have a look?
> 
> I think this LGTM as a short-term solution. Probably long-term we need to fix
> TerminateExecution and SW interaction in a deeper way.

LGTM

A long-term solution is either of the proposals in
https://docs.google.com/a/chromium.org/document/d/1komNUseODFLptg4l45zerYRpWB....

[haraken, 2015-01-08 08:32:25]

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

[haraken, 2015-01-08 08:32:26]

+jochen

Is there any progress on the exception handling work?

[jochen (gone - plz use gerrit), 2015-01-08 13:05:27]

nobody signed up for it yet, no

[xiang, 2015-01-09 02:26:11]

The CQ bit was checked by xiang.long@intel.com

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

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

[commit-bot: I haz the power, 2015-01-09 04:26:31]

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