Dan Callahan [:callahad] Reporter
	Description • 8 years ago

STR:

1. Open DevTools, Enable "Disable Cache (when toolbox is open)" under advanced settings.
2. Switch to the Network tool
3. Visit https://www.pokedex.org/
4. Refresh the page

What should happen:

- The requests hit the network, since caches are disabled

What actually happens:

- All requests are served from the service worker's cache

You can also verify this by visiting an inert page, https://www.pokedex.org/manifest.json, and running the following in the console:

    caches.match('/img/icon-48.png').then(x => console.log(`Resource ${(x && x.ok) ? "WAS" : "WAS NOT"} found in cache:`, x))

This demonstrates that the `caches` object exists and is fully functional, even when caches are disabled in the Dev Tools. This makes it much more difficult to debug Service Worker behavior re: cache misses or an empty cache.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 1 • 8 years ago

Sounds similar to this existing web worker version, bug 1047663.

	Dan Callahan [:callahad] Reporter
	Comment 2 • 8 years ago

AFAICT, the type of worker is the only difference I see between this bug (Service Workers) and Bug 1047663 (Web Workers).

It might be worth leaving this one open, programmatic caches are an explicit feature of Service Workers.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 3 • 8 years ago

Yep, let's keep it open so we can retest after bug 1047663.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 4 • 8 years ago

Seems pretty critical for our recent push around SW.

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

What is the bug even asking for?  "SW caches" are a manual API that script uses.  How are you going to disable that?

Or are you asking to bypass the service worker when this flag is set?  I've been working hard to make sure service workers continue to work in these cases.  In my experience helping developers with problems in the wild they seem to expect service worker fetch events to still fire even with "disable caches" selected.

Even if you start bypassing service workers, the Cache API is still accessible from the main window.  Its basically just another store like IDB.

This option should probably just clarify its talking about the http cache, if anything.

IMO this bug should be marked INVALID.

	Ben Kelly [:bkelly, not reviewing]
	Comment 6 • 8 years ago

Also consider the shift-ctrl-r bypasses service worker interception per the spec, but does not disable caches.match() in any way.

	Ben Kelly [:bkelly, not reviewing]
	Comment 7 • 8 years ago

I feel very strongly that this should be WONTFIX.

	Dan Callahan [:callahad] Reporter
	Comment 8 • 8 years ago

I'm confused when I see the following:

    [✓] Disable Cache

Taking the label at its word, it appears to be broken: there's still a significant cache that is enabled and working.

We can resolve this two ways:

1. Meet the user's expectations. Naively, I'd expect ServiceWorkers to continue working, but either the cache object would not be available, or it would always miss.

2. Reword the label to be more specific. If we explicitly said something akin to "[✓] Disable Built-in Browser Cache," we could set the correct expectations for users.

Either would be acceptable, but I'd favor #1: from a developer's perspective, I'm looking for the ability to quickly simulate what loading my site is like on a completely fresh profile with an empty cache. If we go with #2, I still don't have an easy way to accomplish the task I'm *actually* trying to do, but at least I won't think Firefox is broken.

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

(In reply to Dan Callahan [:callahad] from comment #8)
> I'm confused when I see the following:
> 
>     [✓] Disable Cache
> 
> Taking the label at its word, it appears to be broken: there's still a
> significant cache that is enabled and working.

Cache API is more like IDB than http cache.  At one point it was even called FetchStore, not Cache.  See bug 940273 comment 10.  Please don't get hung up on the name.

> 1. Meet the user's expectations. Naively, I'd expect ServiceWorkers to
> continue working, but either the cache object would not be available, or it
> would always miss.

I can tell you this is absolutely not what service worker devs expect or want.  Because I have fixed bugs written by devs asking why service workers broke when they used an addon to "disable http cache".

> 2. Reword the label to be more specific. If we explicitly said something
> akin to "[✓] Disable Built-in Browser Cache," we could set the correct
> expectations for users.

This is really talking about the http cache.  It probably makes sense to be specific there.

> Either would be acceptable, but I'd favor #1: from a developer's
> perspective, I'm looking for the ability to quickly simulate what loading my
> site is like on a completely fresh profile with an empty cache. If we go
> with #2, I still don't have an easy way to accomplish the task I'm
> *actually* trying to do, but at least I won't think Firefox is broken.

Ok, how are you clearing cookies, IDB, local storage in this case?  Does "disable cache" reset that state as well?  I don't think it does.

What you really want is the "open this page in a fresh profile".  People on chrome use their incognito for that.  Our private browsing is not quite equivalent since we block storage APIs in PB mode.  The new multiple container code is probably our best bet for implementing something like this.  You get a new principal for each container isolating all storage, etc.  You could then add a "load page in new container" button.

But I feel very strongly disabling the http cache should not break random other storage APIs.

I would also consider a "disable service worker interception while devtools is open" option.  That seems like it might also meet what you are asking for.

	Dan Callahan [:callahad] Reporter
	Comment 10 • 8 years ago

Rewording to "Disable HTTP Cache" sounds like a great solution to me.

	Brian Grinstead [:bgrins]
	Comment 11 • 8 years ago

(In reply to Dan Callahan [:callahad] from comment #10)
> Rewording to "Disable HTTP Cache" sounds like a great solution to me.

Alright, renaming bug to match

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 12 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/51105/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51105/

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 13 • 8 years ago

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

	Brian Grinstead [:bgrins]
	Comment 14 • 8 years ago

Comment on attachment 8749707 [details]
MozReview Request: Bug 1253018 - Rename cache option to mention HTTP. r=bgrins

https://reviewboard.mozilla.org/r/51105/#review47783

Works for me, thanks

	Pulsebot
	Comment 15 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/794c7ee92f48

	Carsten Book [:Tomcat]
	Comment 16 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/794c7ee92f48

	Chris Mills [:cmills]
	Comment 17 • 8 years ago

I've documented the naming change here:

https://developer.mozilla.org/en-US/docs/Tools/Settings#Advanced_settings

And also added a note to the Fx 49 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools 

Let me know if that looks ok. Thanks!