Custom Cache-Control directives in the request are not honored, for example when setting Cache-Control on a XMLHttpRequest request

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: mogwaai, Assigned: mayhemer)

Tracking

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 6 obsolete attachments)

Reporter

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

When a XMLHttpRequest is made using either Firefox 3.0b4 or Firefox 2.latest it behaves incorrectly with respect to caches.

It is my expectation that...
1. A XMLHttpRequest made without adjusting the cache-control request headers should be satisfied by the cache if there is a FRESH version of that content available. This works as expected.
2. A XMLHttpRequest made with a HTTP request header added of "Cache-Control: max-age=0" should ignore any locally cached version and rerequest this content from the network. This does not work.
3. For any XMLHttpRequest a response should be cached according the caching directives provided with the response. This does not work.

Here is a quote from the W3C Specification. http://www.w3.org/TR/XMLHttpRequest/. The caching specification from RFC2616 is also relevant http://www.w3.org/Protocols/rfc2616/rfc2616.html

"If the user agent implements a HTTP cache it should respect Cache-Control request headers set by the script (e.g., Cache-Control: no-cache bypasses the cache). It must not send Cache-Control or Pragma request headers automatically unless the user explicitly requests such behavior (e.g., by (force-)reloading the page). 304 Not Modified responses that are a result of a user agent generated conditional request must be presented as 200 OK responses with the appropriate content. The user agent must allow scripts to override automatic cache validation by setting request headers (e.g., If-None-Match, If-Modified-Since), in which case 304 Not Modified responses must be passed through. RFC2616" 

Reproducible: Always

Steps to Reproduce:
1. Create some javascript using XMLHttpRequest using the Cache-Control parameters specified in the Details section.



Expected Results:  
1. A XMLHttpRequest made without adjusting the cache-control request headers should be satisfied by the cache if there is a FRESH version of that content available. This works as expected.
2. A XMLHttpRequest made with a HTTP request header added of "Cache-Control: max-age=0" should ignore any locally cached version and rerequest this content from the network.
3. For any XMLHttpRequest a response should be cached according the caching directives provided with the response. 


W3C Specifications

 XMLHttpRequest  - http://www.w3.org/TR/XMLHttpRequest/
 General Caching - http://www.w3.org/Protocols/rfc2616/rfc2616.html
Reporter

Comment 1

11 years ago
This patch is a sample implementation to fix the specified bug.

If the Cache-Control request header contains no-cache or max-age=0 it will ignore the local cache when satisfying the request. However, the response is still cacheable based on the normal rules (cache-control, last-modified, etc) in the response.
Reporter

Updated

11 years ago
Attachment #315506 - Flags: review?(dmose)

Updated

11 years ago
Component: General → XML
Product: Firefox → Core
QA Contact: general → xml
Summary: XMLHttpRequest → XMLHttpRequest behaves incorrectly with respect to caches
Version: unspecified → Trunk
Comment on attachment 315506 [details] [diff] [review]
Fix to support RFC handing of max-age and no-cache in request

I'm not a reviewer for the networking module; pointing this review request at someone who is...
Attachment #315506 - Flags: review?(dmose) → review?(cbiesinger)
Reporter

Updated

11 years ago
Attachment #315506 - Flags: review?(cbiesinger) → review?(bzbarsky)
Comment on attachment 315506 [details] [diff] [review]
Fix to support RFC handing of max-age and no-cache in request

Christian, or maybe even Darin, is a better reviewer for this.  I'm not up on my HTTP caching stuff, and won't be in the next few months.
Attachment #315506 - Flags: review?(bzbarsky) → review?(cbiesinger)

Updated

9 years ago
Blocks: 443098
Comment on attachment 315506 [details] [diff] [review]
Fix to support RFC handing of max-age and no-cache in request

I'm going to assume this is no longer relevant, please re-request if that's wrong. Also at first glance this does not look correct.
Attachment #315506 - Flags: review?(cbiesinger)
Component: XML → Networking: HTTP
QA Contact: xml → networking.http
Summary: XMLHttpRequest behaves incorrectly with respect to caches → Custom Cache-Control directives in the request are not honored, for example when setting Cache-Control on a XMLHttpRequest request
Bug 706806 has a testcase for no-store
Blocks: 202896
Depends on: 706806

Comment 6

7 years ago
This bug almost made me crazy until I found it was a Firefox problem an not my cache header..

This tools can help to test.

http://www.mnot.net/javascript/xmlhttprequest/cache.html

Comment 7

6 years ago
The bug is still not fixed, not even on Firefox 29 Nightly.
On Chrome setRequestHeader('Cache-Control', 'no-cache') would avoid the cache and retry new XMLHttpRequest.

Comment 8

6 years ago
setRequestHeader('If-Modified-Since', 'Sat, 1 Jan 2000 00:00:00 GMT'); would solve the cache issue, but it still does not avoid this bug relevance. And the same result expected also with setRequestHeader('Cache-Control', 'no-cache').

Comment 9

5 years ago
The problem is - nothing supported from Cache-Control sent by the user agent.
For example Cache-Control: "no-cache", Cache-Control: "max-age=0", Cache-Control: "max-age=0, must-revalidate" must avoid cache, but does not on Firefox.

Cache-Control explained in http://stackoverflow.com/questions/1046966/whats-the-difference-between-cache-control-max-age-0-and-no-cache
+1 to get this bug fixed :-/

I can't help notice that Mozilla is advocating an alternative solution for bypassing the cache with an XMLHttpRequest : https://developer.mozilla.org/en/docs/Web/API/XMLHttpRequest/Using_XMLHttpRequest#Bypassing_the_cache
(in a nutshell: the solution consists in appending '?<timestamp>' to the URL).

Let me emphasis how wrong this alternative "solution" is:
* it makes the assumption that the server will silently ignore this spurious parameter (ok, most of them will, but that is a risky assumption)
* the response of the server will not be cached under the "correct" URL, so by bypassing the cache once, you are basically forced to bypass it all the time
* worse: you are polluting your cache with responses for single-use URLs

So pretty pretty please, Mozilla: comply with the specification and handle cache-control correctly with XHR. Thanks in advance :-)
Whiteboard: [necko-backlog]
Duplicate of this bug: 443098
Duplicate of this bug: 1129500
Assignee: nobody → mcmanus
Whiteboard: [necko-backlog] → [necko-active]
Duplicate of this bug: 706806

Comment 14

3 years ago
Related problem is that XMLHttpRequest doesn't obey Ctrl + F5 or Ctrl + Shift + R.
Whiteboard: [necko-active] → [necko-next]
Assignee

Comment 15

3 years ago
Stealing from Patrick.
Assignee: mcmanus → honzab.moz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: [necko-next] → [necko-active]
Assignee

Comment 16

3 years ago
Never ran this code!

According https://tools.ietf.org/html/rfc7234#section-5.2.1 ; not implementing no-transform and only-if-cached.

Need to write tests before review.
Attachment #315506 - Attachment is obsolete: true
Attachment #8752242 - Flags: feedback?(mcmanus)
Assignee

Updated

3 years ago
Attachment #8752242 - Attachment description: wip1 (no-store, no-cache, max-*, min-* requests C-c headers) handling → wip1 (no-store, no-cache, max-*, min-* requests C-c headers handling)
Comment on attachment 8752242 [details] [diff] [review]
wip1 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

I haven't read this in detail, but this all makes sense to me.. I would add more nspr logging..
Attachment #8752242 - Flags: feedback?(mcmanus) → feedback+
Assignee

Comment 18

3 years ago
- no-store: must not open cache entry for read or write (complete bypass) ; put to both OpenCacheEntry and OnCacheEntryCheck in case the header would be added later
- no-cache: I think the proper way is to respect it after we check load flags (usually set by chrome, which have a higher priority) ; simply forces validation when set
- max-stale: checked before max-age because it supersedes it ; validate = max-stale time < (age - freshness lifetime)
- max-age: validate = max-age time < age
- min-fresh: validate = min-fresh time > (freshness lifetime - age) ; all in this order

- to be realistic and also since I wanted to use cacheStorage.exists() synchronous function, the test needs a profile and an up-to-date cache index ; the test syncs on the index to build up first
- I'm using get-cache-size callback, which waits for the cache index to first build or update.  
- on first run we start building in 50 seconds from the start ; with this patch asking for the size starts the update immediately.  it's beneficial also for users, since the cache size in UI will be up-to-date when they open prefs soon after start
- added SkipUntil method to Tokenizer ; simply throws the input away until EOF or the token is hit

Nathan, please check Tokenizer changes.
Michal, please check the changes in CacheIndex.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=020301193ef3
Attachment #8752242 - Attachment is obsolete: true
Attachment #8753037 - Flags: review?(nfroyd)
Attachment #8753037 - Flags: review?(michal.novotny)
Attachment #8753037 - Flags: review?(mcmanus)
Assignee

Comment 19

3 years ago
Comment on attachment 8753037 [details] [diff] [review]
v1 (no-store, no-cache, max-*, min-* requests C-c headers handling)

Nathan, sorry drop the r?, the Tokenizer changes will update once more, the patch overall needs some updates (try run isn't all green).
Attachment #8753037 - Flags: review?(nfroyd)
Comment hidden (obsolete)
Assignee

Comment 21

3 years ago
for comments on the patch see comment 18

Nathan: please check Tokenizer changes
Michal: please check CacheIndex changes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2b0ec0826c8f96fadf70f26b2b2f22516c63e77
Attachment #8753347 - Attachment is obsolete: true
Attachment #8753450 - Flags: review?(nfroyd)
Attachment #8753450 - Flags: review?(michal.novotny)
Attachment #8753450 - Flags: review?(mcmanus)
Comment on attachment 8753450 [details] [diff] [review]
v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

r=me on the xpcom parts, with the change below.

::: xpcom/tests/gtest/TestTokenizer.cpp
@@ +694,5 @@
> +  EXPECT_TRUE(p.CheckChar(',')); // check the first comma of the ',,,' string
> +
> +  p.Rollback(); // moves cursor back to the first comma of the ',,,' string
> +
> +  p.SkipUntil(Tokenizer::Token::Char(',')); // must not move, we are on the ',' char

I think these checks for not moving are good, but we should also have a test that checks whether the rollback is correctly reset when we skip.  I think your Rollback() call above is meant to test something like it?  Something like:

Tokenizer p("test that rollback works, correctly");

p.SkipUntil(Tokensizer::Token::Char(','));
// Not sure if we can check without moving rollback...
EXPECT_TRUE(p.CheckChar(','))

p.Rollback();
EXPECT_TRUE(p.CheckWord("test"));
Attachment #8753450 - Flags: review?(nfroyd) → review+
Comment on attachment 8753450 [details] [diff] [review]
v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

::: xpcom/ds/Tokenizer.h
@@ +162,5 @@
>    void SkipWhites(WhiteSkipping aIncludeNewLines = DONT_INCLUDE_NEW_LINE);
>  
> +  /**
> +   * Skips all tokens until the given one is found or EOF is hit.  The token
> +   * or EOF are next to read.

Sorry, forgot to mention that this comment needs a little wordsmithing.  How about:

Skips all tokens until the given one is found or EOF is hit.  The token, if present, is not consumed by this call.
Assignee

Comment 24

3 years ago
(In reply to Nathan Froyd [:froydnj] from comment #22)
> Comment on attachment 8753450 [details] [diff] [review]
> v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

> I think these checks for not moving are good, but we should also have a test
> that checks whether the rollback is correctly reset when we skip.  I think
> your Rollback() call above is meant to test something like it?  Something
> like:
> 
> Tokenizer p("test that rollback works, correctly");
> 
> p.SkipUntil(Tokensizer::Token::Char(','));
> // Not sure if we can check without moving rollback...
> EXPECT_TRUE(p.CheckChar(','))
> 
> p.Rollback();
> EXPECT_TRUE(p.CheckWord("test"));

Yeah, I was thinking of it too.  I think we need something like:

Tokenizer p("test0,test1,test2");
p.SkipUntil(Tokenizer::Token::Char(','));
EXPECT_TRUE(p.CheckChar(','))
p.SkipUntil(Tokenizer::Token::Char(','));
p.Rollback();
EXPECT_TRUE(p.CheckWord("test1"));
EXPECT_TRUE(p.CheckChar(','))

(In reply to Nathan Froyd [:froydnj] from comment #23)
> Skips all tokens until the given one is found or EOF is hit.  The token, if
> present, is not consumed by this call.

Nicer wording, definitely :)  Will update.  Thanks!
Comment on attachment 8753450 [details] [diff] [review]
v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

> +  if (index->mUpdateTimer) {
> +    // Move forward with index re/building if it is pending
> +    index->mUpdateTimer->Cancel();
> +    index->mUpdateTimer = nullptr;
> +    index->ScheduleUpdateTimer(0);
> +  }

I need to double check it but I think mUpdateTimer is initialized always on Cache IO thread. This method is called mainly on the main thread, so in theory CacheIndex::DelayedUpdate could be called twice. Btw, why is this change needed for this bug?
Flags: needinfo?(honzab.moz)
Attachment #8753450 - Flags: review?(michal.novotny)
Assignee

Comment 26

3 years ago
(In reply to Michal Novotny (:michal) from comment #25)
> Comment on attachment 8753450 [details] [diff] [review]
> v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)
> 
> > +  if (index->mUpdateTimer) {
> > +    // Move forward with index re/building if it is pending
> > +    index->mUpdateTimer->Cancel();
> > +    index->mUpdateTimer = nullptr;
> > +    index->ScheduleUpdateTimer(0);
> > +  }
> 
> I need to double check it but I think mUpdateTimer is initialized always on
> Cache IO thread. This method is called mainly on the main thread, so in
> theory CacheIndex::DelayedUpdate could be called twice.

OK, so more correct would be to do this whole block on the IO thread?

>  Btw, why is this
> change needed for this bug?

See comment 18.
Flags: needinfo?(honzab.moz) → needinfo?(michal.novotny)
(In reply to Honza Bambas (:mayhemer) from comment #26)
> > > +  if (index->mUpdateTimer) {
> > > +    // Move forward with index re/building if it is pending
> > > +    index->mUpdateTimer->Cancel();
> > > +    index->mUpdateTimer = nullptr;
> > > +    index->ScheduleUpdateTimer(0);
> > > +  }
> > 
> > I need to double check it but I think mUpdateTimer is initialized always on
> > Cache IO thread. This method is called mainly on the main thread, so in
> > theory CacheIndex::DelayedUpdate could be called twice.
> 
> OK, so more correct would be to do this whole block on the IO thread?

Yes

> >  Btw, why is this
> > change needed for this bug?
> 
> See comment 18.

I overlooked this. Makes sense.
Flags: needinfo?(michal.novotny)
Comment on attachment 8753450 [details] [diff] [review]
v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

r+ assuming you're fine with my comments..

::: netwerk/cache2/CacheIndex.cpp
@@ +1368,5 @@
>    LOG(("CacheIndex::AsyncGetDiskConsumption - remembering callback"));
>    // Will be called when the index get to the READY state.
>    index->mDiskConsumptionObservers.AppendElement(observer);
>  
> +  if (index->mUpdateTimer) {

I feel like this change wasn't supposed to be in this patch

::: netwerk/protocol/http/CacheControlParser.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* vim:set expandtab ts=4 sw=4 sts=4 cin: */

these seem to be set to 4 spaces while your code (thankfully!) is using 2. same thing in cpp

::: netwerk/test/unit/head_cache2.js
@@ +409,5 @@
>    this.goon = goon;
>    this.delayed = delayed;
>  }
>  
> +function wait_for_cache_index(continue_func)

I don't think you meant to include this in this patch
Attachment #8753450 - Flags: review?(mcmanus) → review+
Assignee

Comment 29

3 years ago
(In reply to Patrick McManus [:mcmanus] from comment #28)
> Comment on attachment 8753450 [details] [diff] [review]
> v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)
> 
> Review of attachment 8753450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming you're fine with my comments..
> 
> ::: netwerk/cache2/CacheIndex.cpp
> @@ +1368,5 @@
> >    LOG(("CacheIndex::AsyncGetDiskConsumption - remembering callback"));
> >    // Will be called when the index get to the READY state.
> >    index->mDiskConsumptionObservers.AppendElement(observer);
> >  
> > +  if (index->mUpdateTimer) {
> 
> I feel like this change wasn't supposed to be in this patch
> 
> 
> ::: netwerk/test/unit/head_cache2.js
> @@ +409,5 @@
> >    this.goon = goon;
> >    this.delayed = delayed;
> >  }
> >  
> > +function wait_for_cache_index(continue_func)
> 
> I don't think you meant to include this in this patch

No, I did mean, very much.  Those are prerequisites for the test to not block for 50 seconds.  This all is explained in comment 18, btw.

If you believe this should be separated to a different bug, maybe you are right.  I can file it if you insist.  Honestly, would make this feature easier to back out when we find problems (which I assume we will).

> ::: netwerk/protocol/http/CacheControlParser.h
> @@ +1,2 @@
> > +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/* vim:set expandtab ts=4 sw=4 sts=4 cin: */
> 
> these seem to be set to 4 spaces while your code (thankfully!) is using 2.
> same thing in cpp

Copied the header from a bad (old style) file, will update.
Flags: needinfo?(mcmanus)
> > 
> > I don't think you meant to include this in this patch
> 
> No, I did mean, very much.  Those are prerequisites for the test to not
> block for 50 seconds.  This all is explained in comment 18, btw.
> 

sounds good.. leave it in this patch. thanks
Flags: needinfo?(mcmanus)
Assignee

Updated

3 years ago
Depends on: 1274583
Assignee

Updated

3 years ago
Depends on: 1274585
Assignee

Comment 31

3 years ago
[r+ comment 28]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00049c9136e4

I think it's better to split it.  Those two other changes are good to have in the tree regardless this patch.  And I think this is going to bring webcompat issues and may need to back out, so, making it easier just in case.
Attachment #8753450 - Attachment is obsolete: true
Attachment #8754847 - Flags: review+
Assignee

Comment 32

3 years ago
And here with fixed formatting comments.
Attachment #8754847 - Attachment is obsolete: true
Attachment #8754863 - Flags: review+

Comment 34

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a475c764e2
support Cache-control directives in HTTP requests, r=mcmanus+michal+froydnj
Keywords: checkin-needed

Comment 35

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/26a475c764e2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

3 years ago
Depends on: 1277354
Duplicate of this bug: 1302119
You need to log in before you can comment on or make changes to this bug.