Remove duplicate service worker test

Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2873373004
Cr-Commit-Position: refs/heads/master@{#473227}
Committed: https://chromium.googlesource.com/chromium/src/+/51eb76c3327468323acfea2393253f846d5aea25

[mike3, 2017-05-10 20:25:33]

Hi Mek,

Here's the diff between Chromium's version and WPT's version:

https://gist.github.com/jugglinmike/152032e504251f1e3319dd32e02b70ae

WPT makes additional provisions for various worker/registration states. It
looks like they guard against race conditions where the waiting worker could
become activated while the client's Promises are being resolved. I think this
precaution is appropriate, but I'm calling it out here because if it's actually
too lenient, we should probably remove it in this CL.

The diff does not reference the resource files because they are all
byte-for-byte identical.

[falken, 2017-05-11 00:36:56]

On 2017/05/10 20:25:33, mike3 wrote:
> Hi Mek,
> 
> Here's the diff between Chromium's version and WPT's version:
> 
> https://gist.github.com/jugglinmike/152032e504251f1e3319dd32e02b70ae
> 
> WPT makes additional provisions for various worker/registration states. It
> looks like they guard against race conditions where the waiting worker could
> become activated while the client's Promises are being resolved. I think this
> precaution is appropriate, but I'm calling it out here because if it's
actually
> too lenient, we should probably remove it in this CL.

FYI I remember discussion about this...somewhere...but can't find it now.

If I recall, Mozilla's implementation needs this because their service worker
advances to 'activated' "faster" than Chrome's.

Mozilla added this in https://hg.mozilla.org/mozilla-central/rev/1ea2ebce35fe
but the description just says "Similar to other tests, the registration process
allows workers to activate right after install."

> 
> The diff does not reference the resource files because they are all
> byte-for-byte identical.

[mike3, 2017-05-11 17:39:12]

On 2017/05/11 00:36:56, falken wrote:
> If I recall, Mozilla's implementation needs this because their service worker
> advances to 'activated' "faster" than Chrome's.

That sounds like a probable motivation. On my first reading, it didn't seem like
a spec violation. Actually, it made me question whether most of the tests would
need to be updated to account for these cases.

I've been talking with Ben Kelly on IRC, though, and it seems that the service
workers specification is intentionally structured to provide certain guarantees
about registration state during certain turns of the event loop. So it may be
that the precautions we're discussing are a bug workaround, after all.

Ben has always been really helpful in diagnosing stuff like this. I'm going to
see if he can comment here.

[falken, 2017-05-15 05:26:57]

On 2017/05/11 17:39:12, mike3 wrote:
> On 2017/05/11 00:36:56, falken wrote:
> > If I recall, Mozilla's implementation needs this because their service
worker
> > advances to 'activated' "faster" than Chrome's.
> 
> That sounds like a probable motivation. On my first reading, it didn't seem
like
> a spec violation. Actually, it made me question whether most of the tests
would
> need to be updated to account for these cases.
> 
> I've been talking with Ben Kelly on IRC, though, and it seems that the service
> workers specification is intentionally structured to provide certain
guarantees
> about registration state during certain turns of the event loop. So it may be
> that the precautions we're discussing are a bug workaround, after all.
> 
> Ben has always been really helpful in diagnosing stuff like this. I'm going to
> see if he can comment here.

I think I found the discussion I was thinking of:
https://github.com/w3c/ServiceWorker/issues/622

[mike3, 2017-05-15 18:33:03]

On 2017/05/15 05:26:57, falken wrote:
> I think I found the discussion I was thinking of:
> https://github.com/w3c/ServiceWorker/issues/622

I'm a little confused now. We're currently discussing the necessity of logic
added by the Firefox project. The issue you've referenced seems to explain that
code, but it concerns Chromium, and I don't think equivalent logic was ever
implemented in Chromium's version of the test.

As an alternative to figuring out how/why that code was added, I've tried to
work out the expected behavior based on the latest specification text. Here's
my summary of the relevant algorithms, limited to the steps that I think are
relevant:

- Install
  1. Update Registration State (set "installing"; **push new task** for each
worker)
  2. Update Worker State ("installing"; for each worker: **push new task** to
update property and dispatch "statechange" event)
  3. Dispatch "install" lifecycle event (**push new task**)
  4. **Wait for above task**
  5. Update Registration State (set "waiting" and unset "installing"; **push new
task** for each worker)
  6. Update Worker State ("installed"; for each worker: **push new task** to
update property and dispatch "statechange" event))
  7. **Wait for above tasks**
  - Try Activate
    - Activate
      8. Update Registration State (set "active" and unset "waiting"; **push new
task** for each worker)
      9. Update Worker State ("activating"; for each worker: **push new task**
to update property and dispatch "statechange" event)
      10. Dispatch "activate" lifecycle event (**push new task**)
      11. **Wait for above task**
      12. Update Worker State ("activated"; for each worker: **push new task**
to update property and dispatch "statechange" event)

Applying these to the test in question:

- The first Promise handler is invoked as a microtask of the task created in
  step 2 . This microtask registers an event listener for "statechange" via
  `wait_for_install`. At this point, the Install algorithm is still holding at
  step 4 (since the task created in step 3 is still queued), which is why
  `registraton.installing` is guaranteed to be set.
- The second Promise handler is invoked as a microtask of the task created in
  step 6 . Here, the Install algorithm is still holding at step 7 since step 6
  won't be complete until the microtask queue is empty. This means that
  `registration.waiting` is guaranteed to be set--it isn't un-set until step 8.

So the `registration.active`-related conditions expressed in that second
handler do not seem possible in conforming implementations. In other words, I
believe those branches are a bug workaround and therefore not appropriate for
Web Platform Tests.

[Marijn Kruisselbrink, 2017-05-16 18:58:21]

On 2017/05/15 at 18:33:03, mike wrote:
> - The first Promise handler is invoked as a microtask of the task created in
>   step 2 . This microtask registers an event listener for "statechange" via
>   `wait_for_install`. At this point, the Install algorithm is still holding at
>   step 4 (since the task created in step 3 is still queued), which is why
>   `registraton.installing` is guaranteed to be set.

I might be missing something, but why is at this point the task created in step
3 still queued? The task and microtasks from step 2 are in an entirely separate
event loop from the ones in step 3, right? And "install" only waits for the task
from step 3. So it seems legal for the task from step 3 to finish and for the
algorithm to have ran all the way till your step 7 before the task queued in
step 2 even begins to execute.

Of course since all modifications to state and waiting/installing (in the
context of the client) are done as tasks posted on the clients event loop I
think your overall analysis is still correct. When the microtask for the first
promise handler runs, none of the later queued tasks have ran yet, even though
they probably are queued at that point. 

So yes, I agree that the extra checks should not be needed and the spec
guarantees that conforming implementations should not be able to hit any of the
.active checks.

[falken, 2017-05-17 07:07:26]

On 2017/05/16 18:58:21, Marijn Kruisselbrink wrote:
> On 2017/05/15 at 18:33:03, mike wrote:
> > - The first Promise handler is invoked as a microtask of the task created in
> >   step 2 . This microtask registers an event listener for "statechange" via
> >   `wait_for_install`. At this point, the Install algorithm is still holding
at
> >   step 4 (since the task created in step 3 is still queued), which is why
> >   `registraton.installing` is guaranteed to be set.
> 
> I might be missing something, but why is at this point the task created in
step
> 3 still queued? The task and microtasks from step 2 are in an entirely
separate
> event loop from the ones in step 3, right? And "install" only waits for the
task
> from step 3. So it seems legal for the task from step 3 to finish and for the
> algorithm to have ran all the way till your step 7 before the task queued in
> step 2 even begins to execute.
> 
> Of course since all modifications to state and waiting/installing (in the
> context of the client) are done as tasks posted on the clients event loop I
> think your overall analysis is still correct. When the microtask for the first
> promise handler runs, none of the later queued tasks have ran yet, even though
> they probably are queued at that point. 
> 
> So yes, I agree that the extra checks should not be needed and the spec
> guarantees that conforming implementations should not be able to hit any of
the
> .active checks.

I probably derailed the conversation with that link, sorry. Looking at the
current spec is probably better than trying to dig up old conversations.

It sounds like it's fine to remove the extra checks and if Mozilla starts
failing we could resume the conversation on Github.

[mike3, 2017-05-18 17:10:35]

Description was changed from

==========
Remove duplicate service worker test

This test is equivalent to the version available in the Web Platform
Tests project.

BUG=688116
R=mek@chromium.org
==========

to

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=mek@chromium.org
==========

[mike3, 2017-05-18 17:11:16]

On 2017/05/17 07:07:26, falken wrote:
> On 2017/05/16 18:58:21, Marijn Kruisselbrink wrote:
> > On 2017/05/15 at 18:33:03, mike wrote:
> > > - The first Promise handler is invoked as a microtask of the task created
in
> > >   step 2 . This microtask registers an event listener for "statechange"
via
> > >   `wait_for_install`. At this point, the Install algorithm is still
holding
> at
> > >   step 4 (since the task created in step 3 is still queued), which is why
> > >   `registraton.installing` is guaranteed to be set.
> > 
> > I might be missing something, but why is at this point the task created in
> step
> > 3 still queued? The task and microtasks from step 2 are in an entirely
> separate
> > event loop from the ones in step 3, right? And "install" only waits for the
> task
> > from step 3. So it seems legal for the task from step 3 to finish and for
the
> > algorithm to have ran all the way till your step 7 before the task queued in
> > step 2 even begins to execute.
> > 
> > Of course since all modifications to state and waiting/installing (in the
> > context of the client) are done as tasks posted on the clients event loop I
> > think your overall analysis is still correct. When the microtask for the
first
> > promise handler runs, none of the later queued tasks have ran yet, even
though
> > they probably are queued at that point. 
> > 
> > So yes, I agree that the extra checks should not be needed and the spec
> > guarantees that conforming implementations should not be able to hit any of
> the
> > .active checks.
> 
> I probably derailed the conversation with that link, sorry. Looking at the
> current spec is probably better than trying to dig up old conversations.
> 
> It sounds like it's fine to remove the extra checks and if Mozilla starts
> failing we could resume the conversation on Github.

Okay, I've removed that logic and updated the CL's description. Thanks!

[falken, 2017-05-19 02:19:04]

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

[falken, 2017-05-19 02:19:04]

lgtm

https://codereview.chromium.org/2873373004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/onactivate-script-error.https.html
(right):

https://codereview.chromium.org/2873373004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/onactivate-script-error.https.html:41:
});
I think we need to unregister the worker.

[mike3, 2017-05-19 15:53:36]

Description was changed from

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=mek@chromium.org
==========

to

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=mek@chromium.org
==========

[mike3, 2017-05-19 15:54:19]

Description was changed from

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=mek@chromium.org
==========

to

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=falken@chromium.org
==========

[mike3, 2017-05-19 15:54:51]

Thanks, Matt!

https://codereview.chromium.org/2873373004/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/onactivate-script-error.https.html
(right):

https://codereview.chromium.org/2873373004/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/onactivate-script-error.https.html:41:
});
On 2017/05/19 02:19:04, falken wrote:
> I think we need to unregister the worker.

Done.

[mike3, 2017-05-19 15:54:57]

The CQ bit was checked by mike@mikepennisi.com

[mike3, 2017-05-19 15:54:57]

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/2873373004/#ps60001
(title: "Un-register workers following test completion")

[commit-bot: I haz the power, 2017-05-19 15:55:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 17:26:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495209297275660,
"parent_rev": "82369251bef0ac33df90166ce95fdaf5aaf53ee6", "commit_rev":
"51eb76c3327468323acfea2393253f846d5aea25"}

[commit-bot: I haz the power, 2017-05-19 17:26:58]

Description was changed from

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=falken@chromium.org
==========

to

==========
Remove duplicate service worker test

This test exists in both WPT and the Chromium project tree. The WPT
version is slightly weaker because it accounts for a condition that is
not possible according to the latest version of the Service Workers
specification [1].

> # Install
>
> [...]
>
> 16. Run the Update Registration State algorithm passing registration,
>     "waiting" and registration’s installing worker as the arguments.
> 17. Run the Update Registration State algorithm passing registration,
>     "installing" and null as the arguments.
> 18. Run the Update Worker State algorithm passing registration’s
>     waiting worker and installed as the arguments.
> 19. Invoke Finish Job with job.
> 20. Wait for all the tasks queued by Update Worker State invoked in
>     this algorithm have executed.
> 21. Invoke Try Activate with registration.

Due to step 20, worker activation does not occur until all prior tasks
(which includes microtasks such as the Promise handler microtask created
in the test's body) have executed.

Remove the conditional logic from the WPT version and remove the
Chromium test file. Explicitly un-register workers following test
completion.

[1] https://w3c.github.io/ServiceWorker/#installation-algorithm,
    retrieved on 17.05.18

BUG=688116
R=falken@chromium.org

Review-Url: https://codereview.chromium.org/2873373004
Cr-Commit-Position: refs/heads/master@{#473227}
Committed:
https://chromium.googlesource.com/chromium/src/+/51eb76c3327468323acfea239325...
==========

[commit-bot: I haz the power, 2017-05-19 17:27:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/51eb76c3327468323acfea239325...