Skip to content

Commit

Permalink
Scratch that, just punting on Opera and 304s for now - there may not …
Browse files Browse the repository at this point in the history 
…be a good solution here. Fixes #6060.
  • Loading branch information
@jeresig
jeresig committed Sep 21, 2010
1 parent ba9e0fc commit dc8491d
Showing 1 changed file with 3 additions and 7 deletions.
10 changes: 3 additions & 7 deletions src/ajax.js
View file
Expand Up @@ -629,11 +629,8 @@ jQuery.extend( jQuery.ajax, {
try {
// IE error sometimes returns 1223 when it should be 204 so treat it as success, see #1450
return !xhr.status && location.protocol === "file:" ||
( xhr.status >= 200 && xhr.status < 300 ) ||
xhr.status === 304 || xhr.status === 1223 ||
// Opera returns a status of 0 for redirects -
// We can detect this by the fact that Opera also doesn't return any headers
xhr.status === 0 && !xhr.getAllResponseHeaders();
xhr.status >= 200 && xhr.status < 300 ||
xhr.status === 304 || xhr.status === 1223;
} catch(e) {}

return false;
Expand All @@ -652,8 +649,7 @@ jQuery.extend( jQuery.ajax, {
jQuery.ajax.etag[url] = etag;
}

// Opera returns 0 when status is 304
return xhr.status === 304 || xhr.status === 0 && !xhr.getAllResponseHeaders();
return xhr.status === 304;
},

httpData: function( xhr, type, s ) {
Expand Down

22 comments on commit dc8491d

@peol
Copy link

@peol peol commented on dc8491d Sep 21, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, at least until another 'fix' is available (if ever).

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could solve this with a wrapper around the xhr object. That way when the wrappers abort method is called it replaces the httpSuccess method with one that simply returns false or you could check a isAborted() method or something in the httpSuccess method when abort() is called.

@jeresig
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: The problem is that it's not only aborts that trigger this condition: Losing network access also causes this to occur (naturally a wrapper around abort would allow us to handle that one edge of the case). Unfortunately Opera seems to have confused aborts/lost network access with 304s - for some reason.

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to break out a very specific browser version sniff + weak inference. Do you know the range of Opera versions affected?

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toString.call(window.opera) == "[object Opera]" is a pretty solid inference (as internal [[Class]] are like impossible to fake) and then sniff the affected version range (as there seems to be no viable FT for this)

@jeresig
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: It affects all versions that I can find (right up to current). I'm really scared to do a test like this since it would break functionality in future (presumably working) versions of Opera - the abort and lost network access would both trigger as a success. Additionally implementing that check would dissuade Opera from ever fixing the issue (since it would also break jQuery at the same time). Ugh.

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeresig - Welp sucko, when they fix it then something like a weak inference + version sniff will do. As for them not fixing bugs because it will affect a js lib; that is my new pet peeve, browsers should fix bugs and let libs fix there own bugs :P

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, even if you do make a zero status code return a success in Opera, people that notice this issue are probably just going to go ahead and continue implementing if (xhr.status === 0) { return s.error(xhr, statusCode); } in their success callbacks (this is basically the workaround everyone appears to be using based on forum posts about the issue). So there’s no real win to returning success in Opera if status is 0; they really just need to fix their xhr implementation.

The closest hack available would be to 1. override xhr.abort() as per jdalton, 2. override xhr.timeout and use setTimeout to call xhr.abort() instead. This still fails to address same-origin violations or other network failure cases that may not rely on a server timeout (like, for instance, if Opera uses any sort of OS connection sensing and doesn’t even attempt a request if there is no path to the host). It also means that, just for Opera, zero status codes would need to start returning success again, which means both browser sniffing and continued incorrect behaviour in future versions of Opera that have fixed the problem unless we also proactively do version detection under the assumption that the next future version of Opera will fix the problem. So I really, really would not recommend implementing this suggestion.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about performing an actual test, say a HEAD, or a GET with a small range, to the current page. If it returns a 304 then Opera has fixed the bug, if not they haven't. (If it returns 200 then assume the worst.)

@jeresig
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousdannii: It seems unreasonable to impose an additional request upon users just for the sake of one browser. Additionally the page itself isn't guaranteed to get a 304 - or even to be GET-able. There are simply way too many possible ramifications of that action.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't make it clear that such a test should be performed only for Opera. Other browsers already are burdened in different ways for their own errors. But I get what you're saying... too many issues with too low a success rate to be worth it.

I've just noticed that XMLHttpRequest2 has an abort event. Could it be useful?

@jeresig
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousdannii: Unfortunately it still doesn't change the fact that you might be requesting resources that won't even produce a 304 to begin with (not to mention the rather extreme overhead of the extra request). I don't think the XHR2 stuff will help us here, unfortunately.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeresig, could you update http://ejohn.org/files/bugs/304/ to send a body for not_modified.php? If Opera still sets responseText to the right value despite not setting the status code then it's still usable. We may never be able to distinguish between errors and unmodified empty documents, but the use cases for empty 304s must be pretty limited.

So what I'm proposing would be: jQuery.browser.opera && xhr.status === 0 && xhr.responseText != '' (substitute something else for jQuery.browser.opera if need be)

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousdannii: 304 Not Modified MUST NOT return a body as per the HTTP/1.1 spec. Doing so is a violation.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I did not know that.

From the XMLHttpRequest spec:

For 304 Not Modified responses that are a result of a user agent generated conditional request the user agent must act as if the server gave a 200 OK response with the appropriate content. The user agent must allow setRequestHeader() to override automatic cache validation by setting request headers (e.g. If-None-Match or If-Modified-Since), in which case 304 Not Modified responses must be passed through.

What I was suggesting was to make the 304 test more typical. I guess the better way to do that would be to request success.php a second time, with the php changed to support etag etc. If Opera can't distinguish between 304s and errors for UA added headers this is a bug we must fix if we can. If Opera can only not distinguish between 304s and errors if someone manually sets the headers, then it's less of a problem. In that case perhaps all that should be done is recommend not to use the ifModified option in the .ajax() docs.

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousdannii Opera acts identically for a 304 as it does for a network failure. There is no way to differentiate between the two. There is no way to fix this bug in JS.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated john's tests: http://gist.github.com/637837

Now the good news is that in a normal caching situation it works as it should - both success.php files will pass and you'll get the right content. However looking at what actually goes on it all makes sense even less than before!

The first time a call is made to success.php the test page is supposed to add an If-None-Match header of "nocache". This works the first time, but after that, once Opera actually does have a cache, it overwrites your If-None-Match header to add in the real ETag. Then, despite still being "in progress" (as dragonfly states) the test passes and you get your responseText.

The test page then makes a second call to success.php, this time without manually setting the If-None-Match header as the browser is supposed to do that. However it doesn't, and so gets a normal 200 result.

So are there only problems when Opera receives a 304 for which it has no cache?

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curiousdannii As I stated previously, you MUST NOT return a content body with a 304 Not Modified header. Your revised test case violates the HTTP/1.1 spec and is INVALID. Please read RFC 2119 for the meaning of MUST NOT. :)

@jdalton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csnover Is he returning content when using the 304 header? I don't see it http://gist.github.com/637837#file_success.php.

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: echo 'yay';

@csnover
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton: Oh, you’re right—I completely glossed over the fact that it was exiting in the conditional. People in the forum have advocated returning a message-body along with the 304, which supposedly makes Opera stop breaking, but it is a spec vio, and I thought curiousdannii was just doing the same thing there.

@curiousdannii
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csnover, no worries.

Is there a bug for this? Or can discussion just continue here? I think this is something that still needs more research. We've identified some cases where 304s seemingly aren't a problem, and one definite case where it is. There could be more.

Please sign in to comment.