Add terminateServiceWorker method for layout tests - blink side

Add terminateServiceWorker method for layout tests.

Depends on the Chromium side changes in https://codereview.chromium.org/688413002/

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

[Marijn Kruisselbrink, 2014-11-04 22:50:02]

Patchset #1 (id:1) has been deleted

[Marijn Kruisselbrink, 2014-11-04 22:50:08]

Patchset #1 (id:20001) has been deleted

[Marijn Kruisselbrink, 2014-11-04 23:11:54]

mek@chromium.org changed reviewers:
+ dominicc@chromium.org

[Marijn Kruisselbrink, 2014-11-04 23:11:55]

This is (the blink side of) my attempt at adding an
internals.terminateServiceWorker method to allow writing tests that verify
correct behavior in the presence of service workers being terminated.
As written like this, any messages send to the service worker while it is
terminating get dropped on the floor. So for now to get my test to pass I've
added a delay, alternatively I could of course make
internals.terminateServiceWorker return a promise that doesn't resolve until the
termination is complete, but if this is actually a bug in the service worker
implementation it is probably better to keep the test without any delay/waiting?
I'm not entirely sure if this is a bug though, since there seems to be lot of
code in ServiceWorkerVersion to explicitly abort all attempts at delivering an
event to the service worker while it is stopping.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html
(right):

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:40:
//return delay(500);
This test currently only passes when this delay is enabled.

[dominicc (has gone to gerrit), 2014-11-17 02:25:26]

This looks like a good start on this problem--thanks! Some test comments inline.
Sorry for such a slow response.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html
(right):

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:12:
var channel = new MessageChannel();
Could you indent this slightly differently, following the examples here:

http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style

Below too.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:38:
return internals.terminateServiceWorker(sw);
What if the Service Worker terminated itself, perhaps in response to specific
messages? Depending on whether shutdown is enacted immediately that might make
this test more reliable.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:40:
//return delay(500);
On 2014/11/04 23:11:54, Marijn Kruisselbrink wrote:
> This test currently only passes when this delay is enabled.

Let's not add a test that is known to be broken. Obviously timing-based stuff
like this has a higher propensity to flake and/or time out, what happened on Win
XP bots?

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:45:
return sendPing(sw);
Are two subsequent pings necessary?

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:50:
}, 'Terminate terminates the worker.', properties);
Is the test really about that? It's about postMessage getting to a
terminated/terminating worker, right?

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js
(right):

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js:3:
event.data.port.postMessage(++counter);
JavaScript != C++, use postfix increment.

[Marijn Kruisselbrink, 2014-11-17 23:18:45]

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html
(right):

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:12:
var channel = new MessageChannel();
On 2014/11/17 02:25:26, dominicc wrote:
> Could you indent this slightly differently, following the examples here:
> 
> http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style
> 
> Below too.

Done (at least I think so).

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:38:
return internals.terminateServiceWorker(sw);
On 2014/11/17 02:25:25, dominicc wrote:
> What if the Service Worker terminated itself, perhaps in response to specific
> messages? Depending on whether shutdown is enacted immediately that might make
> this test more reliable.
I suppose I could change the test to have the worker call
internals.terminateServiceWorker itself (does a service worker have access to a
ServiceWorker instance? or would this require changing the internals API to be
callable from a service worker?). I'm not sure how that would help with making
the test more reliable? And instead of this test working around the bug in
message/event delivery for terminating workers, message/event delivery should
probably just be fixed before landing this CL/enabling this test.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:40:
//return delay(500);
On 2014/11/17 02:25:25, dominicc wrote:
> On 2014/11/04 23:11:54, Marijn Kruisselbrink wrote:
> > This test currently only passes when this delay is enabled.
> 
> Let's not add a test that is known to be broken. Obviously timing-based stuff
> like this has a higher propensity to flake and/or time out, what happened on
Win
> XP bots?
Well, the test is not broken, it just doesn't currently pass since
http://crbug.com/433439 isn't fixed yet. And it doesn't rely on anything timing
based. The only potential flakyness is that this test isn't guaranteed to fail
even with the current implementation, but if that bug is fixed the test is
guarenteed to always pass, with no chance for timing problems.
The only potential flakyness is that the service worker manages to terminate
completely before the next sendPing is called, in which case the test could
incorrectly pass, but that chance is pretty low, and flaky test success is much
much less of a problem than flaky test failure anyway.
And I can't try this on any bots yet, since I'll first have to land the chromium
side changes.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:45:
return sendPing(sw);
On 2014/11/17 02:25:25, dominicc wrote:
> Are two subsequent pings necessary?
Not really, just a simple sanity check to make sure my worker actually does what
I expect it to do, but it doesn't really add anything for this test in
particular.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:50:
}, 'Terminate terminates the worker.', properties);
On 2014/11/17 02:25:25, dominicc wrote:
> Is the test really about that? It's about postMessage getting to a
> terminated/terminating worker, right?
Well, it's more about both. I started writing this test to make sure that the
internals.terminateServiceWorker method I wrote was working correctly, so that
is really what it is testing. That it happens to expose a completely different
bug (about postMessage, and any other kind of event delivery to a terminating
worker) doesn't really change what the test is testing.

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js
(right):

https://codereview.chromium.org/695643002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js:3:
event.data.port.postMessage(++counter);
On 2014/11/17 02:25:26, dominicc wrote:
> JavaScript != C++, use postfix increment.

Done.

[dominicc (has gone to gerrit), 2014-11-20 05:47:57]

I'm basically confused about what this test is for at this point: The title and
name of the file say one thing; the implementation seems to agree with that
(although it has a redundant message); the description of the test says another
thing; and the test isn't expected to pass except flakily.

Can you just take things out of this until you're left with what you want to
test and then make the filename, etc. match that? And we can test other things
later.

[dominicc (has gone to gerrit), 2014-11-20 05:48:09]

https://codereview.chromium.org/695643002/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html
(right):

https://codereview.chromium.org/695643002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:8:

Delete this and the trailing blank line.

[Marijn Kruisselbrink, 2014-11-20 19:28:22]

On 2014/11/20 05:47:57, dominicc wrote:
> I'm basically confused about what this test is for at this point: The title
and
> name of the file say one thing; the implementation seems to agree with that
> (although it has a redundant message); the description of the test says
another
> thing; and the test isn't expected to pass except flakily.
I'm sorry about all the confusion. I started trying to write a test to make sure
my
terminate method does what I expect it to do. The simplest way to do that seemed
to
use postMessage to inspect some of the state of the service worker. So it uses
postMessage to test terminate, but at the same time it uses terminate to test
postMessage. I'm not sure how to separate those two, but yes, it probably makes
sense to rename either the file or the test to at least say the same thing (and
since testing test infrastructure (the terminate method) is less useful than
testing
postMessage, it probably makes sense to rename it all to make this be a
postMessage
test.

The other part of the confusion is probably the flakiness/delays I had in my
initial
test. None of this test is flaky, as long as there are no bugs in chrome. If
this test
ever fails I would definitely consider that to be a bug in chrome. Now
unfortunately
currently this test does in fact fail, because of http://crbug.com/433439
The delays etc in my original patch set in this CL were a work around, to be
able to
actually run this test even without 433439 being fixed. I removed all those
delays,
since of course a useful test should not rely on any kind of timing. So yes,
until
433439 is fixed, this test can't be landed.
https://codereview.chromium.org/739383002/
is an attempt to fix 433439, but as I commented on that bug, I really don't know
that
part of the code well enough to be able to oversee all the possible side effects
of
such a change.
Given that it might make sense to move the test in this CL out into a separate
CL, to
not block this CL on having a fix for 433439 (although the terminate method
isn't
really useful without 433439 fixed, so I have no problem not landing this CL
until
after 433439 is fixed).

https://codereview.chromium.org/695643002/diff/60001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html
(right):

https://codereview.chromium.org/695643002/diff/60001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html:8:

On 2014/11/20 05:48:08, dominicc wrote:
> Delete this and the trailing blank line.

Done.

[dominicc (has gone to gerrit), 2014-11-27 04:59:31]

LGTM

[Marijn Kruisselbrink, 2014-12-03 12:12:10]

mek@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Marijn Kruisselbrink, 2014-12-03 12:12:10]

+pfeldman for public/platform/WebServiceWorker.h OWNERS

[pfeldman, 2014-12-03 12:48:13]

lgtm

[Marijn Kruisselbrink, 2014-12-03 12:48:44]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-03 12:49:06]

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

[commit-bot: I haz the power, 2014-12-03 12:51:24]

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