service workers gets wrong FetchEvent.request.cache value

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
19 days ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(3 attachments)

When a service worker intercepts a network request it must convert the nsIChannel into a Request object.  This is exposed as FetchEvent.request.  One of the properties on this object is Request.cache.  This reflects how the request should interact with the http cache.

Currently we are not setting the Request.cache value correctly in all cases.  The problem is that HttpBaseChannel::GetFetchCacheMode() is not performing bitwise operations correctly.  For example:

  } else if (mLoadFlags & (LOAD_FROM_CACHE | nsICachingChannel::LOAD_ONLY_FROM_CACHE)) {
    *aFetchCacheMode = nsIHttpChannelInternal::FETCH_CACHE_MODE_ONLY_IF_CACHED;
  } else if (mLoadFlags & LOAD_FROM_CACHE) {
    *aFetchCacheMode = nsIHttpChannelInternal::FETCH_CACHE_MODE_FORCE_CACHE;
  }

If the mLoadFlags as LOAD_FROM_CACHE, this will actually match the first check which was meant to be more restrictive.  These if-statements are only looking for the existence of any of the flags, not the existence of all of the flags.

I believe this probably means we have been misreporting values in this way:

* "reload" was has been getting reported as "no-store"
* "force-cache" has been getting reported as "only-if-cache"

I'll fix this and add a test case that performs a fetch() with every type of cache setting and verifies the FetchEvent sees the same value.
(In reply to Ben Kelly [:bkelly] from comment #0)
> I'll fix this and add a test case that performs a fetch() with every type of
> cache setting and verifies the FetchEvent sees the same value.

Actually, this test exists and we were passing it.  How...
(Assignee)

Updated

a year ago
Blocks: 1263469
(In reply to Ben Kelly [:bkelly] from comment #1)
> (In reply to Ben Kelly [:bkelly] from comment #0)
> > I'll fix this and add a test case that performs a fetch() with every type of
> > cache setting and verifies the FetchEvent sees the same value.
> 
> Actually, this test exists and we were passing it.  How...

We're passing the test because we keep a duplicate cache mode enum on the channel when explicitly set for fetch().  I think we should just remove this now as clearly it can get out of sync.
(Assignee)

Updated

a year ago
Depends on: 1437094
Comment on attachment 8949807 [details] [diff] [review]
P1 Correctly check cache related bit flags on HttpBaseChannel::mLoadFlags. r=valentin

Valentin, this patch corrects some bogus bitwise operations.  The GetFetchCacheMode() method wants to check if *all* the related flags are set on mLoadFlags.  The existing logic, though, only checks if at least one of the flags is set.  To fix this the patch essentially changes logic from:

  (mLoadFlags & mask) != 0

To:

  (mLoadFlags & mask) != mask
Attachment #8949807 - Flags: review?(valentin.gosu)
Comment on attachment 8949808 [details] [diff] [review]
P2 Remove the HttpBaseChannel::mFetchCacheMode override to avoid possibility it can get out of sync with load flags. r=valentin

The reason we never noticed the error in the GetFetchCacheMode() is that we also have an explicit enum override; mFetchCacheMode.  This is set by fetch() and its preferred over reading the mLoadFlags.  The majority of our tests our written using fetch() because its the easiest way to control the cache mode from script.  As a result the tests mainly exercised the explicit enum case and not the load flags case.

Since mFetchCacheMode has been out-of-sync with mLoadFlags, lets just get rid of it now.  We should only use mLoadFlags so we're sure we're always testing against the correct value being used by necko.
Attachment #8949808 - Flags: review?(valentin.gosu)
Comment on attachment 8949807 [details] [diff] [review]
P1 Correctly check cache related bit flags on HttpBaseChannel::mLoadFlags. r=valentin

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

Great catch!
Attachment #8949807 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #7)
> Great catch!

Thanks, although I wish I had caught it when I first wrote this code 2 years ago. :-(
Comment on attachment 8949808 [details] [diff] [review]
P2 Remove the HttpBaseChannel::mFetchCacheMode override to avoid possibility it can get out of sync with load flags. r=valentin

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

This seems a sensible change.
I assume it isn't a strict requirement that nsIHttpChannelInternal::GetFetchCacheMode returns the same as SetFetchCacheMode, right?
Attachment #8949808 - Flags: review?(valentin.gosu) → review+
(In reply to Valentin Gosu [:valentin] from comment #9)
> This seems a sensible change.
> I assume it isn't a strict requirement that
> nsIHttpChannelInternal::GetFetchCacheMode returns the same as
> SetFetchCacheMode, right?

Well, it *should* return the same thing unless some other code changes mLoadFlags.

I guess we should make it more explicit about clearing bit flags for caching things that aren't needed any more.  SetFetchCacheMode probably only handles going from "no cache bit flags" to "some cache bit flags".  We don't clear flags that shouldn't be set any more.  I can do another patch to try to fix this.
Valentin, this patch makes sure to clear any existing cache related flags before adding the new flags in SetFetchCacheMode().  It also adds an assert that GetFetchCacheMode() agrees with the current state.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c37a1ce382b9887e49149bf1b1ee596547f1146
Attachment #8949843 - Flags: review?(valentin.gosu)
Comment on attachment 8949843 [details] [diff] [review]
P3 Make SetFetchCacheMode() to clear other cache flags and assert the final value. r=valentin

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

r=me if we have a green try run.
Attachment #8949843 - Flags: review?(valentin.gosu) → review+
Thanks.  Try is green.

Comment 14

a year ago
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e216f67b25f
P1 Correctly check cache related bit flags on HttpBaseChannel::mLoadFlags. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a3044a631d7
P2 Remove the HttpBaseChannel::mFetchCacheMode override to avoid possibility it can get out of sync with load flags. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/784e9f86bb03
P3 Make SetFetchCacheMode() to clear other cache flags and assert the final value. r=valentin

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3e216f67b25f
https://hg.mozilla.org/mozilla-central/rev/4a3044a631d7
https://hg.mozilla.org/mozilla-central/rev/784e9f86bb03
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Updated

a year ago
Duplicate of this bug: 1454400

Comment 17

11 months ago
Please: 

- was the fix, here, the fix for symptoms such as 'Cannot load app' at instances of Discourse? 

<https://meta.discourse.org/t/-/78008/24?u=grahamperrin> drew attention to the difference between Firefox versions 59 and 60. 

<https://github.com/MrAlex94/Waterfox/issues/581> is for: 

> 'Corrupted Content Error' or 'Cannot load app' after unloading …
(Assignee)

Comment 18

11 months ago
I'm not sure.  This bug was more about issues with view-source.  This is the first time I am seeing the discourse problem report, so I don't know if this bug or another bug addressed it.

Comment 19

11 months ago
Thanks. 

The screenshot for duplicate bug 145400 has expired, still there's enough for me to assume that it's related to what's seen in Waterfox. I'll go ahead make this fixed bug 1437080 a point of reference for the issue in the Waterfox area.

Comment 20

11 months ago
> … 145400 … 

My bad, a typo. The duplicate was 1454400. Apologies for the noise.

Comment 21

19 days ago

(In reply to Ben Kelly [:bkelly, not reviewing] from comment #18)

I'm not sure. This bug was more about issues with view-source. This is the
first time I am seeing the discourse problem report, so I don't know if this
bug or another bug addressed it.

fwiw, I'm still experiencing the discourse issue on Ubuntu 18.10 with Firefox 66.0

You need to log in before you can comment on or make changes to this bug.