81308 - Chrome scripts silently fail to load with when they contain syntax errors

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Blake Ross]

Scripts referenced from chrome are failing to load.  I saw this in one case and 
so did Boris, and Doron saw it another.  This is blocking a lot of work.  
Trying to narrow it down now.

Comment 1

[Blake Ross]

To clarify, it's just some scripts.  We haven't figured out the pattern yet.

Comment 2

[Doron Rosenberg (IBM)]

here is what I see (cvs pull from yesterday):

I load 2 .js (navigator.js and viewsource.js) files in a .xul. However, only the
first one seems to be actually loaded, as calling functions from the 2nd one did
nothing. Changing the order had the effect that only functions in the first one
were accesible.

bz was unable to see this with the same .xul, which leads me to think that my
hacked viewsource.js is at fault. Working on a simplified testcase. 

Comment 3

[Brendan Eich [:brendan]]

Just to rule out bug 74883, now fixed, could you guys who have builds update
js/src/jsdhash.c and xpcom/ds/pldhash.c, and rebuild incrementally (if you have
good dependencies)?

/be

Comment 4

[Brendan Eich [:brendan]]

BTW, I'd expect crashes from 74883, not this -- so I'm pretty sure it won't help
but I wanted to mention it.

/be

Comment 5

[Chris McAfee]

brendan/hyatt don't see this.  go clobber, marking critical
to get off blocker list.

Comment 6

[J.J. Enser]

Blake, did you see this with today's release build?
(2001-05-16-10-trunk or 2001-05-16-12-trunk)

Comment 7

[Boris Zbarsky [:bzbarsky]]

I've narrowed it down to a syntax error in the script that blake was seeing it
on (missing comma between items in the prototype section).  So what I'm seeing
is this:

If there's a syntax error in a script, the script fails to load, no code in it
is executed (I had an alert() at the very beginning) and there is nothing in the
JS console to indicate what's wrong.

Comment 8

[Boris Zbarsky [:bzbarsky]]

oops.  setting back to critical.

Comment 9

[Blake Ross]

Why would "clobber and try again" be a likely solution when three different 
people are seeing the problem?  I think this should be a blocker again because 
it's a major problem and is probably causing many other bugs.

Comment 10

[Blake Ross]

Upgrading this to blocker again because this is causing or probably will cause 
other bugs that we shouldn't waste time tracking down.   Aside from the fact 
that js errors are no longer output to the console, the js errors in the 
problematic file(s) aren't even reported by the jseng, nor is there any 
indication that they are failing to load.  So there's no easy way to tell when 
this bug is biting you.

Comment 11

[Blake Ross]

Ok, talked to the sheriff, we'll re-evaluate this before tree opening tomorrow 
in light of verifications.

Comment 12

[Johnny Stenback (:jst)]

I'm sorry, but this is not a bug, this is just the way script loading works, and
always has worked, and always will work. The same issue exists when webpages
load script files, if the file contains *syntax* errors, the file is not loaded
and *no* part of the script is executed, this is how all browsers work, AFAIK
(verified in Mozilla and IE).

INVALID.

Comment 13

[Johnny Stenback (:jst)]

er, no, not fixed.

Comment 14

[Johnny Stenback (:jst)]

INVALID.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Johnny, the issue here is not that the scripts fail to load.  That's fine.  The 
problem is that the JS console contains no indication of the failure (for web 
content it shows the syntax error and the line the error occured on, for chrome 
it does not).

This leads to mysterious failures in loading chrome scripts and makes such 
failures very hard too debug.  Some error reporting would be immensely helpful.

Reopening and adjusting summary to reflect the real problem.

Comment 16

[Simon Fraser [no longer active]]

This is killing me right now...

Comment 17

[Simon Fraser [no longer active]]

I'm seeing silent failure on script compilation; there is no error reporter set 
on script context that the nsXULDocument is using.

Comment 18

[Simon Fraser [no longer active]]

The XPCDOM landing broke this.

nsJSContext::InitContext() is bailing at the if (!aGlobalObject) clause, so we 
never get down to the code that sets the error reporter.

Comment 19

[Simon Fraser [no longer active]]

Patch:

Index: mozilla/dom/src/base/nsJSEnvironment.cpp
===================================================================
RCS file: /cvsroot/mozilla/dom/src/base/nsJSEnvironment.cpp,v
retrieving revision 1.137
diff -b -u -2 -r1.137 nsJSEnvironment.cpp
--- nsJSEnvironment.cpp	2001/05/15 22:26:18	1.137
+++ nsJSEnvironment.cpp	2001/05/18 02:29:01
@@ -1031,4 +1031,6 @@
   }
 
+  ::JS_SetErrorReporter(mContext, NS_ScriptErrorReporter);
+
   if (!aGlobalObject) {
     // If we don't get a global object then there's nothing more to do here.
@@ -1065,6 +1067,4 @@
   rv = InitClasses(); // this will complete global object initialization
   NS_ENSURE_SUCCESS(rv, rv);
-
-  ::JS_SetErrorReporter(mContext, NS_ScriptErrorReporter);
 
   mIsInitialized = PR_TRUE;

This allows me to find JS errors again. Not sure if it's totally correct (there 
is some other stuff at the bottom of Init() that looks like it should happen if 
aGlobalObject is NULL.

Comment 20

[Chris McAfee]

this bug just bit me, I've changed my mind, this is now a blocker.

Comment 21

[Johnny Stenback (:jst)]

Thanks for the patch, Simon, and I'm sorry for not realizing that this really
was a problem earlier, fix checked in (and yes, the fix is good).

Comment 22

[Sivakiran Tummala]

marking verified based on Johnny's comments.