1452373 - Don't allow multiple ServiceWorkerRegistrarSaveDataRunnable objects to execute at the same time

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistrar use a TaskQueue to avoid overlapping IO runnables. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Make ServiceWorkerRegistrar use a TaskQueue to avoid overlapping IO runnables. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cad15a39003f3341a85e3675f61468cac7c444d

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cad15a39003f3341a85e3675f61468cac7c444d

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only allow one runnable to write to serviceworker.txt at a time. r=asuth

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only allow one runnable to write to serviceworker.txt at a time. r=asuth

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 7

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

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only allow one runnable to write to serviceworker.txt at a time. r=asuth

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 9

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

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/;;/;/

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Only allow one runnable to write to serviceworker.txt at a time. r=asuth

Comment 11

[Pulsebot]

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

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/19d7d934850c