1237158 - dom/canvas/test/test_offscreen_serviceworker.html should unregister service worker

Status: RESOLVED FIXED

Description

[Ben Kelly [:bkelly, not reviewing]]

+++ This bug was initially created as a clone of Bug #1232558 +++

While investigating bug 1232558 I noticed that we are triggering a lot of navigation-based updates for the scope:

  http://mochi.test:8888/tests/dom/canvas/test/

This is because test_offscreen_serviceworker.html does not unregister its service worker when its done.  All following tests in the directory then trigger an update for each navigation (modulo some coalescing due to the 1 second timer).

We should unregister and avoid all this unnecessary work.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Unregister service worker at end of test_offscreen_serviceworker.html. r=ehsan

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9495fb353d2c

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

In 20 pushes I didn't get a single instance of bug 1232558 with this patch applied.  It should help reduce the orange quite a bit.

Comment 3

[:Ehsan Akhgari]

Comment on attachment 8704466 [details] [diff] [review]
Unregister service worker at end of test_offscreen_serviceworker.html. r=ehsan

Review of attachment 8704466 [details] [diff] [review]:
-----------------------------------------------------------------

Holy moly!  Thanks for catching this.

Do you think it's worth failing a mochitest if it forgets to unregister a SW when it finishes in order to avoid running into this in the future?

::: dom/canvas/test/test_offscreencanvas_serviceworker.html
@@ +18,5 @@
>      if (msg.type == "test") {
>        ok(msg.result, msg.name);
>      }
>      if (msg.type == "finish") {
> +      dump('### ### calling unregister\n');

Nit: please remove this.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #3)
> Do you think it's worth failing a mochitest if it forgets to unregister a SW
> when it finishes in order to avoid running into this in the future?

I think that would be useful if we could put it in the mochitest harness somehow.  Also maybe create something like the pushPrefEnv() utility to execute a test with a service worker, but automatically unregister after the test.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Unregister service worker at end of test_offscreen_serviceworker.html. r=ehsan

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a34418d855

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I think we should uplift this to aurora to minimize orange on that branch.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8704685 [details] [diff] [review]
Unregister service worker at end of test_offscreen_serviceworker.html. r=ehsan

Approval Request Comment
[Feature/regressing bug #]: Service workers.
[User impact if declined]: Top 20 intermittent orange.
[Describe test coverage new/current, TreeHerder]: existing leak tests
[Risks and why]: None.  Test only change.
[String/UUID change made/needed]: None.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c6a34418d855

Comment 10

[Sylvestre Ledru [:sylvestre]]

Comment on attachment 8704685 [details] [diff] [review]
Unregister service worker at end of test_offscreen_serviceworker.html. r=ehsan

Improve the test suite, taking it.

FYI, you don't need approval for test-only changes.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/adfd2110f720