ServiceWorker: Fix messed registration attributes on ServiceWorkerGlobalScope

ServiceWorker: Fix messed registration attributes on ServiceWorkerGlobalScope

This is the fix for regression introduced by
https://codereview.chromium.org/1344783002

After the CL, SWProviderContext::ControllerDelegate::AssociateRegistration()
wrongly set its |installing_| field to the active worker and |active_| field
to nullptr. These messed fields eventually show up as
ServiceWorkerGlobalScope.registration's version attributes.

This CL fixes the wrong field assignment and adds layout tests.

BUG=532098, 557551
Committed: https://crrev.com/9cbeba1626fffae8cc657e63e44abc59dec84b17
Cr-Commit-Position: refs/heads/master@{#360557}

[nhiroki, 2015-11-18 08:48:57]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-11-18 09:23:32]

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

[nhiroki, 2015-11-18 09:23:33]

PTAL, thanks!

[nhiroki, 2015-11-18 11:19:37]

Description was changed from

==========
ServiceWorker: Fix messed registration attributes on ServiceWorkerGlobalScope

This is the fix for regression introduced by
https://codereview.chromium.org/1344783002

After the CL, SWProviderContext::ControllerDelegate::AssociateRegistration()
wrongly set its |installing_| field to the active worker and |active_| field
to nullptr. These messed fields eventually show up as
ServiceWorkerGlobalScope.registration's version attributes.

This CL fixes the wrong field assignment and adds layout tests.

BUG=557551
==========

to

==========
ServiceWorker: Fix messed registration attributes on ServiceWorkerGlobalScope

This is the fix for regression introduced by
https://codereview.chromium.org/1344783002

After the CL, SWProviderContext::ControllerDelegate::AssociateRegistration()
wrongly set its |installing_| field to the active worker and |active_| field
to nullptr. These messed fields eventually show up as
ServiceWorkerGlobalScope.registration's version attributes.

This CL fixes the wrong field assignment and adds layout tests.

BUG=532098, 557551
==========

[falken, 2015-11-19 01:29:22]

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html
(right):

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:56:
var sawMessage = new Promise(function(resolve) {
nit: saw_message

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:96:
return service_worker_unregister_and_done(t, scope);
add_completion_callback instead?

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js
(right):

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js:1:
var results = [];
A lot of our tests do this, but I think we shouldn't rely on global state
surviving between events in a W3C test. Can you add a TODO? Or I think you may
be able to restructure the test to move the expectations into the worker, so it
cries immediately when something fails?

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js
(right):

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js:8:
// handlded as timeout. See registration-attribute-newer-worker.js for example.
Ah I see this worker does the global state thing too...

[falken, 2015-11-19 01:30:33]

On 2015/11/19 01:29:22, falken wrote:
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html
> (right):
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:56:
> var sawMessage = new Promise(function(resolve) {
> nit: saw_message
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:96:
> return service_worker_unregister_and_done(t, scope);
> add_completion_callback instead?
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js
> (right):
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js:1:
> var results = [];
> A lot of our tests do this, but I think we shouldn't rely on global state
> surviving between events in a W3C test. Can you add a TODO? Or I think you may
> be able to restructure the test to move the expectations into the worker, so
it
> cries immediately when something fails?
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js
> (right):
> 
>
https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js:8:
> // handlded as timeout. See registration-attribute-newer-worker.js for
example.
> Ah I see this worker does the global state thing too...

I guess maybe we should just do TODOs for now, but should think about how to fix
this in this future.

[nhiroki, 2015-11-19 09:02:55]

Thank you for reviewing!

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html
(right):

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:56:
var sawMessage = new Promise(function(resolve) {
On 2015/11/19 01:29:22, falken wrote:
> nit: saw_message

Done.

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/registration-attribute.html:96:
return service_worker_unregister_and_done(t, scope);
On 2015/11/19 01:29:22, falken wrote:
> add_completion_callback instead?

Done.

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js
(right):

https://codereview.chromium.org/1460653002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-newer-worker.js:1:
var results = [];
On 2015/11/19 01:29:22, falken wrote:
> A lot of our tests do this, but I think we shouldn't rely on global state
> surviving between events in a W3C test. Can you add a TODO? Or I think you may
> be able to restructure the test to move the expectations into the worker, so
it
> cries immediately when something fails?

I agree that we should avoid global states. I'm thinking of some options...


1) having assertions in the worker side
As I commented on registration-attribute-worker.js, worker-side assertion
failures cannot be shown on the result page and are handled as timeout. This
could make debugging hard, so I'd prefer to keep them in the page side if
possible.

2) keeping results in storage (CacheStorage or IDB?)
Regarding CacheStorage, it might be difficult to wait for storing all results on
the message event because it involves multiple async operations (opening the
cache, putting results, ...) and some of events are not waitable (not
ExtendableEvent).

Regarding IDB, we could handle it with transaction, but it seems like
overkill...

3) sending results to the page on each event
On evaluation phase, posting messages via Client object could be the only way to
communicate to the page like following code:

  // worker.js
  function record(event_name) {
    var result = event_name + '...';
    self.clients.matchAll({includeUncontrolled: true})
      .then(function(clients) { clients[0].postMessage(result); });
  }

but currently clients.matchAll() never returns on the evaluation phase (see
issue 533130) and this is not feasible...


IMO, leaving TODO comments for now and working on (3) after the issue is fixed
would be the simplest solution. What do you think?

[falken, 2015-11-19 09:07:29]

lgtm 

Thanks for explaining the approaches! I agree #3 sounds best, with TODOs in the
interim.

Can you make a tracking bug for that work, maybe copy/pasting what you wrote
here?

https://codereview.chromium.org/1460653002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js
(right):

https://codereview.chromium.org/1460653002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js:4:
var events_seen = [];
Can you add a similar comment about global states here. Better yet should we
make a tracking bug?

[nhiroki, 2015-11-19 09:34:09]

Thank you!

https://codereview.chromium.org/1460653002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js
(right):

https://codereview.chromium.org/1460653002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/ServiceWorkerGlobalScope/resources/registration-attribute-worker.js:4:
var events_seen = [];
On 2015/11/19 09:07:29, falken wrote:
> Can you add a similar comment about global states here. Better yet should we
> make a tracking bug?

Done.

[nhiroki, 2015-11-19 09:34:14]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-11-19 09:34:15]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1460653002/#ps60001
(title: "add comments")

[commit-bot: I haz the power, 2015-11-19 09:35:19]

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

[commit-bot: I haz the power, 2015-11-19 10:44:48]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-11-19 10:45:33]

Patchset 3 (id:??) landed as
https://crrev.com/9cbeba1626fffae8cc657e63e44abc59dec84b17
Cr-Commit-Position: refs/heads/master@{#360557}