Bug 8210 - Conditional XMLHttpRequest gets should pass 304 responses unchanged
Summary: Conditional XMLHttpRequest gets should pass 304 responses unchanged
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Alexey Proskuryakov
URL: http://goolamabbas.org/cget/cget.html
Keywords: InRadar
Depends on:
Reported: 2006-04-05 22:13 PDT by Yusuf Goolamabbas
Modified: 2006-07-04 09:43 PDT (History)
3 users (show)

See Also:

proposed fix (14.54 KB, patch)
2006-07-03 10:54 PDT, Alexey Proskuryakov
darin: review-
Details | Formatted Diff | Diff
proposed fix (14.29 KB, patch)
2006-07-03 21:44 PDT, Alexey Proskuryakov
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuf Goolamabbas 2006-04-05 22:13:51 PDT
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 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)
Comment 1 Alexey Proskuryakov 2006-04-06 03:30:08 PDT
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.
Comment 2 Yusuf Goolamabbas 2006-04-09 20:25:14 PDT
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
Comment 3 Alexey Proskuryakov 2006-04-10 04:13:03 PDT
(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.
Comment 4 Alice Liu 2006-04-19 18:44:51 PDT
Comment 5 Yusuf Goolamabbas 2006-07-02 18:12:00 PDT
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)

Comment 6 Alexey Proskuryakov 2006-07-02 21:36:34 PDT
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.
Comment 7 Alexey Proskuryakov 2006-07-03 10:54:14 PDT
Created attachment 9171 [details]
proposed fix
Comment 8 David Kilzer (:ddkilzer) 2006-07-03 11:01:20 PDT
(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.  :)

Comment 9 Geoffrey Garen 2006-07-03 15:09:29 PDT
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?
Comment 10 Darin Adler 2006-07-03 17:48:37 PDT
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.
Comment 11 Alexey Proskuryakov 2006-07-03 21:44:38 PDT
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.
Comment 12 Maciej Stachowiak 2006-07-03 23:46:49 PDT
Comment on attachment 9186 [details]
proposed fix

Comment 13 Alexey Proskuryakov 2006-07-04 09:43:36 PDT
Committed revision 15150.