1424300 - investigate why the service worker job queue is stuck for twitter in bug 1409115

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

We are seeing leaks on twitter in bug 1409115.  This is because the service worker register() promise is never resolving because the job queue is stuck.  While Andrew is working on a fix to avoid leaking through promises like this, we should also figure out what is breaking the SW job queue.

Also see bug 1424299 for adding a timeout and report mechanism for stuck job queues.

I have a broken profile that shows this behavior, so I will investigate in the next week or two.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I think I figured this out.  The old cache exists, but is empty.  So we never perform any fetches here:

https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/dom/workers/ServiceWorkerScriptCache.cpp#471

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

This fixes the problem for me locally.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

Eden, can you review this patch in the ServiceWorkerScriptCache?

The bug here is in the service worker update mechanism.  During update we try to look at our currently installed script and importScripts() resources.  We do a fetch for each one and compare.

The problem here is that sometimes it seems we can get a cache that does not contain any entries.  So we get len == 0 here and don't start any fetches.  This leaves the job hung.

My proposed fix here is to always perform the top level main script fetch.  We use the keys to generate fetches for importScripts() only.  This allows us to also handle any corner cases with the main script being missing, but other resources being there.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I'm going to try to write a test.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

I think I have some more changes to make.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Store response URLs in the service worker script cache. r=edenchuang

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=843aced16121bdeaa158d074da662519593b12ee

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

This patch is unchanged.  Please see comment 3 for description.

In addition, I figured out why we sometimes get a Cache object with no entries.  Its because if you manually delete the cache we just create a new empty Cache for you automatically.  So if something blows away the storage directory from the user's profile you end up with this situation.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936682 [details] [diff] [review]
P2 Store response URLs in the service worker script cache. r=edenchuang

This patch is not strictly necessary, but its nice for writing tests and other diagnostics.  It makes us store the Response.url in Cache API for our various service worker script resources.

I also fix a very dubious nsresult usage here.  The code was setting an rv2 but checking rv instead.  I don't see any reason for the rv2, so lets remove it.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936683 [details] [diff] [review]
P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

This patch adds a mochitest.  It verifies that if we manually delete the service worker script cache data we can still update and restore the service worker.

On current mozilla-central this test times out because the update gets stuck and never completes.  With the patches applied it passes.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I checked our profile refresh feature in case it was triggering this.  Fortunately it does not trigger it.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

I believe we used to handle this before bug 1290951, so its a regression.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

While the code was implemented in bug 1290951, it wasn't enabled until bug 1353636.  So this problem only effects 57+.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

I have a small tweak to the test make it past --verify, but leaving the review flag as is for now.  It doesn't change the general gist of the test.  I just need to do one extra thing to cleanup so the test can re-run consecutively in the same process.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

Actually, lets just update the test.  This just adds an extra waitForState() after the update to make --verify pass.  Please see previous description of the patch.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50a3d991b3b9f6cb000569e4d4b710f9fc051224

Comment 18

[Eden Chuang[:edenchuang]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

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

Thanks. The explanation is very clear, I have nothing to comment.

Comment 19

[Eden Chuang[:edenchuang]]

Comment on attachment 8936694 [details] [diff] [review]
P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

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

Clean up the cache at the end of the test.

::: dom/workers/test/serviceworkers/test_bad_script_cache.html
@@ +57,5 @@
> +    await waitForState(reg.installing, 'activated');
> +
> +    // Verify that the script cache knows about the worker script again.
> +    response = await chromeCaches.match(scriptURL.href);
> +    is(response.url, scriptURL.href, 'worker script should be stored');

call deleteCaches(chromeCaches) again to make sure the test does not contaminate the test environment.

might be the root cause of https://treeherder.mozilla.org/logviewer.html#?job_id=151554852&repo=try&lineNumber=2177

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Eden Chuang[:edenchuang] from comment #19)
> ::: dom/workers/test/serviceworkers/test_bad_script_cache.html
> @@ +57,5 @@
> > +    await waitForState(reg.installing, 'activated');
> > +
> > +    // Verify that the script cache knows about the worker script again.
> > +    response = await chromeCaches.match(scriptURL.href);
> > +    is(response.url, scriptURL.href, 'worker script should be stored');
> 
> call deleteCaches(chromeCaches) again to make sure the test does not
> contaminate the test environment.

This should not be necessary since we do the service worker unregister().  The service worker code should remove its cache when the file is deleted.

> might be the root cause of
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=151554852&repo=try&lineNumber=2177

I'm going to investigate this further, but I think its a Cache API bug.  I'll either do another patch in this bug or file a separate follow-on bug.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936694 [details] [diff] [review]
P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

See comment 20.  Re-flagging because I think the verify failure on windows is a different bug in Cache API.

Comment 22

[Julien Cristau [:jcristau] (back April 22)]

tracking as recent regression

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

I looked at the assert for a few hours this morning.  AFAICT Cache API is doing everything correctly.  It just seems like something during shutdown (QuotaManager?) is scanning files and has a lock on the body file.  So the windows OS refuses to delete it.

If I add a manual GC at the end of the test the assert goes away.  I'm going to go ahead and include that in the test since I don't see any other problems here.  Unfortunately we can't prevent other code from touching these files.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/474703f75734
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang
https://hg.mozilla.org/integration/mozilla-inbound/rev/e48d2f34e8f9
P2 Store response URLs in the service worker script cache. r=edenchuang
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e183521e2de
P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1290951 and Bug 1353636
[User impact if declined]: If the users profile loses storage for a site using service workers the service worker API may stop working.  It does so in a way that can create a serious memory leak for sites.  Its unclear why storage for the site is being removed/corrupted, but there can be many causes including anti-virus, user poking around in their profile, etc.
[Is this code covered by automated tests?]: P3 includes a mochitest
[Has the fix been verified in Nightly?]: Verified locally, but it has not landed on m-c or a nightly build yet.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: The code changes are relatively small and self contained.  We have a good existing set of tests for this code to avoid regressions.
[String changes made/needed]: None

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936682 [details] [diff] [review]
P2 Store response URLs in the service worker script cache. r=edenchuang

See comment 26.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8936882 [details] [diff] [review]
P3 Add a mochitest verifying we can recover if the script cache is deleted. r=edenchuang

See comment 26.

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/474703f75734
https://hg.mozilla.org/mozilla-central/rev/e48d2f34e8f9
https://hg.mozilla.org/mozilla-central/rev/9e183521e2de

Comment 30

[Gerry Chang [:gchang]]

Comment on attachment 8936655 [details] [diff] [review]
P1 Gracefully handle when the service worker script cache exists, but is empty. r=edenchuang

Fix a potential leak issue. Beta58+.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/c200bc68074a
https://hg.mozilla.org/releases/mozilla-beta/rev/a4c977f0603b
https://hg.mozilla.org/releases/mozilla-beta/rev/4121dc99f134

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/44c9c219f7a5

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Sorry, not sure how that extra moxhitest.ini line slipped into my patch.