Make ServiceWorker an ActiveDOMObject

Make ServiceWorker an ActiveDOMObject

Before this patch, the ServiceWorker object returned by
navigator.serviceWorker.register() could be garbage collected
prematurely causing state change event handlers to never be invoked.
This patch makes ServiceWorker an ActiveDOMObject and keeps it alive
until either stop() is called on it (indicating detach of the parent
document) or it reaches the terminal "deactivated" state (soon to be
renamed "redundant").

For future work, it may be possible to be more clever and allow the SW
to die when it has no event handlers.

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

[falken, 2014-06-12 21:06:11]

PTAL

[dominicc (has gone to gerrit), 2014-06-12 22:23:39]

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:2: <script
src="../resources/testharness.js"></script>
This should use the Blink test harness. GC is an implementation detail; it is
not suitable subject matter for a W3C test.

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:9: var t =
async_test('A registered Service Worker with an event handler is not garbage
collected');
FYI I have started doing

async_test(function(t) {
    ...
}, '...');

It is cleaner when there are multiple tests at the top-level because t is local
to the test and in practice we are always starting with test_step and closure
anyway.

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:22: // FIXME: The
expectation should be reversed, but our implementation currently fails it.
I think you want to rephrase this as a "leak test" and that it "should not be
leaked". It will help to have the test in a separate, targeted file. Might be
useful to get kouhei's input on this test.

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:28: function
onRegister(sw) {
FYI I am moving to using promises instead of callbacks in tests; WDYT?

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:79: m_contextStopped = true;
Would it make sense to tie m_contextStopped and m_isPromisePending into one
"state"? WDYT?

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.h (right):

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.h:77: // ActiveDOMObject overrides.
Let's make these private?

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.h:92: // Whether
ActiveDOMObject::stop has been called.
We can probably omit this comment.

[falken, 2014-06-13 19:22:59]

Thanks, PTAL

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:2: <script
src="../resources/testharness.js"></script>
On 2014/06/12 22:23:38, dominicc wrote:
> This should use the Blink test harness. GC is an implementation detail; it is
> not suitable subject matter for a W3C test.

Done.

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:22: // FIXME: The
expectation should be reversed, but our implementation currently fails it.
On 2014/06/12 22:23:38, dominicc wrote:
> I think you want to rephrase this as a "leak test" and that it "should not be
> leaked". It will help to have the test in a separate, targeted file. Might be
> useful to get kouhei's input on this test.

OK, the idea being that the test would succeed on the leak bot? Let's tackle
that when we actually implement it correctly. For now, I've just expanded the
FIXME.

https://codereview.chromium.org/330173003/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:28: function
onRegister(sw) {
On 2014/06/12 22:23:38, dominicc wrote:
> FYI I am moving to using promises instead of callbacks in tests; WDYT?

Done. Yes, I think that's a good idea. Our test-helpers.js should be updated
too.

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:79: m_contextStopped = true;
On 2014/06/12 22:23:38, dominicc wrote:
> Would it make sense to tie m_contextStopped and m_isPromisePending into one
> "state"? WDYT?

Hmm, right it would be good to make the possible combinations of the states
explicit, or disallow combinations. I've made an m_proxyState.

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.h (right):

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.h:77: // ActiveDOMObject overrides.
On 2014/06/12 22:23:38, dominicc wrote:
> Let's make these private?

Done.

https://codereview.chromium.org/330173003/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.h:92: // Whether
ActiveDOMObject::stop has been called.
On 2014/06/12 22:23:38, dominicc wrote:
> We can probably omit this comment.

Done.

[falken, 2014-06-13 19:40:36]

I added a call up to AbstractWorker::hasPendingActivity. I'm not sure whether we
want it to come after or before the check that stop() was called. Other
ActiveDOMObjects go both ways: e.g., MediaKeySession returns true even if stop()
was called, but the IDB* classes return false.

[dominicc (has gone to gerrit), 2014-06-13 19:55:13]

I think the proxy state thing is very neat; some comments and questions about
the test.

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:7: description('Test
that a registered Service Worker with an event handler is not garbage
collected');
Maybe add '...prematurely'?

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:23: function
collectGarbage(shouldBeCollected, description) {
I'm not sure this function is pulling its weight. A specific assertion can be
done in three lines:

observation = internals.observeGC(x);
...
gc();
test observation.wasCollected

That is only two more lines than calling collectGarbage; it might be more
readable that way. WDYT?

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:25: if
(swObservation) {
Would it be better to fail if this is not set up? If this was accidentally
missing, the test would silently pass by not testing any assertions.

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:44: swObservation =
internals.observeGC(sw);
Is this a global just so shouldBe works?

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:49:
collectGarbage(false, 'Service Worker should not be gc\'d in ' +
event.target.state + ' state');
If it is garbage collected, won't event.target be null? Or do we create a new
one?

https://codereview.chromium.org/330173003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/330173003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:164: void
ServiceWorker::setProxyState(ProxyState state)
This is very nice and neat.

[falken, 2014-06-13 22:34:19]

Thanks, I changed the test a bit. It turns out there is a race between the state
change events and the testing function in onRegister queued via setTimeout. I
think that's expected and OK, but it means we should only output something when
an assertion fails, or else there's racy output for the succeeding asserts.

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:7: description('Test
that a registered Service Worker with an event handler is not garbage
collected');
On 2014/06/13 19:55:13, dominicc wrote:
> Maybe add '...prematurely'?

Done.

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:23: function
collectGarbage(shouldBeCollected, description) {
On 2014/06/13 19:55:13, dominicc wrote:
> I'm not sure this function is pulling its weight. A specific assertion can be
> done in three lines:
> 
> observation = internals.observeGC(x);
> ...
> gc();
> test observation.wasCollected
> 
> That is only two more lines than calling collectGarbage; it might be more
> readable that way. WDYT?

I tried but there's a couple problems with that:
1) onUnregister doesn't have sw, so it can't call internals.observeGC(sw)
2) If we replace the collectGarbage in onRegister with a function() { ... }, I
think sw would not be garbage collected inside that function, since it has
access to sw (even if it doesn't actually use it), right?

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:25: if
(swObservation) {
On 2014/06/13 19:55:13, dominicc wrote:
> Would it be better to fail if this is not set up? If this was accidentally
> missing, the test would silently pass by not testing any assertions.

I agree this is making the test more complex and changed it to fail early if
there's no observeGC.

My original intention was that the test could still run in environments without
internals.observeGC. It would timeout if the sw got gc'd too early (and gc()
attempts to trigger gc if internals is not available).

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:44: swObservation =
internals.observeGC(sw);
On 2014/06/13 19:55:13, dominicc wrote:
> Is this a global just so shouldBe works?

That, and onRegister needs it.

https://codereview.chromium.org/330173003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:49:
collectGarbage(false, 'Service Worker should not be gc\'d in ' +
event.target.state + ' state');
On 2014/06/13 19:55:13, dominicc wrote:
> If it is garbage collected, won't event.target be null? Or do we create a new
> one?

From what I've seen on the timing out test, if we got to this line, it meant the
sw was not gc'd. If sw is gc'd, then the event handler is never entered. This
line tries to trigger gc to test that the sw stays alive.

https://codereview.chromium.org/330173003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/330173003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorker.cpp:164: void
ServiceWorker::setProxyState(ProxyState state)
On 2014/06/13 19:55:13, dominicc wrote:
> This is very nice and neat.

Thanks!

[dominicc (has gone to gerrit), 2014-06-14 20:02:47]

More comments on the test.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:5: (function() {
I think that this test would be OK without this closure. Up to you. Saves you
one level of indent.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:24: function
fail(message) {
Doesn't the thing that provides debug also have things like fail and the "test
passed" printer?

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:36: // FIXME: The
expectation should be reversed, but our implementation currently fails it.
I think you can make the FIXME clearer, something like:

Assert that the ServiceWorker *is* collected when the redundant state is
implemented.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:47: if
(!swObservation) {
It might be simpler to test for window.internals at the top of the file and fail
the test fast if it is missing.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:54:
setTimeout(collectGarbage.bind(undefined, false, 'Service Worker should not be
gc\'d after registration'), 0);
setTimeout and the statechange events inevitably race. I think that is
unavoidable and fine. If you want to slow the Service Worker down in a state
transition you could do that with waitUntil and timeouts or postMessage (when we
implement "redundant" it is probably worth testing that a Service Worker sharing
a Message Channel does not leak.)

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:58:
collectGarbage(false, 'Service Worker should not be gc\'d in ' +
event.target.state + ' state');
The calls to collectGarbage are confusing in that collectGarbage(false, ...)
actually collects garbage; and collectGarbage always does other stuff.

I think it will be more readable to simply do two very literal lines:

gc();
shouldBeFalse('serviceWorker.wasCollected');

If the test breaks it may time out; that is not ideal but it is OK.

[falken, 2014-06-16 00:58:57]

Revised the test, how is this?

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:5: (function() {
On 2014/06/14 20:02:47, dominicc wrote:
> I think that this test would be OK without this closure. Up to you. Saves you
> one level of indent.

Done.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:24: function
fail(message) {
On 2014/06/14 20:02:47, dominicc wrote:
> Doesn't the thing that provides debug also have things like fail and the "test
> passed" printer?

Done.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:36: // FIXME: The
expectation should be reversed, but our implementation currently fails it.
On 2014/06/14 20:02:47, dominicc wrote:
> I think you can make the FIXME clearer, something like:
> 
> Assert that the ServiceWorker *is* collected when the redundant state is
> implemented.

Done.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:47: if
(!swObservation) {
On 2014/06/14 20:02:47, dominicc wrote:
> It might be simpler to test for window.internals at the top of the file and
fail
> the test fast if it is missing.

Done.

https://codereview.chromium.org/330173003/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:58:
collectGarbage(false, 'Service Worker should not be gc\'d in ' +
event.target.state + ' state');
On 2014/06/14 20:02:47, dominicc wrote:
> The calls to collectGarbage are confusing in that collectGarbage(false, ...)
> actually collects garbage; and collectGarbage always does other stuff.
> 
> I think it will be more readable to simply do two very literal lines:
> 
> gc();
> shouldBeFalse('serviceWorker.wasCollected');
> 
> If the test breaks it may time out; that is not ideal but it is OK.

OK, done. I thought it'd be nice to avoid time out and also print where the
assert failed, but it may not be worth the complexity. Now with the generic
shouldBeFalse, the race between setTimeout and statechange events doesn't matter
so the code is simpler.

[dominicc (has gone to gerrit), 2014-06-16 05:00:23]

LGTM

https://codereview.chromium.org/330173003/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:43: gc();
Maybe just call assertServiceWorkerIsNotCollected since that fun is these exact
two lines now.

[falken, 2014-06-16 08:29:21]

Thanks

https://codereview.chromium.org/330173003/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/service-worker-gc.html (right):

https://codereview.chromium.org/330173003/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/service-worker-gc.html:43: gc();
On 2014/06/16 05:00:23, dominicc wrote:
> Maybe just call assertServiceWorkerIsNotCollected since that fun is these
exact
> two lines now.

Good point, done.

[falken, 2014-06-16 08:29:32]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-06-16 08:30:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/330173003/120001

[commit-bot: I haz the power, 2014-06-16 10:11:28]

Change committed as 176197