97646 - Exit code should be non-0 after compiler errors

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Phil Schwartau]

I have two testcases which cause "internal errors" in the JS shell.
Yet both testcases terminate with exit code 0; therefore the test driver
does not know there has been an error. The exit code should be non-0.

As Brendan said in bug 89443,

"...any non-warning error detected by the compiler should cause an abnormal
termination of the shell."

Comment 1

[Phil Schwartau]

Testcases added to JS test suite:

       mozilla/js/tests/js1_5/Regress/regress-97646-001-n.js
       mozilla/js/tests/js1_5/Regress/regress-97646-002-n.js

The '-n' in the name means these testcases will report failure
if they terminate with exit code 0. They expect exit code 3.

If I load them in the JS shell, I see the following "InternalErrors":


[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js
js> load('../../tests/js1_5/shell.js') <--- (UTILITY FUNCTIONS)

js> load('../../tests/js1_5/Regress/regress-97646-001-n.js')
InternalError: else statement too large

js> load('../../tests/js1_5/Regress/regress-97646-002-n.js')
InternalError: block too large

Comment 2

[Phil Schwartau]

NOTE: if '3' is not the correct exit code to expect, let me know and 
I will adjust the testcases accordingly...

Comment 3

[Brendan Eich [:brendan]]

Attachment: proposed fix (the JS_CompileFile* API entry points were failing to js_ReportUncaughtException)

Comment 4

[Brendan Eich [:brendan]]

Taking, please r= and sr= soon -- this is easy, it won't take more than a minute
to review.

/be

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 50637 [details] [diff] [review]
proposed fix (the JS_CompileFile* API entry points were failing to js_ReportUncaughtException)

sr=shaver

Comment 6

[John Bandhauer]

Comment on attachment 50637 [details] [diff] [review]
proposed fix (the JS_CompileFile* API entry points were failing to js_ReportUncaughtException)

r=jband.

The change looks fine. But does it always fix the problem? It seems like
the Process function in js.c might need some love. no?

Comment 7

[Brendan Eich [:brendan]]

jband: Process itself should not need to report uncaught exceptions, because non
should go unreported by a JS_Compile/Execute/Evaluate/Call API -- but oops, the
JS_Compile*Script* APIs fail to report.  Fixed, thanks.  New patch, which I'll
check in momentarily, coming up.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: NB: CompileTokenStream can't report uncaught, because of the call to it from JS_BufferIsCompileableUnit

Comment 9

[Brendan Eich [:brendan]]

Fix checked in.

/be

Comment 10

[Brendan Eich [:brendan]]

Oops.  eval is busted by reporting uncaught exceptions from
JS_CompileUCScriptForPrincipals.  My bad -- patch coming up fast!

/be

Comment 11

[Brendan Eich [:brendan]]

This report-uncaught-exceptions is pretty tricky.  I can imagine other native
methods than eval that might want to throw an exception (e.g., SyntaxError) and
not have JS_Compile* or JS_Evaluate* call the API-configured error reporter.
What about setTimeout and setInterval?  Cc'ing jst.

In the mean time, my patch fixes things so eval and other "special" (debugger,
too) calls into the compiler do not report uncaught exceptions on the way out of
JS_Compile*.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: one-line fix to suppress reporting uncaught exceptions from eval

Comment 13

[Brendan Eich [:brendan]]

Attachment: better fix: don't report uncaught exceptions unless unwinding to native code (!cx->fp in API layer)

Comment 14

[Brendan Eich [:brendan]]

Ok, please r= and sr= and double-sr= (jband this means you).  Rather than
special case eval (and debugger analogues), suppress reporting uncaught
exceptions unless the API entry point is called from apparently-native code
(!cx->fp).  If that native caller is itself part of some cloud of native
functions called from a JS native, it's up to the outer JS native to "unreport"
the exception and make it seem to be thrown still.

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51031 [details] [diff] [review]
better fix: don't report uncaught exceptions unless unwinding to native code (!cx->fp in API layer)

Now this I like.  Simple, easy to understand, consistent.  sr=shaver, with a bullet.

Comment 16

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51031 [details] [diff] [review]
better fix: don't report uncaught exceptions unless unwinding to native code (!cx->fp in API layer)

Now this I like.  Simple, easy to understand, consistent.  sr=shaver, with a bullet.

Comment 17

[John Bandhauer]

Comment on attachment 51031 [details] [diff] [review]
better fix: don't report uncaught exceptions unless unwinding to native code (!cx->fp in API layer)

r=jband.
Sure, I'll take a card. hat's the worst that could happen?
Looks like shaver already double sr'd it :)

Comment 18

[John Bandhauer]

s/hat's/What is/

Comment 19

[Brendan Eich [:brendan]]

jband, you're the big winner!  (Always double-down on 11.)

Phil, sorry about that.  Your testsuite output should be back to 11 failures --
please confirm.

/be

Comment 20

[Phil Schwartau]

Verified Fixed in debug and optimized JS shells on WinNT, Linux.
The testcases 

           js/tests/js1_5/Regress/regress-97646-001-n.js                                                                                          
           js/tests/js1_5/Regress/regress-97646-002-n.js

both pass, since they get the exit code 3 they are expecting.
Additionally, this testcase now errors, because we are now getting
exit code 3 from its too-large script:

           js/tests/js1_5/Regress/regress-90445.js


This is all correct behavior with the current jump bytecode limitation. 

The first two testcases will have to be modified or deprecated, however,
once the fix for bug 80981 is checked in. At that point the JS Engine
will no longer produce an InternalError on large scripts, thus there
will no longer be a reason for these tests to expect exit code 3.
Additionally, the last test should no longer produce exit code 3.

Comment 21

[Phil Schwartau]

As promised above, I have CVS-removed

           js/tests/js1_5/Regress/regress-97646-001-n.js
           js/tests/js1_5/Regress/regress-97646-002-n.js


Both tests contain very large blocks. But because of the fix to bug 80981,
these are no longer large enough to trigger the internal error and exit code 3 
that the tests looked for.