Bug 50589 - Should fire error event for empty 404 script
Summary: Should fire error event for empty 404 script
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Edaena Salinas
URL:
Keywords: EasyFix
Depends on:
Blocks:
 
Reported: 2010-12-06 14:21 PST by Ojan Vafai
Modified: 2012-06-07 15:09 PDT (History)
12 users (show)

See Also:


Attachments
test case (217 bytes, text/html)
2010-12-06 14:21 PST, Ojan Vafai
no flags Details
Proposed fix. (6.29 KB, patch)
2012-06-06 13:02 PDT, Edaena Salinas
beidson: review-
Details | Formatted Diff | Diff
Added Test (8.80 KB, patch)
2012-06-06 15:46 PDT, Edaena Salinas
beidson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-01 (535.85 KB, application/zip)
2012-06-07 00:20 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (655.15 KB, application/zip)
2012-06-07 00:34 PDT, WebKit Review Bot
no flags Details
Modified test (5.98 KB, patch)
2012-06-07 11:08 PDT, Edaena Salinas
no flags Details | Formatted Diff | Diff
Modified test (11.00 KB, patch)
2012-06-07 11:53 PDT, Edaena Salinas
ap: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Modified test (9.47 KB, patch)
2012-06-07 13:18 PDT, Edaena Salinas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-12-06 14:21:50 PST
Created attachment 75735 [details]
test case

The script element fires the load event and not the error event for a script that 404s but returns 0 bytes. See the test case. Firefox fires the error handler in this case. 

Note: The test depends on the HTTP server returning an empty 404 response. If that changed, obviously the test case would no longer be valid. :)
Comment 1 Ojan Vafai 2012-03-12 12:31:26 PDT
This appears to have been fixed at some point. I'll leave the bug open just for adding a layout test so we make sure this doesn't regress.
Comment 2 Edaena Salinas 2012-06-06 11:49:04 PDT
I am actually seeing this bug with ToT. Got a fix.
Comment 3 Edaena Salinas 2012-06-06 13:02:54 PDT
Created attachment 146091 [details]
Proposed fix.
Comment 4 Brady Eidson 2012-06-06 13:20:17 PDT
Comment on attachment 146091 [details]
Proposed fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=146091&action=review

Awesome that you're tackling this!  This is a particularly tricking area of the code in that the side effects of such changes can be subtle and wide-reaching.  Feel free to ping me (bradee-oh) on IRC if you have any questions about my comments.

> Source/WebCore/loader/SubresourceLoader.cpp:213
> +
> +    checkForHTTPStatusCodeError();

An interesting side effect of doing the check here is that we'll now be canceling all subresource loads the moment their response comes back as a 404.  This includes loads that would include actual data as well.

This might cause problems for any subresource that expects textual data and might not care that the response was a 404.

For example, how about an XMLHTTPRequest that returns a 404 but also returns data?  This is just theoretical - I'm not even sure it's currently possible - but we need to vet changes at this low of a level for this reason since they are so wide reaching.

A reasonable alternative might be to do the same check in didFinishLoading/didFailWithError so we don't cancel it before the data comes in.

> Source/WebCore/loader/SubresourceLoader.h:75
> -    bool errorLoadingResource();
> +    bool checkForHTTPStatusCodeError();

I'm not sure if this is quite the right rename.  It is true that errorLoadingResource() nominally checks the the response status, but it might also make other calculations and decisions.
Plus, it does more than simply check for the error!

> LayoutTests/http/tests/loading/resources/404-with-empty-body.cgi:4
> +print "Content-type: text/javascript\n\n";

Is the content type actually relevant here?

I would wager many servers would send back a 404 with a 0-byte resource and *not* include the Content-type header.  We should double check that against Firefox and include a test for it with this change.
Comment 5 Brady Eidson 2012-06-06 13:42:22 PDT
Alexey and I discussed this in person.

He clarified that the "m_resource->shouldIgnoreHTTPStatusCodeErrors()" clause inside the current SubresourceLoader::errorLoadingResource() is exactly what prevents my first major concern about regressing behavior.

Great!

We then had a discussion about method naming that came to no conclusions, other than I agreed that the proposed name "checkForHTTPStatusCodeError" is more symmetric with "shouldIgnoreHTTPStatusCodeErrors" and therefore is an improvement.

Great!

I would still prefer a second test be added without the explicit Content-type which will make it more likely that we'd catch changes in behavior - intentional or otherwise - in future changes.
Comment 6 Edaena Salinas 2012-06-06 15:46:47 PDT
Created attachment 146133 [details]
Added Test

Added a test for a script with no content-type.
Comment 7 WebKit Review Bot 2012-06-07 00:20:44 PDT
Comment on attachment 146133 [details]
Added Test

Rejecting attachment 146133 [details] from commit-queue.

New failing tests:
http/tests/misc/link-rel-prefetch-and-subresource.html
Full output: http://queues.webkit.org/results/12921078
Comment 8 WebKit Review Bot 2012-06-07 00:20:48 PDT
Created attachment 146218 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 9 WebKit Review Bot 2012-06-07 00:34:54 PDT
Comment on attachment 146133 [details]
Added Test

Attachment 146133 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12911176

New failing tests:
http/tests/misc/link-rel-prefetch-and-subresource.html
Comment 10 WebKit Review Bot 2012-06-07 00:34:58 PDT
Created attachment 146220 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 11 Brady Eidson 2012-06-07 09:44:05 PDT
Based on the regressions, seems like something was missed.
Comment 12 Brady Eidson 2012-06-07 09:45:27 PDT
(In reply to comment #11)
> Based on the regressions, seems like something was missed.

(Though it's not clear to me what)
Comment 13 Edaena Salinas 2012-06-07 11:08:36 PDT
Created attachment 146330 [details]
Modified test
Comment 14 Darin Adler 2012-06-07 11:12:03 PDT
Comment on attachment 146330 [details]
Modified test

This patch is confusing. It cites a bug, but the patch adds a test and does not include a bug fix.
Comment 15 Edaena Salinas 2012-06-07 11:53:03 PDT
Created attachment 146346 [details]
Modified test
Comment 16 Build Bot 2012-06-07 12:12:26 PDT
Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12922237
Comment 17 Gustavo Noronha (kov) 2012-06-07 12:29:13 PDT
Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12907714
Comment 18 Early Warning System Bot 2012-06-07 12:51:53 PDT
Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12917273
Comment 19 Alexey Proskuryakov 2012-06-07 13:05:34 PDT
Comment on attachment 146346 [details]
Modified test

Please remove HTMLButtonElement related changes.
Comment 20 Edaena Salinas 2012-06-07 13:18:14 PDT
Created attachment 146366 [details]
Modified test
Comment 21 WebKit Review Bot 2012-06-07 15:09:14 PDT
Comment on attachment 146366 [details]
Modified test

Clearing flags on attachment: 146366

Committed r119759: <http://trac.webkit.org/changeset/119759>
Comment 22 WebKit Review Bot 2012-06-07 15:09:24 PDT
All reviewed patches have been landed.  Closing bug.