1120715 - Implement Fetch RequestCache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 2

[(no longer active)]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I'll work on this soonish.

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

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!

Comment 9

[Anne (:annevk)]

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

Comment 10

[(no longer active)]

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

Comment 11

[Honza Bambas (:mayhemer)]

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

Comment 12

[(no longer active)]

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

Comment 13

[Honza Bambas (:mayhemer)]

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

Comment 14

[(no longer active)]

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

Comment 15

[Marco Castelluccio [:marco]]

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?

Comment 16

[(no longer active)]

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

Comment 17

[Marco Castelluccio [:marco]]

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

Comment 18

[(no longer active)]

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

Comment 19

[(no longer active)]

Attachment: Part 1: Add Necko APIs to preserve the Request cache mode on the channel

Comment 20

[(no longer active)]

Attachment: 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.

Comment 21

[(no longer active)]

Attachment: Part 3: Connect the Necko level cache mode parameter with the Request cache mode variable

Comment 22

[(no longer active)]

Attachment: Part 4: Add tests for Request.cache

Comment 23

[(no longer active)]

Attachment: Part 5: Treat a default cache mode Request with a revalidation header as no-store

Comment 24

[(no longer active)]

Attachment: Part 6: Remove the dom.requestcache.enabled pref

Comment 25

[Patrick McManus [:mcmanus]]

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

Comment 26

[Patrick McManus [:mcmanus]]

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

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 29

[(no longer active)]

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

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

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

[Honza Bambas (:mayhemer)]

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.

Comment 32

[Honza Bambas (:mayhemer)]

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.

Comment 33

[(no longer active)]

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

Comment 34

[Pulsebot]

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

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 36

[(no longer active)]

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.

Comment 37

[(no longer active)]

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

Comment 38

[(no longer active)]

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.

Comment 39

[(no longer active)]

Attachment: Part 7: Always use the same date representing 'now' in the RequestCache test

Comment 40

[(no longer active)]

Attachment: Part 8: Expect a 304 response in the cases where we would do a successful revalidation in the RequestCache test

Comment 41

[Pulsebot]

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Turns out this was due to Android mochitest chunking issues. See bug 1255784. Sorry for the hassle :(

Comment 44

[(no longer active)]

Ugh....  Should I reland this?

Comment 45

[(no longer active)]

Oh hmm I guess those tests need fixing first.

Comment 46

[Ryan VanderMeulen [: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

[Pulsebot]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 49

[Chris Mills [:cmills]]

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?