Bug 50589 – Should fire error event for empty 404 script

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Ojan Vafai]

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. :)

[Ojan Vafai]

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.

[Edaena Salinas]

I am actually seeing this bug with ToT. Got a fix.

[Edaena Salinas]

Created attachment 146091 [details]
Proposed fix.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Edaena Salinas]

Created attachment 146133 [details]
Added Test

Added a test for a script with no content-type.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Brady Eidson]

Based on the regressions, seems like something was missed.

[Brady Eidson]

(In reply to comment #11)
> Based on the regressions, seems like something was missed.

(Though it's not clear to me what)

[Edaena Salinas]

Created attachment 146330 [details]
Modified test

[Darin Adler]

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.

[Edaena Salinas]

Created attachment 146346 [details]
Modified test

[Build Bot]

Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12922237

[Gustavo Noronha (kov)]

Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12907714

[Early Warning System Bot]

Comment on attachment 146346 [details]
Modified test

Attachment 146346 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12917273

[Alexey Proskuryakov]

Comment on attachment 146346 [details]
Modified test

Please remove HTMLButtonElement related changes.

[Edaena Salinas]

Created attachment 146366 [details]
Modified test

[WebKit Review Bot]

Comment on attachment 146366 [details]
Modified test

Clearing flags on attachment: 146366

Committed r119759: <http://trac.webkit.org/changeset/119759>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.