Closed
Bug 1437080
Opened 7 years ago
Closed 7 years ago
service workers gets wrong FetchEvent.request.cache value
Categories
(Core :: DOM: Service Workers, enhancement, P2)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: bkelly, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
2.30 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
11.37 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
3.54 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
(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 | ||
Comment 2•7 years ago
|
||
(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 | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 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
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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+
Assignee | ||
Comment 10•7 years ago
|
||
(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.
Assignee | ||
Comment 11•7 years ago
|
||
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+
Assignee | ||
Comment 13•7 years ago
|
||
Thanks. Try is green.
Comment 14•7 years 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•7 years 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
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 17•6 years 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•6 years 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•6 years 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•6 years ago
|
||
> … 145400 …
My bad, a typo. The duplicate was 1454400. Apologies for the noise.
Comment 21•6 years 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.
Description
•