Bug 8210 – Conditional XMLHttpRequest gets should pass 304 responses unchanged

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Yusuf Goolamabbas]

i, I was trying to write a simple ajaxy application to learn Mochikit. The URL
above calls some simple JS functions which get a static file periodically and
print the HTTP status response code. I have setup mod_expires headers which send outs Cache-Control: max-age=3 for the xml file delivered from the URL above

I use the If-Modified-Since and If-None-Match header fields to simulate a
conditional get. The expectation is that the the alert window will show 200 the
first time and then 304 subsequently. This behaviour is demonstrated with
Internet Explorer 6.0/Windows and Firefox 1.0.7 on MacOSX. However if I use
Safari 2.0.3 and the latest nightly build on MacOSX 10.4.6 I am only shown '200' in the alert box on the first time and then on subsequent call outs. 

I also see similar behaviour as Safari on Firefox 1.5.0.1 on Windows XP/2000
However, if I change the pref network.http.use-cache from true to false. Firefox behaves as expected (alert shows '200' the first time and then '304' subsequently)

[Alexey Proskuryakov]

With stock 10.4.6 Safari, I'm getting an alert saying "200" every 8-9 seconds. The reason is that NSURLConnection catches the 304 response and translates it into a 200 one (with a full cached response body and other headers, of course).

I am getting exactly the same results from Mac Firefox 1.5.

If you issue a conditional request the response for which is not cached, the behavior is incorrect, see bug 7385. However, normally caching is performed automatically and transparently, so you needn't specify the conditional headers at all.

So, this doesn't look like a bug to me.

[Yusuf Goolamabbas]

even if caching is done, I am sending out a Cache-Control: max-age=3 response thus after 3 seconds the response should expire and Safari (or NSURLConnection) should ask for the file again with the appropiate Request headers to the server

[Alexey Proskuryakov]

(In reply to comment #2)

  Yes, that's the behavior I am seeing. It might be skewed by an HTTP proxy I'm behind, so please attach a tcpdump or tcpflow output if you are getting different results.

[Alice Liu]

<rdar://problem/4504276>

[Yusuf Goolamabbas]

Just to let you know that I had filed a similar bug with Firefox and it was fixed recently (was marked as a blocker for FF 2.0)

https://bugzilla.mozilla.org/show_bug.cgi?id=331825

[Alexey Proskuryakov]

Thank you for for the b.m.o reference, it's now clear what we are doing wrong:

"304 Not Modified responses that are a result of a UA-generated conditional request MUST be presented as 200 OK responses with the appropriate content. Such UAs MUST allow authors 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."

It is the second part (overriding the automatic cache) that is not supported. It would good to know the reasoning behind this requirement, but in any case, we should definitely follow the spec and Firefox here.

[Alexey Proskuryakov]

Created attachment 9171 [details]
proposed fix

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Created an attachment (id=9171) [edit]
> proposed fix 

Be careful when applying this patch if you use svn-apply.  It doesn't handle property value changes on text file diffs very well (it actually includes the property change text in the contents of the new file).  If you file a bug about this, please copy me on it.  :)

[Geoffrey Garen]

I'm a little unclear about this. If the cache override behavior is specific to the XMLHttpRequest object, as the spec leads me to believe, why make the change for all WebKit requests? If the behavior should apply to all network requests, isn't this an issue for NSURLConnection to resolve?

[Darin Adler]

Comment on attachment 9171 [details]
proposed fix

I think that _web_isConditionalRequest would be more efficient if it called valueForHTTPHeaderField: instead of keyEnumerator. It's almost going out of its way to be slow!

Also, why not return BOOL rather than bool since it's a Objective-C method?

Otherwise, looks fine.

[Alexey Proskuryakov]

Created attachment 9186 [details]
proposed fix

(In reply to comment #9)
The WebKit side doesn't have a separate code path for XHR-initiated requests (which is probably good), but other requests we are making do not carry manually set conditional headers, so they are not affected. I am not sure if bypassing the cache for all conditional requests is necessarily the expected behavior for NSURLConnection.

(In reply to comment #10)
> (From update of attachment 9171 [details] [edit])
> I think that _web_isConditionalRequest would be more efficient if it called
> valueForHTTPHeaderField: instead of keyEnumerator. It's almost going out of its
> way to be slow!

  I am not sure if this change makes it much faster (probably depends on the internal implementation of valueForHTTPHeaderField:), but the resulting code definitely looks better, thank you!

> Also, why not return BOOL rather than bool since it's a Objective-C method?

Oops, fixed.

[Maciej Stachowiak]

Comment on attachment 9186 [details]
proposed fix

r=me

[Alexey Proskuryakov]

Committed revision 15150.