FetchEvent.request.cache is not set correctly for non-fetch channels

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
8 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
There is a mapping between our load flags and fetch RequestCache values.  We convert from RequestCache to load flags when setting an explicit RequestCache value.  We don't, however, translate the other way creating a Request from an nsIChannel.  So non-fetch channels never produce any RequestCache values besides "default".

I think we should fix this.
(Assignee)

Comment 1

3 years ago
Created attachment 8739796 [details] [diff] [review]
P1 Set FetchEvent.request.cache value correctly for non-fetch channels. r=mayhemer

This maintains the current behavior for explicitly set fetch cache modes, but will otherwise guess the cache mode from the load flags.
Attachment #8739796 - Flags: review?(honzab.moz)
(Assignee)

Comment 2

3 years ago
I will add a check for this in one of our tests that trigger a window refresh since we should get "no-cache" for those requests.  Should get to that tomorrow.
(Assignee)

Comment 3

3 years ago
Created attachment 8739988 [details] [diff] [review]
bug1263469_p2_test.patch

Add a check for evt.request.cache to our current refresh test.  This causes the test to time out without the P1 and pass with the P1.
Attachment #8739988 - Flags: review?(ehsan)
Whiteboard: btpp-active
(Assignee)

Comment 5

3 years ago
I will add a test for EventSource as well since its required to bypass the http cache.

Comment 6

3 years ago
Comment on attachment 8739988 [details] [diff] [review]
bug1263469_p2_test.patch

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

This is good, but it would be nice if you can add a web platform test for this.  Either in addition or instead of this one.  r=me.
Attachment #8739988 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

3 years ago
We discussed this at the service worker face-to-face and decided we should implement this:

  https://github.com/slightlyoff/ServiceWorker/issues/875
See Also: → bug 1120715
Attachment #8739796 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8741834 [details] [diff] [review]
P3 Test FetchEvent.request.cache value on reload in wpt test. r=ehsan

WPT version of test validating frame reload cache value.  I'll look at writing a wpt EventSource test now.  We don't test EventSource at all there.
Attachment #8741834 - Flags: review?(ehsan)
(Assignee)

Comment 9

3 years ago
Created attachment 8741885 [details] [diff] [review]
P4 Update test_eventsource_intercept.html to validate FetchEvent.request.cache. r=ehsan

Update our EventSource mochitest to check for cache.
Attachment #8741885 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8741834 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

3 years ago
Created attachment 8741920 [details] [diff] [review]
P5 Add a wpt test case for EventSource. r=ehsan

After much pain, this adds a wpt test case for EventSource.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b42011ab8f71
Attachment #8741920 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8741885 - Flags: review?(ehsan) → review+

Updated

3 years ago
Attachment #8741920 - Flags: review?(ehsan) → review+
(Assignee)

Updated

8 months ago
Depends on: 1437080
You need to log in before you can comment on or make changes to this bug.