120197 - Certain syntax errors are being reported twice

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Schwartau]

This grew out of bug 120157. In the following code, {} represents a
block statement, and not an object literal. That leads to a syntax error.
However, the error is reported twice:

[//d/JS_TRUNK/mozilla/js/src/WINNT4.0_DBG.OBJ] ./js -s -w

js> {}.a
4: syntax error:
4: {}.a
4: ..^
4: SyntaxError: syntax error:
4: {}.a
4: ..^


In case this helps, here are some other syntax errors:

js> var obj = {a;b}
1: missing : after property id:
1: var obj = {a;b}
1: ............^
1: SyntaxError: missing : after property id:
1: var obj = {a;b}
1: ............^


js> var obj = {];
4: invalid property id:
4: var obj = {];
4: ...........^
4: SyntaxError: invalid property id:
4: var obj = {];
4: ...........^

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix, i think this is right, but it needs more testing

rogerl, could you please review?  The problem seems to me to be
js_ReportUncaughtException's setting of JSREPORT_EXCEPTION when it is
*reporting an error* (not throwing an exception), coupled with the failure of
the js shell's error reporter to follow the (confusingly written) comment in
jsscan.c around line 540 --
http://lxr.mozilla.org/mozilla/source/js/src/jsscan.c#540.

Phil, can you apply and test the heck out of this?  Thanks.

/be

Comment 2

[rogerl (gone)]

The comment in jsexn.h for js_ReportUncaughtException states that the error
report will have the JSREPORT_EXCEPTION flag set, but now that won't be so.

Comment 3

[Phil Schwartau]

The current patch, when applied on WinNT, causes two regressions:

*-* Testcase ecma_2/Exceptions/exception-010-n.js failed:
Bug Number 21799
Expected exit code 3, got 0
Testcase terminated with signal 0
Complete testcase output was:
Null throw test.
BUGNUMBER: 21799


*-* Testcase ecma_2/Exceptions/exception-011-n.js failed:
Expected exit code 3, got 0
Testcase terminated with signal 0
Complete testcase output was:
Undefined throw test.


These are both negative tests; they expect exit code 3 in order to pass.
With the patch, they're exiting with code 0. They do things like this:

test1();
test2();
 
function test1()
{
    print ("Null throw test.");
    print ("BUGNUMBER: 21799");  
    throw null;
    print ("FAILED!: Should have exited with uncaught exception.");
}

function test2()
{
    print ("Undefined throw test.");   
    throw (void 0);
    print ("FAILED!: Should have exited with uncaught exception.");
}

Comment 4

[Brendan Eich [:brendan]]

rogerl: I haven't studied the code further, but I did see two error reports,
both with that bit set.  Hence the double report in the shell (or no reports at
all, if you suppress reports with that bit set).  How should this stuff work, do
you recall?

/be

Comment 5

[rogerl (gone)]

Frankly I'm confused. I really don't understand the comment in jsscan.c - I'm
not sure if it's recommending that the error reporter ignore the error/exception
or just the flag setting.
However, the ReportError mechanism in the rest of the engine only invokes the
error reporter if the error is not an exception. I'm not sure why that doesn't
apply here also? If js_ReportCompileErrorNumber always returns an exception
(when it can, as in these examples) the mechanism in place for reporting
uncaught exceptions should pick that up. We already prevent follow-on errors
from overriding the initial error exception, so it's not necessary to force the
error report immediately, is it?

I'm attaching this solution as a tentative patch - it handles the bugs reported
here as well as the test suite. [If this seems like the right thing to do, I
vote we also remove the confusing comment]

Comment 6

[rogerl (gone)]

Attachment: Another proposed fix...

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 65154 [details] [diff] [review]
proposed fix, i think this is right, but it needs more testing

Thanks to rogerl for pointing out the right way.

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 66487 [details] [diff] [review]
Another proposed fix...

Seems to me the comment (and the my_LoadErrorReporter wrapper for
my_ErrorReporter in js.c) say that JSREPORT_EXCEPTION means "Uncaught exception
is being reported as an error".  The comment's use of "probably" when
addressing "Proper behavior" is unsettling, to say the least!  How about
revising that to say "if handling an error from a native method that runs the
compiler, you'll want to ignore uncaught exception error reports, letting the
native method propagate failure along with the exception (which will not be
cleared, but will remain pending)."  Or something better -- I can't write well
in this tiny textarea!

sr=[e-mail] with a better comment.  Thanks again,

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: rogerl's patch with my attempt at comment cleanup

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 66648 [details] [diff] [review]
rogerl's patch with my attempt at comment cleanup

Hoping shaver will sr=.

r=[e-mail], climbing down from my sr= on rogerl's patch cuz I wrote
the comment here -- am I being too scrupulous?

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 66648 [details] [diff] [review]
rogerl's patch with my attempt at comment cleanup

I am enlightened! sr=shaver

Comment 12

[Mike Shaver (:shaver emeritus)]

Comment on attachment 66648 [details] [diff] [review]
rogerl's patch with my attempt at comment cleanup

Patch manager, you need to learn about collisions!

Comment 13

[Brendan Eich [:brendan]]

Fix checked in -- thanks.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed, I say!

/be

Comment 15

[Phil Schwartau]

Verified FIXED in debug/optimized JS shell on WinNT, Linux, Mac9.1