service worker registrar file getting wiped

VERIFIED FIXED in Firefox 61

Status

()

defect
P2
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment)

Today I was trying to investigate another bug and looked in my serviceworker.txt file.   To my surprise it was almost completely empty even though I have like 10 or 20 service workers in about:debugging#workers.

This was the first load of the browser after updating nightly to the latest.  Is the file lost on update?

Re-visiting sites to get the service worker installed again start populating the txt file again.
Restarting firefox drops the registrations that were in memory and re-syncs to the file's reduced state.
(Assignee)

Updated

a year ago
Summary: service worker registrar file getting wiped → service worker registrar file getting wiped (maybe on update?)
Status: NEW → UNCONFIRMED
Ever confirmed: false
Priority: -- → P2
Summary: service worker registrar file getting wiped (maybe on update?) → service worker registrar file getting wiped
My serviceworker.txt file got corrupted on this windows machine again.  I checked after an update and the txt file was missing many entries that were in about:debugging#workers before updating.

I'm still not sure if its the update doing the damage or if its getting corrupted while running and then I notice after update.  Next time I will copy the txt file off so I can do an exact diff.
This is happening on a daily basis on my windows machine.  I don't think its related to updates.  It seems if we hit any error conditions in the WriteData() method we delete all the existing data.
Assignee: nobody → bkelly
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8965831 [details] [diff] [review]
Retry failed ServiceWorkerRegistrar saves and never wipe registrar data if saving fails. r=asuth

Andrew, this is a bit of a speculative patch to try fix the problem on my windows machine.

It seems that ServiceWorkerRegistrar wipes both the disk file and the registrar's copy of its registration whenever WriteData() returns an error.  This means any further updates from SWM will only append new registrations, but all old registrations are completely lost unless the unregister and re-register.

This seems unnecessary to me since the code uses an nsISafeOutputStream:

https://searchfox.org/mozilla-central/source/xpcom/io/nsISafeOutputStream.idl

Also, one windows disk writes can definitely fail temporarily due to virus scanning, etc.  Since I only see this on windows I'm inclined to believe that is really happening here.

This patch removes the DeleteData() call if we fail to WriteData() during the SaveData() operation.  It also adds a single retry if SaveData() fails.  I don't want to retry too long so that shutdown is delayed excessively, but an async retry might handle a large number of temporary failures on windows.

Unfortunately I have no idea how to test this other than to just try it for a few days on this machine.
Attachment #8965831 - Flags: review?(bugmail)
Comment on attachment 8965831 [details] [diff] [review]
Retry failed ServiceWorkerRegistrar saves and never wipe registrar data if saving fails. r=asuth

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

Restating: The current code both a) uses nsISafeOutputStream, which guarantees atomic file updates, and b1) deletes the file on any failure to write to the file PLUS it b2) wipes out its entire canonical store of registrations (the mData.clear() call).  "b1" makes some sense from a privacy perspective but "b2" seems like the unintentional byproduct of an attempt to make DeleteData() maintain a state invariant without thinking through the potential risk for data-loss.  Given that we leak cache storage for ServiceWorkers whose registrations disappear, on balance losing "b1" seems like an improvement.
Attachment #8965831 - Flags: review?(bugmail) → review+

Comment 8

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7881465e0c6b
Retry failed ServiceWorkerRegistrar saves and never wipe registrar data if saving fails. r=asuth
https://hg.mozilla.org/mozilla-central/rev/7881465e0c6b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This is more of a stability issue vs compat.
(Assignee)

Updated

a year ago
Blocks: 1452373
(Assignee)

Updated

a year ago
Blocks: 1452374
I have not seen this happen since the nightly where this bug landed.  Since I was getting wipes on a daily or weekly basis before I'm going to mark this verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.