Closed Bug 1244122 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(4 files)

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.
This only triggers in non-e10s.
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");
};
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.
Attachment #8713725 - Flags: review?(honzab.moz)
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)
Attachment #8713725 - Flags: review?(honzab.moz) → review+
(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
Oh, I had already filed bug 1201683.  Feel free to dupe to your new bug, though.
I am going to write a test before landing this.
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.
Attachment #8713756 - Flags: review?(ehsan)
Attachment #8713756 - Flags: review?(ehsan) → review+
There are some try failures I will need to sort out on monday.
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.
Do it!
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.
Attachment #8713725 - Flags: approval-mozilla-beta?
Attachment #8713725 - Flags: approval-mozilla-aurora?
Comment on attachment 8713756 [details] [diff] [review]
P2 Perform refresh testing with http cache disabled. r=ehsan

See comment 14.
Attachment #8713756 - Flags: approval-mozilla-beta?
Attachment #8713756 - Flags: approval-mozilla-aurora?
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
Duplicate of this bug: 1244977
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
Attachment #8714820 - Flags: review?(ehsan)
Attachment #8714820 - Flags: review?(ehsan) → review+
Comment on attachment 8714820 [details] [diff] [review]
P3 Execute browser_download.js service worker in separate scope. r=ehsan

See comment 14.
Attachment #8714820 - Flags: approval-mozilla-beta?
Attachment #8714820 - Flags: approval-mozilla-aurora?
Test changes don't require approval.
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.
Attachment #8714820 - Flags: approval-mozilla-beta?
Attachment #8714820 - Flags: approval-mozilla-beta+
Attachment #8714820 - Flags: approval-mozilla-aurora?
Attachment #8714820 - Flags: approval-mozilla-aurora+
Attachment #8713725 - Flags: approval-mozilla-beta?
Attachment #8713725 - Flags: approval-mozilla-beta+
Attachment #8713725 - Flags: approval-mozilla-aurora?
Attachment #8713725 - Flags: approval-mozilla-aurora+
Attachment #8713756 - Flags: approval-mozilla-beta?
Attachment #8713756 - Flags: approval-mozilla-beta+
Attachment #8713756 - Flags: approval-mozilla-aurora?
Attachment #8713756 - Flags: approval-mozilla-aurora+
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 ?
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
Is there still a problem with the beta uplift?
Flags: needinfo?(cbook)
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 ?
Flags: needinfo?(sledru)
Flags: needinfo?(cbook)
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
I think we should land this patch with ba=someone.  Feel free to use my name.  :-)
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.
Attachment #8713725 - Flags: approval-mozilla-release?
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!
Attachment #8713725 - Flags: approval-mozilla-release? → approval-mozilla-release-
Ehsan approved the ba.
Flags: needinfo?(sledru)
You need to log in before you can comment on or make changes to this bug.