RESOLVED FIXED Bug 38428
HTTP code 500 (and other non-4xx codes) wrongfully treated as success for subresources 
https://bugs.webkit.org/show_bug.cgi?id=38428
Summary HTTP code 500 (and other non-4xx codes) wrongfully treated as success for sub...
Thomas Steinacher
Reported 2010-05-02 03:24:41 PDT
The following code produces an alert in Firefox, but none in Safari/Chrome: <html> <head> <script src="http://example.com/NOTFOUND.js" type="text/javascript" onerror="alert('error')"></script> </head> <body> </body> </html>
Attachments
proposed patch (3.37 KB, patch)
2010-08-20 17:50 PDT, Alexey Proskuryakov 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Thomas Steinacher
Comment 1 2010-05-02 03:36:20 PDT
Sorry, my first example is actually working correctly in both browsers (although I thought I tested it, maybe a cache issue). Here's another test case that doesn't work however: test.php: <?php header('HTTP/1.1 500 Internal Server Error'); ?> alert('executing script'); onerror.html: <html> <head> <script onerror="alert('error');" src="test.php" type="text/javascript"></script> <head> <body> </body> </html> Firefox displays an "error" alert here, whereas Safari/Chrome try to execute the script (i.e. the "executing script" alert box shows up). As 500 is an error code, I believe Firefox' behavior is correct.
Alexey Proskuryakov
Comment 2 2010-05-03 10:48:12 PDT
Yes, we wrongfully only treat 4xx as error for subresources: if (resource->response().httpStatusCode() / 100 == 4) { // Treat a 4xx response like a network error for all resources but images (which will ignore the error and continue to load for // legacy compatibility). resource->httpStatusCodeError(); return; }
Brady Eidson
Comment 3 2010-05-03 11:01:07 PDT
(In reply to comment #2) > Yes, we wrongfully only treat 4xx as error for subresources: > > if (resource->response().httpStatusCode() / 100 == 4) { > // Treat a 4xx response like a network error for all resources but > images (which will ignore the error and continue to load for > // legacy compatibility). > resource->httpStatusCodeError(); > return; > } Funny thing is we do a `>= 400` check in many other places. Resource.js: if (this.statusCode >= 400) InspectorController.cpp: if (response.httpStatusCode() >= 400) { DocumentLoader.cpp: return m_substituteData.isValid() || m_response.httpStatusCode() >= 400; HistoryController.cpp: if (!unreachableURL.isEmpty() || !docLoader || docLoader->response().httpStatusCode() >= 400) NetscapePlugInStreamLoader.cpp: if (response.httpStatusCode() < 100 || response.httpStatusCode() >= 400) I wonder if there's any particular reason the cache doesn't do that, or if it's just an ancient bug.
Alexey Proskuryakov
Comment 4 2010-05-03 11:30:27 PDT
ApplicationCache does "response.httpStatusCode() / 100 != 2", which I thought was the formally correct thing to do. Of course, the networking layer insulates us from seeing things like 101 or 304 in the loader too often.
Alexey Proskuryakov
Comment 5 2010-08-20 17:50:08 PDT
Created attachment 65013 [details] proposed patch Let's try and see what breaks?
Brady Eidson
Comment 6 2010-08-20 19:58:39 PDT
Comment on attachment 65013 [details] proposed patch Yes. Let's see. :)
WebKit Commit Bot
Comment 7 2010-08-20 22:06:40 PDT
Comment on attachment 65013 [details] proposed patch Clearing flags on attachment: 65013 Committed r65774: <http://trac.webkit.org/changeset/65774>
WebKit Commit Bot
Comment 8 2010-08-20 22:06:46 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.