139040 - [FIX]JavaScript interpreter tries to parse 404 messages

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[kirun]

When a page has a broken link to a javascript file, the web server will send a
404 page back when it is requested. This should result in an error message on
the console of "foo.js not found". However, what actually happens is the 404
page is treated as a javascript file. This means the error shown on the console
is usually a syntax error when the page's DTD is interpreted.

Although this isn't going to stop any scripts from working (as the file wasn't
there anyway), it would make debugging easier if the friendlier error was given.

Comment 1

[Kai Lahmann (is there, where MNG is)]

can you give a test-URL?

Comment 2

[Kai Lahmann (is there, where MNG is)]

ah.. got (private) test URL and confirmed on Linux, 1.0RC1. It just tries to
parse the first line of the 404-file (with a non existant JS file linked)

..can someone set OS to all?

Comment 3

[Boris Zbarsky [:bzbarsky]]

We _could_ just take the channel in nsScriptLoader::OnStreamComplete, QI to
nsIHttpChannel, and look at the status code.... Darin, I seem to recall you and
Bradley talking about a possible "status" attr on nsIChannel2 or something along
those lines. Is that happening?  Or should this code just special-case
nsIHttpChannel?

jst?  do we event want to do this?  Are there valid use cases for a site to
serve up an application/x-javascript 404 page (eg. in response to a request that
has an Accept: header that asks for JS types only)....

Comment 4

[Phil Schwartau]

I'm not sure which component this is for, but it's not JS Engine.
Reassigning to Networking as a guess - please reassign as nec.; thx -

Comment 5

[benc]

I think the page info would help here.

Comment 6

[Darin Fisher]

bz: there aren't any real plans for a uber-status attribute that would indicate
failure when a 404 page is sent, and there definitely aren't any plans for
nsIChannel2 ... i hope!! ;-)

special casing nsIHttpChannel seems like the right solution to me.

care to own this bug?

Comment 7

[Boris Zbarsky [:bzbarsky]]

If we're willing to wait till mid-June for a fix, then yes.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.0

Comment 9

[Boris Zbarsky [:bzbarsky]]

mine

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 88104 [details] [diff] [review]
Patch v 1.0

r=bbaetz, but get a dom-ish person to look at it too.

We really do need an uber-attribute.

Maybe we can add another success value for aStutus? It wouldn't affect anyone
using NS_SUCCEEDED, but people could explicitly check for a document which is
'abnormal', or something?

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 88104 [details] [diff] [review]
Patch v 1.0

>+    // Only accept 2xx responses
>+    if (NS_SUCCEEDED(rv) && statusCode / 100 != 2) {

I don't like this http specific statusCode / 100 != 2 stuff in here, seems like
we should hidden in a macro in nsIHttp* and the name of the macro should
reflect what this really means (which I don't even know, what do 2xx http
status' mean?).

Let me know what others think about that and I'll sr once we agree...

Comment 12

[Darin Fisher]

jst: agreed... we talked about doing this before, but just didn't finalize
anything.  i suppose now is as good a time as any given that a similar patch
just went in to fix a similar bug in the CSS parser.

bbaetz: yeah, a successful nsresult code might just do the trick.  any
suggestions on what we might call it? 
NS_BINDING_SUCCEEDED_WITH_UNEXPECTED_CONTENT?  too long of course, but i can't
really think of anything else.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So.  Do we just want a method on nsIHttpChannel that will return |true| if the
status is 2xx? 

Comment 14

[Johnny Stenback (:jst)]

Wouldn't macros with meaningful names that do the status code checking be enough
here?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Sure; just a matter of whether we want it to be available to script as well...

Comment 16

[Darin Fisher]

long term, i think we want some sort of solution that will work for non-http
protocols as well.  but, for now... it probably is sufficient to either add a
method to nsIHttpChannel or add some macros to nsIHttpChannel.idl.  a method
sounds best for the reason bz mentioned.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch v 1.5

This should be better.....

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 89679 [details] [diff] [review]
Patch v 1.5

r=jst, rpotts, could you sr?

Comment 19

[rpotts (gone)]

Comment on attachment 89679 [details] [diff] [review]
Patch v 1.5

sr=[e-mail]

Comment 20

[Boris Zbarsky [:bzbarsky]]

checked in on the trunk

Comment 21

[benc]

-> HTTP

Darin: what happens w/ non-http protocols? Also, to clarify, since you check
positively for 2xx codes, that also means ugly 500 codes from proxy servers
would be handled better as well, right?

Comment 22

[Darin Fisher]

> Darin: what happens w/ non-http protocols? Also, to clarify, since you check
> positively for 2xx codes, that also means ugly 500 codes from proxy servers
> would be handled better as well, right?

bz's patch only effects HTTP channels, so nothing changes with non-http
protocols.  this patch didn't effect the way 5xx error codes are handled.

Comment 23

[Jesse Ruderman]

See also bug 320881, "External scripts with 400 status code fail to execute".