Summary: | Conditional XMLHttpRequest gets should pass 304 responses unchanged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuf Goolamabbas <yusufg> | ||||||
Component: | XML | Assignee: | Alexey Proskuryakov <ap> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap, ddkilzer, ian | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 420+ | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
URL: | http://goolamabbas.org/cget/cget.html | ||||||||
Attachments: |
|
Description
Yusuf Goolamabbas
2006-04-05 22:13:51 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. 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 (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. 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 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. Created attachment 9171 [details]
proposed fix
(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. :) 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 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.
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 on attachment 9186 [details]
proposed fix
r=me
Committed revision 15150. |