[webkit-reviews] review denied: [Bug 50589] Should fire error event for empty 404 script : [Attachment 146091] Proposed fix.

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Edaena Salinas <edaena at gmail.com>'s
request for review:
Bug 50589: Should fire error event for empty 404 script
https://bugs.webkit.org/show_bug.cgi?id=50589

Attachment 146091: Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=146091&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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.