1437080 - service workers gets wrong FetchEvent.request.cache value

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Correctly check cache related bit flags on HttpBaseChannel::mLoadFlags. r=valentin

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Remove the HttpBaseChannel::mFetchCacheMode override to avoid possibility it can get out of sync with load flags. r=valentin

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

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!

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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

[Valentin Gosu [:valentin] (he/him)]

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?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make SetFetchCacheMode() to clear other cache flags and assert the final value. r=valentin

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

Comment 12

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  Try is green.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3e216f67b25f
https://hg.mozilla.org/mozilla-central/rev/4a3044a631d7
https://hg.mozilla.org/mozilla-central/rev/784e9f86bb03

Comment 17

[Graham Perrin]

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 …

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

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

[Graham Perrin]

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

[Graham Perrin]

> … 145400 … 

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

Comment 21

[wromijn]

(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