1450991 - service worker registrar file getting wiped

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Restarting firefox drops the registrations that were in memory and re-syncs to the file's reduced state.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Retry failed ServiceWorkerRegistrar saves and never wipe registrar data if saving fails. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=187677ccd0a51bd927fe726eb683b57ffff89603

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7881465e0c6b
Retry failed ServiceWorkerRegistrar saves and never wipe registrar data if saving fails. r=asuth

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7881465e0c6b

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

This is more of a stability issue vs compat.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

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.