Closed
Bug 1267733
Opened 8 years ago
Closed 8 years ago
service worker not updating when navigation interception fails
Categories
(Core :: DOM: Service Workers, defect)
Tracking
()
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
(Whiteboard: btpp-active)
Attachments
(12 files, 2 obsolete files)
I have a report of a production site in a broken state on firefox due to a service worker. See the twitter thread starting here: https://twitter.com/nekrtemplar/status/725012340660404224 The way this is supposed to work is that the site fixes its service worker and then an update loads the new/fixed service worker. Unfortunately we have a couple bugs here: 1) Since its a navigation, the update should be made regardless of how long since the last update has occurred. We don't do this, however, because the document load event doesn't fire since we ended up at about:neterror. 2) So we treat it as a non-navigation which should still trigger an update after 24 hours. Unfortunately we don't persist the last update time and have a bug where we won't fire a time based update until an update has been run another way. So these updates will never fire in this case. Fixing both of these problems should be very small bugs and I'd like to uplift as far as we can go. This is breaking a site in the wild so I'm asking to track this on the release channel.
Assignee | ||
Updated•8 years ago
|
Blocks: ServiceWorkers-compat
Assignee | ||
Comment 1•8 years ago
|
||
This fixes issue (2) from comment 0 by treating last update time as infinitely in the past after browser restart. Until we actually persist the last update time its better to just trigger one extra update after restart.
Attachment #8745470 -
Flags: review?(josh)
Updated•8 years ago
|
Attachment #8745470 -
Flags: review?(josh) → review+
Updated•8 years ago
|
Whiteboard: btpp-active
Assignee | ||
Comment 2•8 years ago
|
||
This is a mechanical patch to pass the ServiceWorkerRegistrationInfo down to the CancelChannelRunnable. This is setup for the next patch.
Attachment #8745541 -
Flags: review?(josh)
Assignee | ||
Comment 3•8 years ago
|
||
This patch makes us trigger an update regardless of last check time whenever a network request fails due to the service worker interception. This will make us update more frequently on problem sites, but that is much better than letting them get stuck longer in a broken state. Also note the spec says this under the soft update algorithm: "The user agent may call this as often as it likes to check for updates." This should allow sites to escape a broken service worker by shipping a new service worker script. This works in my local testing. I'll work on a test next.
Attachment #8745548 -
Flags: review?(josh)
Updated•8 years ago
|
Attachment #8745541 -
Flags: review?(josh) → review+
Updated•8 years ago
|
Attachment #8745548 -
Flags: review?(josh) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Last patch. This test verifies that we can recover from a bad navigation interception. It times out without the P1 to P3 patches. It passes with them. I apologize for the setTimeout(), but I wanted to verify the iframe actually failed to load. There is no good cross-browser way to detect this other than to set an artificial timeout. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ae53ce2091
Attachment #8745771 -
Flags: review?(josh)
Assignee | ||
Comment 5•8 years ago
|
||
Hmm, splinter is showing the diff from the files I hg cp'd to make the test. Sorry if that is confusing.
Assignee | ||
Comment 6•8 years ago
|
||
Updated the test so update-recovery-worker.py does not use the same cookie as update-worker.py. They were not playing well together. https://treeherder.mozilla.org/#/jobs?repo=try&revision=25d7767245cd
Attachment #8745771 -
Attachment is obsolete: true
Attachment #8745771 -
Flags: review?(josh)
Attachment #8745813 -
Flags: review?(josh)
Comment 7•8 years ago
|
||
We may be planning a dot release for mid next week. Tracking to make sure we uplift this at least to beta.
status-firefox49:
--- → affected
Comment 8•8 years ago
|
||
Comment on attachment 8745813 [details] [diff] [review] P4 Add a wpt test that verifies a service worker update can recover from a broken navigation interception. r=jdm Review of attachment 8745813 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/service-workers/service-worker/resources/update-recovery-worker.py @@ +7,5 @@ > # no-cache itself to ensure the user agent finds a new version for each update. > headers = [('Cache-Control', 'no-cache, must-revalidate'), > ('Pragma', 'no-cache')] > > + content_type = 'application/javascript' This isn't used. @@ +18,5 @@ > + extra_body = "addEventListener('fetch', function(e) { e.respondWith(Promise.reject()); });" > + elif mode == 'bad': > + # When the update tries to pull the script again, update to > + # a worker service worker that does not break document > + # navigation. Servce the same script from then on. Serves @@ +26,2 @@ > # Return a different script for each access. Use .time() and .clock() for > # best time resolution across different platforms. Comment doesn't make sense for this test.
Attachment #8745813 -
Flags: review?(josh) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Updated wpt test per review comments. I also changed the update-recovery-worker.py to clear its cookie when its done. This will let the test work with run-until-failure, etc.
Attachment #8745813 -
Attachment is obsolete: true
Attachment #8746046 -
Flags: review+
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75764a011b6e https://hg.mozilla.org/integration/mozilla-inbound/rev/c595e49da220 https://hg.mozilla.org/integration/mozilla-inbound/rev/23da91f0f80e https://hg.mozilla.org/integration/mozilla-inbound/rev/b396a6c17381
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8745470 [details] [diff] [review] Bug 1267733 P1 Treat last-update-time as infinitely in the past when checking for service worker update after restart. r=jdm Approval Request Comment [Feature/regressing bug #]: Service workers. [User impact if declined]: See comment 0. If a site accidentally pushes a broken service worker to its users it cannot then update the service worker again. This can effectively permanently brick their site. We need to fix the bug preventing us from triggering a service worker update for sites in this state. [Describe test coverage new/current, TreeHerder]: Existing mochitests and wpt tests. New wpt test verifying this particular fix. [Risks and why]: This is extremely low risk. We are using an existing update mechanism and just triggering it in these particular failure cases. [String/UUID change made/needed]: None.
Attachment #8745470 -
Flags: approval-mozilla-release?
Attachment #8745470 -
Flags: approval-mozilla-beta?
Attachment #8745470 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8745541 [details] [diff] [review] P2 Pass ServiceWorkerRegistrationInfo down to CancelChannelRunnable. r=jdm See comment 11.
Attachment #8745541 -
Flags: approval-mozilla-release?
Attachment #8745541 -
Flags: approval-mozilla-beta?
Attachment #8745541 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8745548 [details] [diff] [review] P3 Trigger service worker update after failed interception. r=jdm See comment 11.
Attachment #8745548 -
Flags: approval-mozilla-release?
Attachment #8745548 -
Flags: approval-mozilla-beta?
Attachment #8745548 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8746046 [details] [diff] [review] P4 Add a wpt test that verifies a service worker update can recover from a broken navigation interception. r=jdm See comment 11.
Attachment #8746046 -
Flags: approval-mozilla-release?
Attachment #8746046 -
Flags: approval-mozilla-beta?
Attachment #8746046 -
Flags: approval-mozilla-aurora?
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75764a011b6e https://hg.mozilla.org/mozilla-central/rev/c595e49da220 https://hg.mozilla.org/mozilla-central/rev/23da91f0f80e https://hg.mozilla.org/mozilla-central/rev/b396a6c17381
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 16•8 years ago
|
||
Do you have a sense of how widespread this issue is? Would it drive a point release on its own?
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16) > Do you have a sense of how widespread this issue is? Would it drive a point > release on its own? Right now it's only affecting one site. It's a risk for any site implementing service worker offline with Firefox, though. It's tough to say if it would drive it's own point release. I guess I'd like to do a point release if people are willing to avoid sites doing a UA version check to disable service workers.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 19•8 years ago
|
||
The patches apply to aurora, but need rebase for beta and release. Working on it.
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
Assignee | ||
Comment 22•8 years ago
|
||
Assignee | ||
Comment 23•8 years ago
|
||
I built these beta rebased patches locally and verified the WPT tests still pass.
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Comment 26•8 years ago
|
||
Assignee | ||
Comment 27•8 years ago
|
||
I locally built and tested these release branch patches as well.
Comment on attachment 8745470 [details] [diff] [review] Bug 1267733 P1 Treat last-update-time as infinitely in the past when checking for service worker update after restart. r=jdm P1 regression in SW, Aurora48+, Beta47+
Attachment #8745470 -
Flags: approval-mozilla-beta?
Attachment #8745470 -
Flags: approval-mozilla-beta+
Attachment #8745470 -
Flags: approval-mozilla-aurora?
Attachment #8745470 -
Flags: approval-mozilla-aurora+
Attachment #8745541 -
Flags: approval-mozilla-beta?
Attachment #8745541 -
Flags: approval-mozilla-beta+
Attachment #8745541 -
Flags: approval-mozilla-aurora?
Attachment #8745541 -
Flags: approval-mozilla-aurora+
Attachment #8745548 -
Flags: approval-mozilla-beta?
Attachment #8745548 -
Flags: approval-mozilla-beta+
Attachment #8745548 -
Flags: approval-mozilla-aurora?
Attachment #8745548 -
Flags: approval-mozilla-aurora+
Attachment #8746046 -
Flags: approval-mozilla-beta?
Attachment #8746046 -
Flags: approval-mozilla-beta+
Attachment #8746046 -
Flags: approval-mozilla-aurora?
Attachment #8746046 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 29•8 years ago
|
||
aurora: remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/4bfa10283a4e remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/67ab63244d1a remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/5c7d009b9784 remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/7c17eb918219 I have to run, but will land beta patches this evening.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 30•8 years ago
|
||
beta: remote: https://hg.mozilla.org/releases/mozilla-beta/rev/8ee93984adcf remote: https://hg.mozilla.org/releases/mozilla-beta/rev/c9d7a206612e remote: https://hg.mozilla.org/releases/mozilla-beta/rev/b7cef86d445d remote: https://hg.mozilla.org/releases/mozilla-beta/rev/8008dbbe1841
Comment 31•8 years ago
|
||
Comment on attachment 8745470 [details] [diff] [review] Bug 1267733 P1 Treat last-update-time as infinitely in the past when checking for service worker update after restart. r=jdm OK, let's take this for 46.0.1 so that service workers will behave correctly
Attachment #8745470 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Comment 32•8 years ago
|
||
Comment on attachment 8745470 [details] [diff] [review] Bug 1267733 P1 Treat last-update-time as infinitely in the past when checking for service worker update after restart. r=jdm I'm having cold feet, I'd like to talk with Ben or someone who can explain the impact more clearly first. Please find me on Monday morning to discuss!
Attachment #8745470 -
Flags: approval-mozilla-release+ → approval-mozilla-release-
Comment 33•8 years ago
|
||
Comment on attachment 8745470 [details] [diff] [review] Bug 1267733 P1 Treat last-update-time as infinitely in the past when checking for service worker update after restart. r=jdm After discussion with ben about impact, approved for m-r uplift
Attachment #8745470 -
Flags: approval-mozilla-release- → approval-mozilla-release+
Updated•8 years ago
|
Attachment #8745541 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•8 years ago
|
Attachment #8745548 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•8 years ago
|
Attachment #8746046 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Assignee | ||
Comment 34•8 years ago
|
||
Thanks! Landed: remote: https://hg.mozilla.org/releases/mozilla-release/rev/5784ab091ad1 remote: https://hg.mozilla.org/releases/mozilla-release/rev/2682c84f8645 remote: https://hg.mozilla.org/releases/mozilla-release/rev/80bb482b4a58 remote: https://hg.mozilla.org/releases/mozilla-release/rev/bfe772817f4c
You need to log in
before you can comment on or make changes to this bug.
Description
•