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 LoadingAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, commit-queue, koivisto, playmobil
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
proposed patch none

Description Thomas Steinacher 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>
Comment 1 Thomas Steinacher 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.
Comment 2 Alexey Proskuryakov 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;
    }
Comment 3 Brady Eidson 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.
Comment 4 Alexey Proskuryakov 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.
Comment 5 Alexey Proskuryakov 2010-08-20 17:50:08 PDT
Created attachment 65013 [details]
proposed patch

Let's try and see what breaks?
Comment 6 Brady Eidson 2010-08-20 19:58:39 PDT
Comment on attachment 65013 [details]
proposed patch

Yes.  Let's see.  :)
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2010-08-20 22:06:46 PDT
All reviewed patches have been landed.  Closing bug.