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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Paul]

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

Comment 1

[Paul]

Attachment: Fix to support RFC handing of max-age and no-cache in request

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.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

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

Comment 3

[Boris Zbarsky [: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.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Bug 706806 has a testcase for no-store

Comment 6

[perdrisat]

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

[Binyamin]

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

[Binyamin]

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

[Binyamin]

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

Comment 10

[Pierre-Antoine Champin]

+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 :-)

Comment 14

[Ondra Zizka]

Related problem is that XMLHttpRequest doesn't obey Ctrl + F5 or Ctrl + Shift + R.

Comment 15

[Honza Bambas (:mayhemer)]

Stealing from Patrick.

Comment 16

[Honza Bambas (:mayhemer)]

Attachment: wip1 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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.

Comment 17

[Patrick McManus [:mcmanus]]

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

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v1 (no-store, no-cache, max-*, min-* requests C-c headers handling)

- 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

Comment 19

[Honza Bambas (:mayhemer)]

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

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (no-store, no-cache, max-*, min-* requests C-c headers handling)

for comments on the patch see comment 18

not much has changed (small rebase, some parser cleanup), will ask for r after a green try (popped off some failing patches from the series)

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

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v1.2 (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

Comment 22

[Nathan Froyd [:froydnj]]

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"));

Comment 23

[Nathan Froyd [:froydnj]]

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.

Comment 24

[Honza Bambas (:mayhemer)]

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

[Michal Novotny [:michal]]

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?

Comment 26

[Honza Bambas (:mayhemer)]

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

Comment 27

[Michal Novotny [:michal]]

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

Comment 28

[Patrick McManus [: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+ 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

Comment 29

[Honza Bambas (:mayhemer)]

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

Comment 30

[Patrick McManus [: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

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: v1.2 [split out] (no-store, no-cache, max-*, min-* requests C-c headers handling)

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

Comment 32

[Honza Bambas (:mayhemer)]

Attachment: v1.3 [split out] (no-store, no-cache, max-*, min-* requests C-c headers handling)

And here with fixed formatting comments.

Comment 33

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8eba918466e7

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a475c764e2
support Cache-control directives in HTTP requests, r=mcmanus+michal+froydnj

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/26a475c764e2