Bug 38428

Summary:

HTTP code 500 (and other non-4xx codes) wrongfully treated as success for subresources

Product:

WebKit

Reporter:

Thomas Steinacher <tom>

Component:

Page Loading

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, beidson, commit-queue, koivisto, playmobil

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
proposed patch	none

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	Details Formatted Diff Diff
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.