When we adjust the ServiceWorkerRegistrar data we schedule a runnable to save to disk. The I/O is dispatched to StreamTransportService (STS): https://searchfox.org/mozilla-central/rev/2ce99e8054b0ff6ed1adf484aeaacacf2fea084c/dom/serviceworkers/ServiceWorkerRegistrar.cpp#825 STS is a thread pool. This means multiple runnables can get executed on different threads simultaneously. This could cause SaveData() to fail and trigger behavior like in bug 1450991. We should use a TaskQueue to dispatch the ServiceWorkerRegistrarSaveDataRunnable objects. We also need to adjust the retry mechanism from bug 1450991.

Comment on attachment 8965975 [details] [diff] [review] P1 Make ServiceWorkerRegistrar use a TaskQueue to avoid overlapping IO runnables. r=asuth I thought about this some more and I think it would be better to limit ourselves to one dispatched runnable at a time. This will deal with rapid state changes and effectively coallesce these changes into fewer writes.

This needs some cleanup and a limit on retries, but I think it mostly does the right thing. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b499f8ccfcd9dfb3e601adb29a4ba1dffc01084d

Andrew, this makes ServiceWorkerRegistrar only dispatch a single runnable at a time to save data to disk. It uses time stamps to determine if new data has been received since it capture the data and a new save is needed. It also implements the retry mechanism from the PBackground thread at the same place. If shutdown occurs both saving again and retrying failed saves are bypassed. I tested the retry mechanism locally by chown'ing serviceworker.txt to root ownership. I confirmed we only try to write the file 3 times before giving up. Reverting the file permissions allows us to write again on the next service worker change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aac3e9a30f10052418bdc1ddf42c615ec5785f4

Comment on attachment 8966628 [details] [diff] [review] Only allow one runnable to write to serviceworker.txt at a time. r=asuth Review of attachment 8966628 [details] [diff] [review]: ----------------------------------------------------------------- The core logic seems sound, but TimeStamp doesn't provide sufficient guarantees for `mDataTimeStamp <= mFileTimeStamp` to guarantee we don't suppress a mutation. But using a "generation" number that's incremented each time we mutate mData instead of overwriting with TimeStamp::Now() would. You already have the lock usage in-place, so this should be straightforward.

Updated to use generation counts instead of time stamps. I deal with count wrapping by resetting the generations back to zero when they become in sync after a successful write. This makes it very unlikely we will wrap given we would need a uint32_t's worth of pending writes. Note, I also added a comment about sketchy locking in ReadData(), but did not attempt to fix them here. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8222d819af5304c2946bb2f002c41a0c6d6e214c

Comment on attachment 8966672 [details] [diff] [review] Only allow one runnable to write to serviceworker.txt at a time. r=asuth Review of attachment 8966672 [details] [diff] [review]: ----------------------------------------------------------------- It's grrrrreat! ::: dom/serviceworkers/ServiceWorkerRegistrar.cpp @@ +880,5 @@ > + new ServiceWorkerRegistrarSaveDataRunnable(Move(data), generation); > + nsresult rv = target->Dispatch(runnable.forget(), NS_DISPATCH_NORMAL); > + NS_ENSURE_SUCCESS_VOID(rv); > + > + mRunnableDispatched = true;; typo nit: s/;;/;/

Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/19d7d934850c Only allow one runnable to write to serviceworker.txt at a time. r=asuth