Closed Bug 1437080 Opened 3 years ago Closed 3 years ago

service workers gets wrong FetchEvent.request.cache value

Categories

(Core :: DOM: Service Workers, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: bkelly, Assigned: bkelly)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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...
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.
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.
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
Duplicate of this bug: 1454400
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 …
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.
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.
> … 145400 … 

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

(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.