1244122 - service worker produces garbled output with "web developer toolbar" addon disabling http cache

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

I noticed some folks reporting bad output with service workers in firefox here:

  https://twitter.com/jgarber/status/693069807416029184

Got these steps to reproduce:

1. Unregister Service Workers in about:serviceworkers
2. Disable the browser's cache (I use the Web Developer Toolbar's "Disable Entire Cache" for this)
3. Visit https://whoistheorchid.com/
4. Reload about:serviceworkers and note a registered Service Worker
5. Reload https://whoistheorchid.com/ and observe garbled output

The key here is to use the "Web Developer Toolbar" to disable the cache and not our devtools.  That addon must be doing something different and confusing the service worker logic.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This only triggers in non-e10s.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

This is how the addon is disabling the cache:

// Toggles the cache
WebDeveloper.Overlay.Disable.toggleCache = function(element)
{
  WebDeveloper.Preferences.enablePreference(element, "browser.cache.disk.enable");
  WebDeveloper.Preferences.enablePreference(element, "browser.cache.memory.enable");
};

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Always support SW intercept even when http cache is disabled. r=mayhemer

I think at this point we need to have some indication in the http cache that we are doing a service worker interception.  This new flag could be backed out if we divorce service workers from cache in the future.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8713725 [details] [diff] [review]
P1 Always support SW intercept even when http cache is disabled. r=mayhemer

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

My original idea how to use the cache - that I didn't probably communicate well!! - was totally different from how jdm eventually implemented it.  My idea was to have a separate nsICacheStorage implementation that would be used to open entries when interception may happen, and not to play with entries...  However, jdm faced some issues and I mostly accepted his approach at the early stages also to save work (because my approach would at the start be a bit more complicated) but much more elegant than how the http channel hacks look now and what we have to do here, in this bug in particular...

I understand we need this flag.  Let's go with it.

But soon we should do either of two things:
- don't use cache at all (new thing to impl)
- use the cache properly (but my design idea first has to be reevaluated)

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> - use the cache properly (but my design idea first has to be reevaluated)

Filed bug 1244229 on that

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Oh, I had already filed bug 1201683.  Feel free to dupe to your new bug, though.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I am going to write a test before landing this.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Perform refresh testing with http cache disabled. r=ehsan

I decided to just disable the http cache during the test I wrote previously for testing the force-refresh service worker bypass.  I removed an extraneous refresh and locked down the asserts a bit more in the test too.  This fails without the P1 patch.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f48645e8cb8

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

There are some try failures I will need to sort out on monday.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

There is a single failure on browser_force_refresh.js on e10s here:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8396f9cbcc8&selectedJob=16164859

I did 100 retriggers and could not reproduce.  I'm inclined to land since this is apparently very low frequency.

Comment 12

[(no longer active)]

Do it!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1eacad41db4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d383f170be3f

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8713725 [details] [diff] [review]
P1 Always support SW intercept even when http cache is disabled. r=mayhemer

Approval Request Comment
[Feature/regressing bug #]: Service workers
[User impact if declined]: If the user has Web Developer Toolbar installed and set to disable http cache (or are flipping prefs themselves), then they will see garbled text on service worker enabled sites.
[Describe test coverage new/current, TreeHerder]: Tests included.
[Risks and why]: Minimal.  Only affects service workers.
[String/UUID change made/needed]: None.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8713756 [details] [diff] [review]
P2 Perform refresh testing with http cache disabled. r=ehsan

See comment 14.

Comment 16

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/41c4c65312d2. Our CI is totally discontinuous, running a random tiny percentage of the tests on each push, and the full set when it happens to feel like it, or not, but at least on OS X 10.6 opt e10s, that gave us a 50% chance of hitting https://treeherder.mozilla.org/logviewer.html#?job_id=20878975&repo=mozilla-inbound

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Execute browser_download.js service worker in separate scope. r=ehsan

The log shows we're getting a .ready promise callback before the .register promise resolves.  This suggests that a previous test's service worker is active for the scope and controlling the page which then screws up the test invariants.

There is only one previous browser chrome test for this directory; browser_download.js.  It does indeed have a racy teardown of its service worker.

This patch moves the service worker in browser_download.js to run in a separate scope to avoid cross-test interactions.  Lets see if that helps.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9336317f1358

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40a7c388a4b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e2ef4983a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/966415ddf784

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8714820 [details] [diff] [review]
P3 Execute browser_download.js service worker in separate scope. r=ehsan

See comment 14.

Comment 21

[(no longer active)]

Test changes don't require approval.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40a7c388a4b5
https://hg.mozilla.org/mozilla-central/rev/b8e2ef4983a9
https://hg.mozilla.org/mozilla-central/rev/966415ddf784

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8714820 [details] [diff] [review]
P3 Execute browser_download.js service worker in separate scope. r=ehsan

Improve service workers, taking it.
Should be in 45 beta 3.

Comment 24

[Carsten Book [:Tomcat]]

Hi Ben, when checkin-in to aurora i get:

remote: *************************** ERROR ***************************
remote: Push rejected because the following IDL interfaces were
remote: modified without changing the UUID:
remote:   - nsICacheStorage in changeset c610249bba01
remote: 
remote: To update the UUID for all of the above interfaces and their
remote: descendants, run:
remote:   ./mach update-uuids nsICacheStorage
remote: 
remote: If you intentionally want to keep the current UUID, include
remote: 'IGNORE IDL' in the commit message.
remote: *************************************************************

could you take a look ?

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: aurora/beta p1 patch

Here is a beta/aurora patch.  I tried landing it myself, but I don't know the right checkin message given it has UUID changes now.  Note, the IDL changes requiring the UUID are all backward compatible.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6600b943d180
https://hg.mozilla.org/releases/mozilla-aurora/rev/d07d740a8d99
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb00d903d285

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Is there still a problem with the beta uplift?

Comment 28

[Carsten Book [:Tomcat]]

for part 1 i get :

remote: ************************** ERROR ****************************
remote: 
remote: *** IDL file netwerk/cache2/nsICacheStorage.idl altered in changeset cc860536dba4***
remote: 
remote: Changes to IDL files in this repo require you to provide binary change approval in your top comment in the form of ba=... (or, more accurately, ba\S*=...)
remote: This is to ensure that UUID changes (or method changes missing corresponding UUID change) are caught early, before release.


is this expected ?

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

I saw that as well.  I'm not familiar with that commit hook, so I was hoping you would know what to do.  It is a UUID change, but its backward compatible (just adding a constant).  The UUID was not changed in 47 because we no longer require rev'ing UUIDs in newer code.

Comment 30

[(no longer active)]

I think we should land this patch with ba=someone.  Feel free to use my name.  :-)

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b0daf9e6c343
https://hg.mozilla.org/releases/mozilla-beta/rev/eb0d4cdcf860
https://hg.mozilla.org/releases/mozilla-beta/rev/f0641ff126c2

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8713725 [details] [diff] [review]
P1 Always support SW intercept even when http cache is disabled. r=mayhemer

If we do another point release, I'd like to request that all patches here be included as a ride along.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8713725 [details] [diff] [review]
P1 Always support SW intercept even when http cache is disabled. r=mayhemer

I don't see a strong reason for this to be uplifted to a dot release. I would prefer to let this ride the beta45 -> release45 train. Thanks!

Comment 34

[Sylvestre Ledru [:Sylvestre]]

Ehsan approved the ba.