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. :)
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.
I am actually seeing this bug with ToT. Got a fix.
Created attachment 146091 [details] Proposed fix.
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.
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.
Created attachment 146133 [details] Added Test Added a test for a script with no content-type.
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
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 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
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
Based on the regressions, seems like something was missed.
(In reply to comment #11) > Based on the regressions, seems like something was missed. (Though it's not clear to me what)
Created attachment 146330 [details] Modified test
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.
Created attachment 146346 [details] Modified test
Comment on attachment 146346 [details] Modified test Attachment 146346 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12922237
Comment on attachment 146346 [details] Modified test Attachment 146346 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12907714
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 on attachment 146346 [details] Modified test Please remove HTMLButtonElement related changes.
Created attachment 146366 [details] Modified test
Comment on attachment 146366 [details] Modified test Clearing flags on attachment: 146366 Committed r119759: <http://trac.webkit.org/changeset/119759>
All reviewed patches have been landed. Closing bug.