The default bug view has changed. See this FAQ.

Implement Fetch RequestCache

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: nsm, Assigned: Ehsan)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete})

33 Branch
mozilla48
x86_64
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(8 attachments)

Comment hidden (empty)
Duplicate of this bug: 1082230
Blocks: 1131322
No longer blocks: 1039846
(Assignee)

Comment 2

2 years ago
Over in bug 1126819, I'm enabling the Request constructor to respect the cache attribute, which is probably part of the fix to this bug.
See Also: → bug 1126819
Note that we currently use the LOAD_BYPASS_CACHE flag to detect a force reload from the UI.  We will need some way to distinguish a fetch(url, { cache: 'no-cache' }) from a user pressing shift-reload in the browser.  In the shift-reload case we need to avoid doing any interception, but we do need to do interception in the fetch() case.

See bug 1157619 for where I added the code to disable intercept for shift-reload.
(In reply to Ben Kelly [:bkelly] from comment #3)
> Note that we currently use the LOAD_BYPASS_CACHE flag to detect a force
> reload from the UI.  We will need some way to distinguish a fetch(url, {
> cache: 'no-cache' }) from a user pressing shift-reload in the browser.  In
> the shift-reload case we need to avoid doing any interception, but we do
> need to do interception in the fetch() case.
> 

Probably best to stick an attribute on nsIHttpChannelInternal like we've done with request mode.
Keywords: dev-doc-needed
Summary: Implement Fetch CacheMode → Implement Fetch RequestCache
Duplicate of this bug: 1216373
Depends on: 1201717
Blocks: 1201717
No longer depends on: 1201717
Blocks: 1173500
No longer blocks: 1131322
(Assignee)

Updated

a year ago
Blocks: 1237782
I'll work on this soonish.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Assignee: bkelly → ehsan
(Assignee)

Comment 7

a year ago
(In reply to Ben Kelly [:bkelly] from comment #3)
> Note that we currently use the LOAD_BYPASS_CACHE flag to detect a force
> reload from the UI.  We will need some way to distinguish a fetch(url, {
> cache: 'no-cache' }) from a user pressing shift-reload in the browser.  In
> the shift-reload case we need to avoid doing any interception, but we do
> need to do interception in the fetch() case.
> 
> See bug 1157619 for where I added the code to disable intercept for
> shift-reload.

This was addressed in bug 1202895.
(Assignee)

Updated

a year ago
Depends on: 1240161
(Assignee)

Comment 8

a year ago
I came up with a plan on how to implement different RequestCache values on top of Necko.  I'd like to validate this plan before starting to write code.  Also there are some differences with what this plan will give us versus what's currently spec'ed. That may be OK (and in the case of "no-cache" and "reload" I think we should probably change the spec to match.  Therefore needinfoing Honza to validate the implementation plan, and Anne on the spec discrepancies.

* default: Don't do anything special.

* no-store: Set LOAD_BYPASS_CACHE and INHIBIT_CACHING.  The spec doesn't differentiate between a permanent and temporary HTTP cache so I think we should use INHIBIT_CACHING over INHIBIT_PERSISTENT_CACHING.  One problem here is that setting INHIBIT_CACHING *and* LOAD_BYPASS_CACHE causes us to not open a cache entry at all which would make it impossible to intercept, so if from a spec perspective INHIBIT_PERSISTENT_CACHING is not enough, I would probably need to make up a new flag.  Honza, I'd appreciate if you can suggest something here!

* force-cache: Set VALIDATE_NEVER.  There are some edge cases that Necko will still try to validate the cached response with this, such as if the response would vary, or if the response is stored with |Cache-Control: no-store|, or if it's ssl and |Cache-Control: no-cache|.  If this is not fine, then we'd need to add a VALIDATE_NEVER_AND_I_MEAN_IT flag!  Anne, I think the exceptions I mentioned here are not acceptable, right?  Honza, is it ok to add a new VALIDATE flag for this?  Any suggestions for a good name?

* reload: Set LOAD_BYPASS_CACHE and LOAD_FROM_CACHE.  This doesn't match the spec in that we'd also set the |Pragma: no-cache| and |Cache-Control: no-cache| headers, which I think is fine for compatibility with HTTP proxies.  If this is not OK, we'd need to add a new load flag to suppress setting these headers in nsHttpChannel::SetupTransaction().  (Perhaps we should fix the spec here?)

* no-cache: Only set LOAD_BYPASS_CACHE, and not LOAD_FROM_CACHE.  This also doesn't match the spec in the same way as "reload" (since that part is a result of setting LOAD_BYPASS_CACHE.)

I'd appreciate feedback!
Flags: needinfo?(honzab.moz)
Flags: needinfo?(annevk)

Comment 9

a year ago
"force-cache" was indeed meant as ignore all consideration, although I could see us not wanting to lose out on Vary. That might lead to getting the wrong result with CORS.

For "reload" maybe adding the headers is fine. The specification is meant to be as low-level as we can give developers, but I cannot imagine anyone not wanting those headers.
Flags: needinfo?(annevk)
(Assignee)

Comment 10

a year ago
(In reply to Anne (:annevk) from comment #9)
> "force-cache" was indeed meant as ignore all consideration, although I could
> see us not wanting to lose out on Vary. That might lead to getting the wrong
> result with CORS.

OK, then I think we need to add a new Necko level flag for this, since VALIDATE_NEVER won't do the job.  Honza?

> For "reload" maybe adding the headers is fine. The specification is meant to
> be as low-level as we can give developers, but I cannot imagine anyone not
> wanting those headers.

OK, filed https://github.com/whatwg/fetch/issues/201.
(In reply to :Ehsan Akhgari from comment #8)
> I came up with a plan on how to implement different RequestCache values on
> top of Necko.  I'd like to validate this plan before starting to write code.
> Also there are some differences with what this plan will give us versus
> what's currently spec'ed. That may be OK (and in the case of "no-cache" and
> "reload" I think we should probably change the spec to match.  Therefore
> needinfoing Honza to validate the implementation plan, and Anne on the spec
> discrepancies.
> 
> * default: Don't do anything special.
> 
> * no-store: Set LOAD_BYPASS_CACHE and INHIBIT_CACHING.  The spec doesn't
> differentiate between a permanent and temporary HTTP cache so I think we
> should use INHIBIT_CACHING over INHIBIT_PERSISTENT_CACHING.  One problem
> here is that setting INHIBIT_CACHING *and* LOAD_BYPASS_CACHE causes us to
> not open a cache entry at all which would make it impossible to intercept,

We should stop implementing interception on top of the cache.  This is not the first time we need to somewhat hack http channel to allow interception.  But I think now when interception is "maybe" we do open.

Difference between INHIBIT_CACHING and INHIBIT_PERSISTENT_CACHING is that the first will not cache at all, while the second will cache in memory (won't persist - aka Cache-control: no-store response header).

> so if from a spec perspective INHIBIT_PERSISTENT_CACHING is not enough, I
> would probably need to make up a new flag.  Honza, I'd appreciate if you can
> suggest something here!

...hack it or don't intercept via the cache.

> 
> * force-cache: Set VALIDATE_NEVER.  There are some edge cases that Necko
> will still try to validate the cached response with this, such as if the
> response would vary, or if the response is stored with |Cache-Control:
> no-store|, or if it's ssl and |Cache-Control: no-cache|.  If this is not
> fine, then we'd need to add a VALIDATE_NEVER_AND_I_MEAN_IT flag!  Anne, I
> think the exceptions I mentioned here are not acceptable, right?  Honza, is
> it ok to add a new VALIDATE flag for this?  Any suggestions for a good name?

Wasn't this recently removed from the spec for security reasons?

You have LOAD_FROM_CACHE.  That will use whatever cached w/o validation and even expired (however not if it would vary), if no cache entry found, go to the server.  If you want to prevent the server reload use LOAD_ONLY_FROM_CACHE.  Then when nothing in the cache that could be use is found, you get DOCUMENT_NOT_CACHED error.

> 
> * reload: Set LOAD_BYPASS_CACHE and LOAD_FROM_CACHE.  

That sounds like a non-sense - those two are mutually exclusive, why both?  Also, do you want to bypass just gecko cache or any transparent cache on the path?  I assume the letter, then you want LOAD_BYPASS_CACHE only.

> This doesn't match the
> spec in that we'd also set the |Pragma: no-cache| and |Cache-Control:
> no-cache| headers, which I think is fine for compatibility with HTTP
> proxies.  

Yes, see above.  If you don't want that, you have LOAD_BYPASS_LOCAL_CACHE.

> If this is not OK, we'd need to add a new load flag to suppress
> setting these headers in nsHttpChannel::SetupTransaction().  (Perhaps we
> should fix the spec here?)

I don't follow why anything like that would be needed. 

> 
> * no-cache: Only set LOAD_BYPASS_CACHE, and not LOAD_FROM_CACHE.  This also
> doesn't match the spec in the same way as "reload" (since that part is a
> result of setting LOAD_BYPASS_CACHE.)

What is no-cache supposed to do exactly?  It's been some time I've read the spec.

> 
> I'd appreciate feedback!
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 12

a year ago
(In reply to Honza Bambas (:mayhemer) from comment #11)
> (In reply to :Ehsan Akhgari from comment #8)
> > I came up with a plan on how to implement different RequestCache values on
> > top of Necko.  I'd like to validate this plan before starting to write code.
> > Also there are some differences with what this plan will give us versus
> > what's currently spec'ed. That may be OK (and in the case of "no-cache" and
> > "reload" I think we should probably change the spec to match.  Therefore
> > needinfoing Honza to validate the implementation plan, and Anne on the spec
> > discrepancies.
> > 
> > * default: Don't do anything special.
> > 
> > * no-store: Set LOAD_BYPASS_CACHE and INHIBIT_CACHING.  The spec doesn't
> > differentiate between a permanent and temporary HTTP cache so I think we
> > should use INHIBIT_CACHING over INHIBIT_PERSISTENT_CACHING.  One problem
> > here is that setting INHIBIT_CACHING *and* LOAD_BYPASS_CACHE causes us to
> > not open a cache entry at all which would make it impossible to intercept,
> 
> We should stop implementing interception on top of the cache.

Yeah, that's the long term goal but for now I prefer to not make that a blocker for this bug.  :-)

> This is not
> the first time we need to somewhat hack http channel to allow interception. 
> But I think now when interception is "maybe" we do open.

Ah, yeah.  /me notes the |!PossiblyIntercepted()| here: <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2871>

> Difference between INHIBIT_CACHING and INHIBIT_PERSISTENT_CACHING is that
> the first will not cache at all, while the second will cache in memory
> (won't persist - aka Cache-control: no-store response header).

So which one do you suggest I should use here?

> > so if from a spec perspective INHIBIT_PERSISTENT_CACHING is not enough, I
> > would probably need to make up a new flag.  Honza, I'd appreciate if you can
> > suggest something here!
> 
> ...hack it or don't intercept via the cache.

Cool!

> > * force-cache: Set VALIDATE_NEVER.  There are some edge cases that Necko
> > will still try to validate the cached response with this, such as if the
> > response would vary, or if the response is stored with |Cache-Control:
> > no-store|, or if it's ssl and |Cache-Control: no-cache|.  If this is not
> > fine, then we'd need to add a VALIDATE_NEVER_AND_I_MEAN_IT flag!  Anne, I
> > think the exceptions I mentioned here are not acceptable, right?  Honza, is
> > it ok to add a new VALIDATE flag for this?  Any suggestions for a good name?
> 
> Wasn't this recently removed from the spec for security reasons?

No, that was "only-if-cached".

> You have LOAD_FROM_CACHE.  That will use whatever cached w/o validation and
> even expired (however not if it would vary), if no cache entry found, go to
> the server.  If you want to prevent the server reload use
> LOAD_ONLY_FROM_CACHE.  Then when nothing in the cache that could be use is
> found, you get DOCUMENT_NOT_CACHED error.

Since force-cache only forces a load through cache, LOAD_FROM_CACHE is probably enough here.

> > * reload: Set LOAD_BYPASS_CACHE and LOAD_FROM_CACHE.  
> 
> That sounds like a non-sense - those two are mutually exclusive, why both? 
> Also, do you want to bypass just gecko cache or any transparent cache on the
> path?  I assume the letter, then you want LOAD_BYPASS_CACHE only.

Hmm, you're right.  I don't know why I was thinking this reading over things again now!  LOAD_BYPASS_CACHE is what I want here.

> > This doesn't match the
> > spec in that we'd also set the |Pragma: no-cache| and |Cache-Control:
> > no-cache| headers, which I think is fine for compatibility with HTTP
> > proxies.  
> 
> Yes, see above.  If you don't want that, you have LOAD_BYPASS_LOCAL_CACHE.

I think we do want that.

> > If this is not OK, we'd need to add a new load flag to suppress
> > setting these headers in nsHttpChannel::SetupTransaction().  (Perhaps we
> > should fix the spec here?)
> 
> I don't follow why anything like that would be needed. 

This is moot now!

> > * no-cache: Only set LOAD_BYPASS_CACHE, and not LOAD_FROM_CACHE.  This also
> > doesn't match the spec in the same way as "reload" (since that part is a
> > result of setting LOAD_BYPASS_CACHE.)
> 
> What is no-cache supposed to do exactly?  It's been some time I've read the
> spec.

no-cache means don't look for a response in the local cache before loading from network, but do set the validation headers if necessary (depending on what's actually in the cache.)  I'm no longer sure that LOAD_BYPASS_CACHE is the same thing.
Flags: needinfo?(honzab.moz)
(In reply to :Ehsan Akhgari from comment #12)
> > We should stop implementing interception on top of the cache.
> 
> Yeah, that's the long term goal but for now I prefer to not make that a
> blocker for this bug.  :-)

Sure, hack it :)

> > Difference between INHIBIT_CACHING and INHIBIT_PERSISTENT_CACHING is that
> > the first will not cache at all, while the second will cache in memory
> > (won't persist - aka Cache-control: no-store response header).
> 
> So which one do you suggest I should use here?

Depends on what "no-store" in your context means.  It resembles Cache-control: no-store response header we treat as "cache but not persistently - aka session only".  But this spec may mean something else.  So, up to you.  But I presume INHIBIT_CACHING is what you want more (total bypass of the HTTP cache for writing).

> > You have LOAD_FROM_CACHE.  That will use whatever cached w/o validation and
> > even expired (however not if it would vary), if no cache entry found, go to
> > the server.  If you want to prevent the server reload use
> > LOAD_ONLY_FROM_CACHE.  Then when nothing in the cache that could be use is
> > found, you get DOCUMENT_NOT_CACHED error.
> 
> Since force-cache only forces a load through cache, LOAD_FROM_CACHE is
> probably enough here.

"force-cache" sound like "only give me what is in cache or bail out", but that was "only-if-cached".  What you want (according above) is more like "prefer stale cache".  I had a similar 'naming' talk with Jason recently on a the same topic ;)  However, depends on what the spec says.  IIRC LOAD_FROM_CACHE is what you want here, right.

I remember suggesting the modes according what we implement (so that it can be achieved with load flags easily - as you do now!) and according what use cases seemed to me useful from experience.

> 
> > > * reload: Set LOAD_BYPASS_CACHE and LOAD_FROM_CACHE.  
> > 
> > That sounds like a non-sense - those two are mutually exclusive, why both? 
> > Also, do you want to bypass just gecko cache or any transparent cache on the
> > path?  I assume the letter, then you want LOAD_BYPASS_CACHE only.
> 
> Hmm, you're right.  I don't know why I was thinking this reading over things
> again now!  LOAD_BYPASS_CACHE is what I want here.

Yep, that's it.

> 
> > > This doesn't match the
> > > spec in that we'd also set the |Pragma: no-cache| and |Cache-Control:
> > > no-cache| headers, which I think is fine for compatibility with HTTP
> > > proxies.  
> > 
> > Yes, see above.  If you don't want that, you have LOAD_BYPASS_LOCAL_CACHE.
> 
> I think we do want that.

Me too.

> > What is no-cache supposed to do exactly?  It's been some time I've read the
> > spec.
> 
> no-cache means don't look for a response in the local cache before loading
> from network, but do set the validation headers if necessary 

Didn't you rather want to say: "always validate what is in the cache"?

Because otherwise what you write doesn't make sense, probably unless you s/don't look for/don't use to satisfy/

> (depending on
> what's actually in the cache.)  I'm no longer sure that LOAD_BYPASS_CACHE is
> the same thing.

Definitely isn't!  VALIDATE_ALWAYS is.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 14

a year ago
(In reply to Honza Bambas (:mayhemer) from comment #13)
> > > Difference between INHIBIT_CACHING and INHIBIT_PERSISTENT_CACHING is that
> > > the first will not cache at all, while the second will cache in memory
> > > (won't persist - aka Cache-control: no-store response header).
> > 
> > So which one do you suggest I should use here?
> 
> Depends on what "no-store" in your context means.  It resembles
> Cache-control: no-store response header we treat as "cache but not
> persistently - aka session only".  But this spec may mean something else. 
> So, up to you.  But I presume INHIBIT_CACHING is what you want more (total
> bypass of the HTTP cache for writing).

Alrighty then.  Based on the spec, it seems like INHIBIT_CACHING is the way to go.

> > > You have LOAD_FROM_CACHE.  That will use whatever cached w/o validation and
> > > even expired (however not if it would vary), if no cache entry found, go to
> > > the server.  If you want to prevent the server reload use
> > > LOAD_ONLY_FROM_CACHE.  Then when nothing in the cache that could be use is
> > > found, you get DOCUMENT_NOT_CACHED error.
> > 
> > Since force-cache only forces a load through cache, LOAD_FROM_CACHE is
> > probably enough here.
> 
> "force-cache" sound like "only give me what is in cache or bail out", but
> that was "only-if-cached".  What you want (according above) is more like
> "prefer stale cache".  

Yep.

> I had a similar 'naming' talk with Jason recently on
> a the same topic ;)  However, depends on what the spec says.  IIRC
> LOAD_FROM_CACHE is what you want here, right.

Great!

> I remember suggesting the modes according what we implement (so that it can
> be achieved with load flags easily - as you do now!) and according what use
> cases seemed to me useful from experience.

Yeah, I'm indeed trying to stick to what we already have as much as I can.

> > > What is no-cache supposed to do exactly?  It's been some time I've read the
> > > spec.
> > 
> > no-cache means don't look for a response in the local cache before loading
> > from network, but do set the validation headers if necessary 
> 
> Didn't you rather want to say: "always validate what is in the cache"?

Yes! :-)  I did a poor job at phrasing it.

> Because otherwise what you write doesn't make sense, probably unless you
> s/don't look for/don't use to satisfy/
> 
> > (depending on
> > what's actually in the cache.)  I'm no longer sure that LOAD_BYPASS_CACHE is
> > the same thing.
> 
> Definitely isn't!  VALIDATE_ALWAYS is.

Perfect!

I should have everything I need now, I think.  Hopefully I'll have patches soon.
Should we make it clear on https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch that this isn't supported yet in Firefox?
(Assignee)

Comment 16

a year ago
(In reply to Marco Castelluccio [:marco] from comment #15)
> Should we make it clear on
> https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch that this
> isn't supported yet in Firefox?

Sure!  Please feel free to do so.
(In reply to :Ehsan Akhgari from comment #16)
> (In reply to Marco Castelluccio [:marco] from comment #15)
> > Should we make it clear on
> > https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch that this
> > isn't supported yet in Firefox?
> 
> Sure!  Please feel free to do so.

I'm not sure what's the best way to do it. Developers might skip reading notes in the compatibility table if they know the Fetch API is supported. Adding an inline note might pollute the page (although there is already an inline note for another option about Chrome).

Is this the only option that Firefox doesn't support?
(Assignee)

Comment 18

a year ago
(In reply to Marco Castelluccio [:marco] from comment #17)
> (In reply to :Ehsan Akhgari from comment #16)
> > (In reply to Marco Castelluccio [:marco] from comment #15)
> > > Should we make it clear on
> > > https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch that this
> > > isn't supported yet in Firefox?
> > 
> > Sure!  Please feel free to do so.
> 
> I'm not sure what's the best way to do it. Developers might skip reading
> notes in the compatibility table if they know the Fetch API is supported.
> Adding an inline note might pollute the page (although there is already an
> inline note for another option about Chrome).

I'm not sure either!

> Is this the only option that Firefox doesn't support?

No, there is referrer and referrerPolicy (bug 1184549 and dependencies) and also integrity and window.
(Assignee)

Comment 19

a year ago
Created attachment 8726879 [details] [diff] [review]
Part 1: Add Necko APIs to preserve the Request cache mode on the channel
Attachment #8726879 - Flags: review?(mcmanus)
(Assignee)

Comment 20

a year ago
Created attachment 8726880 [details] [diff] [review]
Part 2: Don't use OPEN_TRUNCATE when reopening a fake synthesized cache entry

This shows up when intercepting the reload cache type channels, since
the cache storage will reopen the cache entry that has been used to fill
up the synthesized response otherwise because of the LOAD_BYPASS_CACHE
flag.
Attachment #8726880 - Flags: review?(mcmanus)
(Assignee)

Comment 21

a year ago
Created attachment 8726881 [details] [diff] [review]
Part 3: Connect the Necko level cache mode parameter with the Request cache mode variable
Attachment #8726881 - Flags: review?(bkelly)
(Assignee)

Comment 22

a year ago
Created attachment 8726882 [details] [diff] [review]
Part 4: Add tests for Request.cache
Attachment #8726882 - Flags: review?(bkelly)
(Assignee)

Comment 23

a year ago
Created attachment 8726883 [details] [diff] [review]
Part 5: Treat a default cache mode Request with a revalidation header as no-store
Attachment #8726883 - Flags: review?(bkelly)
(Assignee)

Comment 24

a year ago
Created attachment 8726884 [details] [diff] [review]
Part 6: Remove the dom.requestcache.enabled pref
Attachment #8726884 - Flags: review?(bkelly)
Comment on attachment 8726879 [details] [diff] [review]
Part 1: Add Necko APIs to preserve the Request cache mode on the channel

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

general thumbs up on the patches, but honza is best to do this review
Attachment #8726879 - Flags: review?(mcmanus) → review?(honzab.moz)
Comment on attachment 8726880 [details] [diff] [review]
Part 2: Don't use OPEN_TRUNCATE when reopening a fake synthesized cache entry

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

general thumbs up on the patches, but honza is best to do this review
Attachment #8726880 - Flags: review?(mcmanus) → review?(honzab.moz)
Attachment #8726881 - Flags: review?(bkelly) → review+
Comment on attachment 8726882 [details] [diff] [review]
Part 4: Add tests for Request.cache

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

::: testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-event.https.html
@@ +442,5 @@
> +          frame = f;
> +          var tests = cacheTypes.map(function(type) {
> +            return new Promise(function(resolve) {
> +                return frame.contentWindow.fetch(scope + '=' + type,
> +                                                 {cache: type})

Is the unspecified default for fetch() checked anywhere?

::: testing/web-platform/tests/fetch/api/request/request-cache.html
@@ +229,5 @@
> +        var uuid = token();
> +        var identifier = (type == "tag" ? Math.random() : new Date().toGMTString());
> +        var content = Math.random().toString();
> +        var url = make_url(uuid, type, identifier, content, info);
> +        var fetch_promises = [function() {

The fetch_promises name is a bit misleading.  Its an array of functions that return promises.  I think "fetch_functions" or something similar would be easier to understand.
Attachment #8726882 - Flags: review?(bkelly) → review+
Comment on attachment 8726883 [details] [diff] [review]
Part 5: Treat a default cache mode Request with a revalidation header as no-store

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

::: dom/fetch/FetchDriver.cpp
@@ +327,5 @@
>      // Conversion between enumerations is safe due to static asserts in
>      // dom/workers/ServiceWorkerManager.cpp
>      internalChan->SetCorsMode(static_cast<uint32_t>(mRequest->Mode()));
>      internalChan->SetRedirectMode(static_cast<uint32_t>(mRequest->GetRedirectMode()));
> +    mRequest->MaybeSkipCacheIfPerformingRevalidation();

So, this iterates over the headers on the main thread when the fetch() was potentially initiated from a worker thread.  This is safe to do because we make a copy of the request and headers when fetch() is called, right?  I think that's the case, but its a bit scary we don't have asserts for this sort of thing.

Also, I think you can remove this comment on line 245:

  // FIXME(nsm): Bug 1120715.
  // Step 3.4 "If request's cache mode is default and request's header list
  // contains a header named `If-Modified-Since`, `If-None-Match`,
  // `If-Unmodified-Since`, `If-Match`, or `If-Range`, set request's cache mode
  // to no-store."
Attachment #8726883 - Flags: review?(bkelly) → review+
Attachment #8726884 - Flags: review?(bkelly) → review+
(Assignee)

Comment 29

a year ago
(In reply to Ben Kelly [:bkelly] from comment #27)
> :::
> testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-
> event.https.html
> @@ +442,5 @@
> > +          frame = f;
> > +          var tests = cacheTypes.map(function(type) {
> > +            return new Promise(function(resolve) {
> > +                return frame.contentWindow.fetch(scope + '=' + type,
> > +                                                 {cache: type})
> 
> Is the unspecified default for fetch() checked anywhere?

No, good point.  I'll add a test for that.

(In reply to Ben Kelly [:bkelly] from comment #28)
> ::: dom/fetch/FetchDriver.cpp
> @@ +327,5 @@
> >      // Conversion between enumerations is safe due to static asserts in
> >      // dom/workers/ServiceWorkerManager.cpp
> >      internalChan->SetCorsMode(static_cast<uint32_t>(mRequest->Mode()));
> >      internalChan->SetRedirectMode(static_cast<uint32_t>(mRequest->GetRedirectMode()));
> > +    mRequest->MaybeSkipCacheIfPerformingRevalidation();
> 
> So, this iterates over the headers on the main thread when the fetch() was
> potentially initiated from a worker thread.  This is safe to do because we
> make a copy of the request and headers when fetch() is called, right?

Yeah.

> I think that's the case, but its a bit scary we don't have asserts for this
> sort of thing.

Hmm, what kinds of assertions did you have in mind?  We mutate mRequest in a whole bunch of places just like this...

> Also, I think you can remove this comment on line 245:
> 
>   // FIXME(nsm): Bug 1120715.
>   // Step 3.4 "If request's cache mode is default and request's header list
>   // contains a header named `If-Modified-Since`, `If-None-Match`,
>   // `If-Unmodified-Since`, `If-Match`, or `If-Range`, set request's cache
> mode
>   // to no-store."

Yep, will do. Thanks!
(In reply to :Ehsan Akhgari from comment #29)
> > I think that's the case, but its a bit scary we don't have asserts for this
> > sort of thing.
> 
> Hmm, what kinds of assertions did you have in mind?  We mutate mRequest in a
> whole bunch of places just like this...

I'm not sure.  Normally I would use the owning refcount system, but these objects are created on one thread and used on another.

We could create some kind of secondary owning system for asserting.  Default owned to constructing thread and then add a "StealThreadOwnership()" method that the second thread can use.  Then assert all methods are only called by the owning thread.

I don't know, maybe that is too much for such a case.  We could also just add some comments as to what is going on.

> 
> > Also, I think you can remove this comment on line 245:
> > 
> >   // FIXME(nsm): Bug 1120715.
> >   // Step 3.4 "If request's cache mode is default and request's header list
> >   // contains a header named `If-Modified-Since`, `If-None-Match`,
> >   // `If-Unmodified-Since`, `If-Match`, or `If-Range`, set request's cache
> > mode
> >   // to no-store."
> 
> Yep, will do. Thanks!
Comment on attachment 8726879 [details] [diff] [review]
Part 1: Add Necko APIs to preserve the Request cache mode on the channel

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

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2351,5 @@
> +  switch (mCacheMode) {
> +  case nsIHttpChannelInternal::CACHE_MODE_NO_STORE:
> +    // no-store means don't consult the cache on the way to the network, and
> +    // don't store the response in the cache even if it's cacheable.
> +    mLoadFlags |= INHIBIT_CACHING | LOAD_BYPASS_CACHE;

Just a note that LOAD_BYPASS_CACHE will bypass our DNS cache, our HTTP cache, and any transparent intermediate caches between UA and the origin server.  If you only want to bypass our (local) caches, you want LOAD_BYPASS_LOCAL_CACHE.

@@ +2356,5 @@
> +    break;
> +  case nsIHttpChannelInternal::CACHE_MODE_RELOAD:
> +    // reload means don't consult the cache on the way to the network, but
> +    // do store the response in the cache if possible.
> +    mLoadFlags |= LOAD_BYPASS_CACHE;

This flag makes also transparent caches revalidate - which is correct here!  but also refreshes our DNS cache...

@@ +2360,5 @@
> +    mLoadFlags |= LOAD_BYPASS_CACHE;
> +    break;
> +  case nsIHttpChannelInternal::CACHE_MODE_NO_CACHE:
> +    // no-cache means always validate what's in the cache.
> +    mLoadFlags |= VALIDATE_ALWAYS;

Note that VALIDATE_ALWAYS has similar effect as LOAD_BYPASS_CACHE regarding our DNS cache.

I will consult the performance impact with necko folks during our next meeting.

The better way to implement would IMO be to set Pragma: no-cache and Cache-control: no-cache REQUEST headers.  However, that is something necko doesn't respect right now.  There is a long standing open bug for it.  Bug 428916.

For now I think you can go with this flag tho.

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +492,5 @@
>  
>    bool mCorsIncludeCredentials;
>    uint32_t mCorsMode;
>    uint32_t mRedirectMode;
> +  uint32_t mCacheMode;

mFetchCacheMode please.

::: netwerk/protocol/http/nsIHttpChannelInternal.idl
@@ +251,5 @@
> +     * Set to indicate Request.cache mode, which simulates the fetch API
> +     * semantics, and is also used for exposing this value to the Web page
> +     * during service worker interception.
> +     */
> +    attribute unsigned long cacheMode;

fatchCacheMode please.  "cacheMode" is too general while this is an exact mirror of the fetch spec and should only be used for achieving the required fetch behavior.
Attachment #8726879 - Flags: review?(honzab.moz) → review+
Comment on attachment 8726880 [details] [diff] [review]
Part 2: Don't use OPEN_TRUNCATE when reopening a fake synthesized cache entry

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

Makes sense for me but jdm should say the final word here.
Attachment #8726880 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

a year ago
Attachment #8726880 - Flags: review?(josh)
(Assignee)

Comment 33

a year ago
(In reply to Honza Bambas (:mayhemer) from comment #31)
> Comment on attachment 8726879 [details] [diff] [review]
> Part 1: Add Necko APIs to preserve the Request cache mode on the channel
> 
> Review of attachment 8726879 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> @@ +2351,5 @@
> > +  switch (mCacheMode) {
> > +  case nsIHttpChannelInternal::CACHE_MODE_NO_STORE:
> > +    // no-store means don't consult the cache on the way to the network, and
> > +    // don't store the response in the cache even if it's cacheable.
> > +    mLoadFlags |= INHIBIT_CACHING | LOAD_BYPASS_CACHE;
> 
> Just a note that LOAD_BYPASS_CACHE will bypass our DNS cache, our HTTP
> cache, and any transparent intermediate caches between UA and the origin
> server.  If you only want to bypass our (local) caches, you want
> LOAD_BYPASS_LOCAL_CACHE.

We definitely want to bypass transparent intermediate caches.  It's unfortunate that this passes the DNS cache too, but I don't think that's a huge deal.

> @@ +2356,5 @@
> > +    break;
> > +  case nsIHttpChannelInternal::CACHE_MODE_RELOAD:
> > +    // reload means don't consult the cache on the way to the network, but
> > +    // do store the response in the cache if possible.
> > +    mLoadFlags |= LOAD_BYPASS_CACHE;
> 
> This flag makes also transparent caches revalidate - which is correct here! 
> but also refreshes our DNS cache...

Again, the DNS cache bit is unfortunate.  In the future if we decide that is an issue, perhaps we can add a separate flag for that.

> @@ +2360,5 @@
> > +    mLoadFlags |= LOAD_BYPASS_CACHE;
> > +    break;
> > +  case nsIHttpChannelInternal::CACHE_MODE_NO_CACHE:
> > +    // no-cache means always validate what's in the cache.
> > +    mLoadFlags |= VALIDATE_ALWAYS;
> 
> Note that VALIDATE_ALWAYS has similar effect as LOAD_BYPASS_CACHE regarding
> our DNS cache.

Same above.

> I will consult the performance impact with necko folks during our next
> meeting.
> 
> The better way to implement would IMO be to set Pragma: no-cache and
> Cache-control: no-cache REQUEST headers.  However, that is something necko
> doesn't respect right now.  There is a long standing open bug for it.  Bug
> 428916.
> 
> For now I think you can go with this flag tho.

Great, thanks!

> ::: netwerk/protocol/http/HttpBaseChannel.h
> @@ +492,5 @@
> >  
> >    bool mCorsIncludeCredentials;
> >    uint32_t mCorsMode;
> >    uint32_t mRedirectMode;
> > +  uint32_t mCacheMode;
> 
> mFetchCacheMode please.
> 
> ::: netwerk/protocol/http/nsIHttpChannelInternal.idl
> @@ +251,5 @@
> > +     * Set to indicate Request.cache mode, which simulates the fetch API
> > +     * semantics, and is also used for exposing this value to the Web page
> > +     * during service worker interception.
> > +     */
> > +    attribute unsigned long cacheMode;
> 
> fatchCacheMode please.  "cacheMode" is too general while this is an exact
> mirror of the fetch spec and should only be used for achieving the required
> fetch behavior.

Will do.

Updated

a year ago
Attachment #8726880 - Flags: review?(josh) → review+

Comment 34

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef66b64e9f8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e003fb64d41
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5bf9f7bb86e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9391f22c08c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f87a25ade31e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee4b18f6b3a
This seems to have broken a wpt test: https://treeherder.mozilla.org/logviewer.html#?job_id=23393703&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3a0551e7b1
Flags: needinfo?(ehsan)
(Assignee)

Updated

a year ago
Depends on: 1255415
(Assignee)

Comment 36

a year ago
Hmm, this is an intermittent failure that I can reproduce some of the times on OSX opt (but so far not under Linux with rr.)

Looking at the HTTP log, in the case where this failure happens, we fall into this code path: <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2651> which causes ProcessNotModified() to fail because mDidReval is false.  I suspect the intermittent nature of this failure is caused by the if branch here being taken sometimes <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3433> which would happen if mConcurentCacheAccess is true, causing mDidReval to never become true...  Why is any of this happening is beyond me for now.
(Assignee)

Comment 37

a year ago
It seems like the difference between the failing and non-failing case is whether OnCacheEntryCheck() is called.  In the case where the test passes, OnCacheEntryCheck() gets called and it sets mDidReval.  In the case where the test fails, OnCacheEntryCheck() is not called on the channel at all...
(Assignee)

Comment 38

a year ago
I finally found out what's going on here!

First things first, the reason for the test "failing" intermittently was my usage of |new Date()| in the test.  That caused the date query string argument and the If-Modified-Since headers to sometimes have the same value, causing the server to send back a 304, and sometimes different values, causing the server to send back a 200.

Now, the test was actually intermittently passing!  In case where the 304 response is actually received by the server, Necko will always refuse to process it (as I said in comment 36) because mDidReval is false.  This is actually fine, since in this case, it's the web page that's triggering the revalidation manually, so there is no reason why Necko would return anything other than the 304.  Indeed, there's nothing in the spec which would require that either.  So the test should expect 304 in these cases.

Fixes coming up.
Flags: needinfo?(ehsan)
(Assignee)

Comment 39

a year ago
Created attachment 8729198 [details] [diff] [review]
Part 7: Always use the same date representing 'now' in the RequestCache test
Attachment #8729198 - Flags: review?(bkelly)
(Assignee)

Comment 40

a year ago
Created attachment 8729199 [details] [diff] [review]
Part 8: Expect a 304 response in the cases where we would do a successful revalidation in the RequestCache test
Attachment #8729199 - Flags: review?(bkelly)
Attachment #8729198 - Flags: review?(bkelly) → review+
Attachment #8729199 - Flags: review?(bkelly) → review+

Comment 41

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d6045125e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f346712a3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/c731944ecc64
https://hg.mozilla.org/integration/mozilla-inbound/rev/c74866f52320
https://hg.mozilla.org/integration/mozilla-inbound/rev/79158f028ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2871dbeb7cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10c375353f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/043770204431
Backed out for Android dom/u2f/tests/test_no_token.html permafail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/60633fe1415f

https://treeherder.mozilla.org/logviewer.html#?job_id=23486449&repo=mozilla-inbound
TEST-UNEXPECTED-FAIL | dom/u2f/tests/test_no_token.html | uncaught exception - ReferenceError: u2f is not defined at http://mochi.test:8888/tests/dom/u2f/tests/test_no_token.html:29
Turns out this was due to Android mochitest chunking issues. See bug 1255784. Sorry for the hassle :(
Depends on: 1255784
(Assignee)

Comment 44

a year ago
Ugh....  Should I reland this?
Flags: needinfo?(ryanvm)
(Assignee)

Comment 45

a year ago
Oh hmm I guess those tests need fixing first.
Flags: needinfo?(ryanvm)
Or disabling, yeah. In the past, we've been pretty intolerant of tests that break due to chunking boundary issues (so as to not hold innocent parties hostage to poorly-written tests). That said, it looks like a fix is imminent, so maybe we can just wait a bit.

Comment 47

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a89a3c36893
https://hg.mozilla.org/integration/mozilla-inbound/rev/53bafeddc993
https://hg.mozilla.org/integration/mozilla-inbound/rev/1308f7beeaa1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58317eb7f23
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a97c346298f
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cce63231d48
https://hg.mozilla.org/integration/mozilla-inbound/rev/2134819f1c61
https://hg.mozilla.org/integration/mozilla-inbound/rev/53638cfd818d

Comment 48

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6a89a3c36893
https://hg.mozilla.org/mozilla-central/rev/53bafeddc993
https://hg.mozilla.org/mozilla-central/rev/1308f7beeaa1
https://hg.mozilla.org/mozilla-central/rev/f58317eb7f23
https://hg.mozilla.org/mozilla-central/rev/1a97c346298f
https://hg.mozilla.org/mozilla-central/rev/5cce63231d48
https://hg.mozilla.org/mozilla-central/rev/2134819f1c61
https://hg.mozilla.org/mozilla-central/rev/53638cfd818d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I had a look at the documentation, and it turns out we had the cache property already documented. I've just updated a few details (e.g. only-if-cached no longer in spec, compat info wrong), but I think this looks ok now:

https://developer.mozilla.org/en-US/docs/Web/API/Request/Request
https://developer.mozilla.org/en-US/docs/Web/API/Request/cache

Is this ok? 

I was a bit confused, as I'm sure we've had the cache property for a while. Were its values defined somewhere different before this was implemented?
Keywords: dev-doc-needed → dev-doc-complete
See Also: → bug 1263469
Blocks: 1272436
You need to log in before you can comment on or make changes to this bug.