Closed
Bug 1237158
Opened 10 years ago
Closed 10 years ago
dom/canvas/test/test_offscreen_serviceworker.html should unregister service worker
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: bkelly, Assigned: bkelly)
References
Details
Attachments
(1 file, 1 obsolete file)
1.47 KB,
patch
|
bkelly
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8704466 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8704466 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8704466 -
Attachment is obsolete: true
Attachment #8704685 -
Flags: review+
Assignee | ||
Comment 7•10 years ago
|
||
I think we should uplift this to aurora to minimize orange on that branch.
Assignee | ||
Comment 8•10 years ago
|
||
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.
Attachment #8704685 -
Flags: approval-mozilla-aurora?
Comment 9•10 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 10•10 years ago
|
||
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.
Attachment #8704685 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 11•10 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•