203271 - max-age should override expires in http headers

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Tom Everingham]

Overview Description:  Max-age should override the expires header if a response
includes both.  
See rfc2616 section 14.9.3 -
http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.9.3  

Steps to Reproduce:
1.)  Run the test script which uses a clock to demonstrate the problem. (sorry,
internal only but I will attach the script)
2.)  Click reload (or highlight the url and press enter)

Actual Results:  The html Date/Time display updates on every reload

Expected Results:  The Date/Time display should wait 20 seconds before updating.

Build Date & Platform Bug Found:  WinNT, Linux trunk builds 04/24/03

Additional Information:  See cookie bug 177930

Comment 1

[Tom Everingham]

Attachment: script for expires + max-age

Comment 2

[Darin Fisher]

-> suresh

Comment 3

[MG]

*** Bug 234105 has been marked as a duplicate of this bug. ***

Comment 4

[Ken Johanson]

In my 2004092716 build, Expires itself is not even being honored.. I tried with
cache setting to 'check evey time'....

Modified and Expires info on info-screen (CTRL-I) is also behaving strangely.

Sample:
http://www.kensystem.com/mozilla/date-headers.jsp

returns:
HTTP/1.x 200 OK
Content-Type: text/html
Transfer-Encoding: chunked
Connection: Keep-Alive
MIME-version: 1.0
Server: SAMBAR/JavaEngine
Date: Tue, 19 Oct 2004 21:24:53 GMT
Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Keep-Alive: timeout=180

Comment 5

[Darin Fisher]

headers for that page are:

HTTP/1.1 200 OK
Content-Type: text/html
MIME-version: 1.0
Server: SAMBAR/JavaEngine
Date: Tue, 19 Oct 2004 21:45:24 GMT
Last-Modified: Thu, 01 Jan 1970 00:00:00 GMT
Expires: Thu, 01 Jan 1970 00:00:00 GMT

mozilla generates expiration time of:

Wed 31 Dec 1969 04:00:00 PM PST

that looks correct to me.

Comment 6

[Ken Johanson]

Darin, just wanted to be sure I'm interpreting your assessment of 
'correct' correctly  :-) 

Are you saying that those dates should be ignored because they represent a 
pre-epoch value (negating correct timezone-offset calculation); in other words 
Mozilla is okay? 

Or, just that the headers are okay and Moz is broke? :-)

Thanks,
ken

Comment 7

[Darin Fisher]

i'm saying that

  "Thu, 01 Jan 1970 00:00:00 GMT" == "Wed 31 Dec 1969 04:00:00 PM PST"

so, mozilla appears to be parsing the headers correctly.  are you saying that
mozilla does not re-fetch the page when loaded a second time despite the
expiration time?

Comment 8

[Ken Johanson]

Sorry Darin, just me being retarded :-)

You're right, Moz isnt refetching the page, even tried a couple different cache
settings.

It's almost as if the reponse or date parser itself is not fully working since
several date headers seem to not working.

Comment 9

[Ken Johanson]

This might narrow it a little bit. Even though the page isnt being re-fetched as
it should, and Page-Info shows no values being set for these two headers,
Javascript's document.lastModified *does* show the Last-Modified header value
(see my sample URL) -- so it would appear that the header/date parser is
working. I dont know of a javascript property that would print Expires though...

Comment 10

[Darin Fisher]

could you do me a favor and generate a Mozilla HTTP log showing the problem you
are seeing?  instructions available here:

  http://www.mozilla.org/projects/netlib/http/http-debugging.html

please use the 'Create a New Attachment' link in this bug report to attach the
resulting log file to this bug.  thanks!

Comment 11

[Ken Johanson]

Attachment: http-debug

Okay, here it is. I set my startup page to the home page which I made my sample
URL. After it loaded I click on google.com, then hit the back button, then
closed the app.

Comment 12

[Darin Fisher]

> Okay, here it is. I set my startup page to the home page which I made my sample
> URL. After it loaded I click on google.com, then hit the back button, then
> closed the app.

the browser is not required to fetch an expired page from the server when the
user navigates to it via the back button.  if you press enter in the URL bar or
click a link to the page, then it should result in a fetch from the server (or a
conditional GET in the hopes of receiving a 304).

Comment 13

[Ken Johanson]

In that case, the desired behavior is occuring, and my assertion that Expires
was broken is not true. The Page-Info screen still seems to have a problem
though, this is part of my excuse for being confused :-).

I read http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2 and agree
that the history mechanism is not required to re-query the page (though for
reaons of application programming I wish the reload could be forced even against
the history)

Thanks.

Comment 14

[Darin Fisher]

> In that case, the desired behavior is occuring, and my assertion that Expires
> was broken is not true. The Page-Info screen still seems to have a problem
> though, this is part of my excuse for being confused :-).

yeah, page-info probably has a bug.  feel free to file a separate bug on it
since it is not part of the networking library, this wouldn't be the right place
to get into the details of its shortcomings.


> ...I wish the reload could be forced even against the history)

send 'cache-control: no-store' and it will be.

Comment 15

[Ken Johanson]

Thanks Darin!!! Years of playing with this stuff, didnt know that!! Awesome.

Filed Page-Info bug#265167

Comment 16

[Ross MacKinnon]

I'm a bit confused by the comments here.  Is the thinking that this isn't a bug?
Because from what I can tell, Firefox (as of 1.0.4) still has an issue, when the
Expires is set in the past, and max-age (in Cache-Control) is in the future. 

When this happens, Firefox acts as if there was no expiration set at all, and it
re-fetches the page from the server.  I've confirmed this by watching my local
Apache access log that I'm testing with.  

If I put an Expires value in the future, or have no Expires at all, the
Cache-Control max-age is properly taking precedence, as per the spec
(http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.2.4).  But there
seems to be a bug when the Expires is any time in the past.

I purposefully want an Expires in the past and max-age in the future, in order
to prevent HTTP/1.0 proxies caching a private document (since they won't
recognize Cache-Control, of course, and the "private" attribute I specify
there), yet still allow the browser to cache the document.

Here's some response headers I'm setting:

This works:
==========
Date: Wed, 08 Jun 2005 22:47:31 GMT

Expires: Wed, 08 Jun 2005 22:57:32 GMT

Cache-Control: private, max-age=86400, must-revalidate


Page Info reports the Expires time as: 06/09/2005 03:47:30 PM (PDT), as expected
(based on max-age), and my Apache access log shows the page is not being
requested, as expected.

This does NOT work:
==================
Date: Wed, 08 Jun 2005 22:51:55 GMT

Expires: Wed, 08 Jun 2005 22:41:56 GMT

Cache-Control: private, max-age=86400, must-revalidate


Page Info reports the Expires time as "Not Specified".  In this scenario, it
should be the same as in above test: 06/09/2005 03:47:30 PM (PDT).  And I see
the document re-fetched in the Apache access log.

Comment 17

[Ross MacKinnon]

has anyone seen my comments?

Comment 18

[Ross MacKinnon]

is anyone looking into this?

Comment 19

[u7736]

I'm not sure where the middle of the discussion went here, but +1 to Ross and the original reporter -- this is a problem; the behaviour seen isn't conformant with RFC2616. Mozilla is the only major browser that doesn't do this; IE6, Safari and Opera all conform to the RFC. 

This behaviour breaks sites that want to separate caches that understand HTTP/1.1 caching semantics from those that don't.

See also:
  http://www.mnot.net/javascript/xhr/cache.html#max_age_expires
for a test of this (among other things)

Comment 20

[Steven Chamberlain]

Could I please emphasise that the originally reported problem persists, and that the earlier comments have nothing to do with the originally reported issue.  The problem is exactly as Ross explained in Comment #16 and Mark in Comment #19.

I tested this in the latest Firefox 2.x release and have just confirmed that it is still the case in the latest development build (3.1a1pre).  The behaviour is in direct contravention of RFC 2616, section 14.9.3:

   If a response includes both an Expires header and a max-age
   directive, the max-age directive overrides the Expires header, even
   if the Expires header is more restrictive. This rule allows an origin
   server to provide, for a given response, a longer expiration time to
   an HTTP/1.1 (or later) cache than to an HTTP/1.0 cache.

I serve some content which must not be cached by HTTP/1.0 caches, because they do not support the newer Cache-Control directives.  I therefore use the technique suggested above in the text of the RFC, of providing a max-age directive for the benefit of HTTP/1.1 caches, and also an Expires header which will disable caching by HTTP/1.0 caches.  I'm sure there will be many content providers who do the same.

Until this bug is fixed, Firefox users will not benefit from local caching of content from content servers employing this technique, and will suffer an unnecessarily slower browsing experience.

Comment 21

[Bjarne (:bjarne)]

Attachment: xpcshell test and proposed patch

Investigating bug #462243 took me here. Since activity around this defect seems to be low I take the liberty to attach a xpcshell testcase and a proposed patch.

This problem occurs because the call to mResponseHead->MustValidate() early in nsHttpChannel::UpdateExpirationTime() returns 'true', resulting in expiration time for the cache-entry being set to 0. nsHttpResponseHead::MustValidate() relies on nsHttpResponseHead::ExpiresInPast(), which does not take max-age into account, hence this particular situation expires the cache-entry even though max-age says differently.

Comment 22

[Bjarne (:bjarne)]

Comment on attachment 346877 [details] [diff] [review]
xpcshell test and proposed patch

Changed reviewer

Comment 23

[Bjarne (:bjarne)]

This issue describes a direct validation of RFC 2616. Requesting for 3.1.

Comment 24

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

(In reply to comment #21)
> This problem occurs because the call to mResponseHead->MustValidate() early in
> nsHttpChannel::UpdateExpirationTime() returns 'true',

Why does it do that?

Comment 25

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

Comment on attachment 346877 [details] [diff] [review]
xpcshell test and proposed patch

Oh, this is specifically for the case that expires<date, but max-age is specified.

I think this is not correct though. What you want is make ExpiresInPast return true if NS_SUCCEEDED(GetMaxAgeValue()), and handle the check whether max-age expired using the normal mechanism (i.e. the expiration time of the cache entry)

Comment 26

[Bjarne (:bjarne)]

(In reply to comment #25)
> Oh, this is specifically for the case that expires<date, but max-age is
> specified.

Not really. Max-age takes precedence over expires. I.e. if both are present max-age is used, otherwise expires is used, regardless of their values. This is how I interpret RFC 2616 section section 14.9.3. See also comment #20.

The general logic in the code seems correct and deals with max-age, as far as I can see, but max-age must have been forgotten when writing ExpiresInPast(). The suggested patch simply adds max-age to ExpiresInPast() since the rest of the logic relies on it.
 
> I think this is not correct though. What you want is make ExpiresInPast return
> true if NS_SUCCEEDED(GetMaxAgeValue()), and handle the check whether max-age
> expired using the normal mechanism (i.e. the expiration time of the cache
> entry)

Do you mean that ExpiresInPast() should return true if a max-age directive is present, regardless of the value of the directive? And I don't understand which normal mechanism you mean - is it a utility-method? Keep in mind that this issue arise when expiration-time of the cache-entry is about to be updated based on the headers in the response, thus the cache-entry does not have up-to-date expiration-info at this point.

If you provide a triplet [current-time, expires, max-age] which makes the logic fail I might better understand what you mean.

Comment 27

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

(Urg, I need a better way to deal with bugmail... Sorry for only seeing this now)

(In reply to comment #26)
> Not really. Max-age takes precedence over expires. I.e. if both are present
> max-age is used, otherwise expires is used, regardless of their values. This is
> how I interpret RFC 2616 section section 14.9.3. See also comment #20.

Yeah I know. I meant we only have a bug in the case I mentioned, not in other cases involving max-age.

> Do you mean that ExpiresInPast() should return true if a max-age directive is
> present, regardless of the value of the directive? And I don't understand which
> normal mechanism you mean - is it a utility-method? Keep in mind that this
> issue arise when expiration-time of the cache-entry is about to be updated
> based on the headers in the response, thus the cache-entry does not have
> up-to-date expiration-info at this point.

I meant false instead of true, sorry. That way you'd implement the exact semantics you want: If max-age is present, you pretend that there was no expires header.

This method is not only used when calculating the expires time. With this patch, the expiration time of the cache entry won't be considered when checking whether to reuse it. See nsHttpChannel::CheckCache. You're also making MustValidate dependent on the current time which I don't think is a good idea.

> If you provide a triplet [current-time, expires, max-age] which makes the logic
> fail I might better understand what you mean.

For example, your patch will incorrectly claim that a resource is outdated when the server time (i.e. the Date: header) is set incorrectly. E.g.:

Date: Sun, 02 Mar 2008 13:46:42 GMT
Expires: Tue, 02 Mar 2010 13:46:42 GMT
Cache-Control: private, max-age=2592000

If a header like that is received today, your patch will assume it has to be validated immediately.

Comment 28

[Bjarne (:bjarne)]

Ahh - that clears up some things. :)

I see that I've over-estimated the semantics of ExpiresInPast() - it's simply supposed to return true if the Expires-header is in the past relative to the Date-header. I tried to make it do more of the expiration-model, which was wrong.

It looks like it would it be sufficient to just return false from ExpiresInPast() if the max-age directive is present... the rest of the code should take care of the rest of the expiration-model. Is this aligned with your understanding? I'll attach an updated patch which passes all the tests.

> For example, your patch will incorrectly claim that a resource is outdated when
> the server time (i.e. the Date: header) is set incorrectly. E.g.:
> 
> Date: Sun, 02 Mar 2008 13:46:42 GMT
> Expires: Tue, 02 Mar 2010 13:46:42 GMT
> Cache-Control: private, max-age=2592000
> 
> If a header like that is received today, your patch will assume it has to be
> validated immediately.

I agree with your argument, but shouldn't this entry in fact be considered expired at the end of the day anyway? According to RFC 2616 section 12.2.3, the age of this entry is one year (or 31536000 seconds). Furthermore, according to section 13.2.4, the freshness_lifetime of this entry is derived from max-age = 2592000 seconds (or 1 month). In other words : the entry has expired. Or...?

Do you think it would be useful to add this example to the list of tests, btw?

Comment 29

[Bjarne (:bjarne)]

Attachment: Updated patch

Updated patch as suggested in previous comment.

Comment 30

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

(In reply to comment #28)
> It looks like it would it be sufficient to just return false from
> ExpiresInPast() if the max-age directive is present... the rest of the code
> should take care of the rest of the expiration-model. Is this aligned with your
> understanding? I'll attach an updated patch which passes all the tests.

Yep, that's exactly what I suggested :)

> I agree with your argument, but shouldn't this entry in fact be considered
> expired at the end of the day anyway? According to RFC 2616 section 12.2.3, the
> age of this entry is one year (or 31536000 seconds). Furthermore, according to
> section 13.2.4, the freshness_lifetime of this entry is derived from max-age =
> 2592000 seconds (or 1 month). In other words : the entry has expired. Or...?

Hm, yeah, good point. RFC 2616 does assume that clocks are reasonably well synchronized, which I'm not sure is a particularly good assumption.

> Do you think it would be useful to add this example to the list of tests, btw?

yeah, that seems like a good idea.

Comment 31

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

Comment on attachment 365317 [details] [diff] [review]
Updated patch

+++ b/netwerk/test/unit/test_bug203271.js	Wed Mar 04 00:39:46 2009 +0100
+    {url : "/precedence", server : "0", expected : "0", responseheader :
+                                                            [ "Expires: "+getDateString(-1),
+                                                              "Cache-Control: max-age=3600"]},


Don't put a space in front of the colon. Also, if you put responseheader on the second line, you could easily limit your lines to 80 characters.

+function logit(i, data, ctx) {

Please put spaces around the operators in this function (i.e. the pluses), and indent the comment correctly. Also limit the lines to 80 characters (in the entire file)

+    httpChan.requestMethod = "GET";

that's the default...

+    channel.asyncOpen(new ChannelListener(checkValueAndTrigger, channel),null);

space after the comma please

+    if (index < tests.length-1) {

space around the minus

+        for (var i=0;i<header.length;i++) {

spaces around = and < and after ;

+            var splitHdr = header[i].split(": "); // NOTE split string!

what does that note mean?

Comment 32

[Bjarne (:bjarne)]

> Hm, yeah, good point. RFC 2616 does assume that clocks are reasonably well
> synchronized, which I'm not sure is a particularly good assumption.
> 
> > Do you think it would be useful to add this example to the list of tests, btw?
> 
> yeah, that seems like a good idea.

I tried your example when updating the unit-test but it doesn't expire. IMO it should according to RFC 2616, and I have a feeling you agree. The currently suggested patch is not capable of handling this entry though.

Should we consider your example rare and broken and open a new bug to handle it? Or would you prefer that the patch for this bug handles it?

Comment 33

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

A new bug seems better. When I was briefly looking at the code it seemed like it should expire though... anyway, let's not discuss that in this bug :)

Comment 34

[Bjarne (:bjarne)]

Attachment: Updated unit-test

Unit-test updated according to comments from reviewer. The example given in comment #27 is added, but commented out as it 1) requires a minor patch to httpd.js and 2) fails.

Bug #481887 has been registered to address cases like the example in comment #27.

Comment 35

[Bjarne (:bjarne)]

(In reply to comment #33)
> A new bug seems better. When I was briefly looking at the code it seemed like
> it should expire though... anyway, let's not discuss that in this bug :)

After adding a 1s delay before re-requesting the test for the example, it expires as expected. Thus FF actually handles your example, like you expected.

Comment 36

[Bjarne (:bjarne)]

Attachment: Updated unit-test + patch for httpd.js

Unit-test now includes some cases when clocks on server and client are really out of sync. To make this work a simple patch to httpd.js is also required, which is why I'm requesting a new review.

Comment 37

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

Comment on attachment 366139 [details] [diff] [review]
Updated unit-test + patch for httpd.js

+// The test below is the example from
+//
+//         https://bugzilla.mozilla.org/show_bug.cgi?id=203271#c27
+//
+//  max-age=2592000s (1 month), expires=1 year from now, date=1 year ago

please indent comments like the code around them

+// The two tests below are also examples of clocks really out of synch

here too

+ // Dump all response-headers

and here

+// This logic has no effect since httpd.js overrides the date-header.
+// Leaving it here anyway since that might change.

and here

Isn't the comment now wrong though?

+    var date = tests[index].explicitDate;
+    if (date == null) {
and
+    var header = tests[index].responseheader;
+    if (header == null) {

you want undefined instead of null for checking whether a property is not defined

(It occurs to me that getDateString produces a wrong weekday when a year delta is specified... guess that doesn't really matter)


It's very unfortunate that this unit test introduces multi-second delays though :/

Comment 38

[Bjarne (:bjarne)]

Attachment: Hopefully final version [Backout: Comment 41]

Updated to address latest comments.

The yearDelta is a rather coarse-grained way to offset the date, so a a day or so is not important.

To address the issue of multiple delays the test-list have been re-organized to first load all initial entries into the cache, then delay once for 3s, and finally re-load all entries to check for validity. It reduces time with 8s per run.

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 366539 [details] [diff] [review]
Hopefully final version
[Backout: Comment 41]


http://hg.mozilla.org/mozilla-central/rev/be6d39f94146

Comment 40

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> updated patch which passes all the tests.

Was it ?

Comment 41

[Serge Gautherie (:sgautherie)]

Comment on attachment 366539 [details] [diff] [review]
Hopefully final version
[Backout: Comment 41]


http://hg.mozilla.org/mozilla-central/rev/6f9cdb6932a2
Backed out [...] which breaks all test suites.

See:
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1236957092.1236961255.2098.gz
Linux comm-central check on 2009/03/13 08:11:32
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1236959150.1236965227.9991.gz
MacOSX 10.4 comm-central check on 2009/03/13 08:45:50
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1236959252.1236964250.8203.gz
Win2k3 comm-central check on 2009/03/13 08:47:32
and
https://bugzilla.mozilla.org/show_bug.cgi?id=203271
Linux mozilla-central unit test on 2009/03/13 07:54:53
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236956093.1236965206.9944.gz
OS X 10.5.2 mozilla-central unit test on 2009/03/13 07:54:53

Comment 42

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)

And last
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236956093.1236968361.15551.gz
WINNT 5.2 mozilla-central unit test on 2009/03/13 07:54:53

Comment 43

[Jeff Walden [:Waldo]]

I don't know how your httpd.js patch could possibly have worked; getHeader throws if the given header isn't present.

Setting a Date header iff one hasn't already been set seems like reasonable behavior for this scenario, but you really should have run it past me.

Comment 44

[Bjarne (:bjarne)]

(In reply to comment #43)
> I don't know how your httpd.js patch could possibly have worked; getHeader
> throws if the given header isn't present.

*blush*
Stupid mistake - I forgot to run the full mochitestsuite after the new test passed. Seems like I screwed up this one... my apologies! :(

> Setting a Date header iff one hasn't already been set seems like reasonable
> behavior for this scenario, but you really should have run it past me.

I see now that should definitely have requested another review, yes!

Glad you agree that the behaviour is reasonable though - any suggestions for a more robust solution? Try-catch? HasHeaderValue() method or similar (not sure that one is mapped to JS).

Comment 45

[Jeff Walden [:Waldo]]

(In reply to comment #44)
> Glad you agree that the behaviour is reasonable though - any suggestions for a
> more robust solution? Try-catch? HasHeaderValue() method or similar (not sure
> that one is mapped to JS).

There's a hasHeader method on nsHttpHeaders that's not exposed on Response, just do response._headers.hasHeader("Date") here, I think.  Don't worry too much about exactly what, as what you have is going to be moved around a lot as part of bug 396226.

Comment 46

[Bjarne (:bjarne)]

Attachment: Proper patch to httpd.js

The only change in this version is to use response_headers.hasHeader() as suggested in comment #45. Requesting review by Jeff due to this.

All mochitests have passed with this patch (with the normal exceptions since I'm using 64-bit Ubuntu).

Comment 47

[Jeff Walden [:Waldo]]

Comment on attachment 367779 [details] [diff] [review]
Proper patch to httpd.js

>diff -r 844d5410d576 netwerk/test/httpserver/httpd.js

>-    response.setHeader("Date", toDateString(Date.now()), false);
>+    if (! response._headers.hasHeader("Date"))
>+        response.setHeader("Date", toDateString(Date.now()), false);

There shouldn't be a space after !, and the setHeader line should be indented only two spaces, not four.  Otherwise this looks good assuming you've tested it.

Comment 48

[Bjarne (:bjarne)]

Attachment: Previous patch with spaces removed, as requested [Checkin: Comment 49]

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 367872 [details] [diff] [review]
Previous patch with spaces removed, as requested
[Checkin: Comment 49]


http://hg.mozilla.org/mozilla-central/rev/5dd87bff4e11