350268 - "}" in "new Function" should cause syntax error instead of making the rest be ignored

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jesse Ruderman]

> new Function("}")
function anonymous() { }

> new Function("}}}}}")
function anonymous() { }

> new Function("alert(6); } alert(5);")
function anonymous() { alert(6); }

> new Function("} {")
function anonymous() { }

I think these should all trigger syntax errors.

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

The old "forgot to check for unconsumed tokens" gotcha.

/be

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 235536 [details] [diff] [review]
fix

Simple and safe fix, which matches the same error case for top-level scripts.  I didn't attach a diff -w, but you can see that the main change is really only four lines of added error checking and reporting, and the rest is due to existing code being indented in an else block.

/be

Comment 3

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 4

[Boris Zbarsky [:bzbarsky]]

I'm a little worried about checking this in on the 1.8 branch; if there's Mozilla/Netscape-only content out there this could bite it badly, and we sorta promised Gecko compat between 1.8 and 1.8.1.  As an example, this broke our Tinderbox startup test, because the onload handler had a trailing '}'.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 235536 [details] [diff] [review]
fix

We can not worry and leave this in the trunk for 1.9 only, for sure.

As bz's comment implied, and as gavin pointed out, IE silently fails on trailing junk, so cross-platform web content won't care.

/be

Comment 6

[Bob Clary [:bc] (inactive)]

Checking in regress-350268.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350268.js,v  <--  regress-350268.js
initial revision: 1.1

Comment 7

[Jesse Ruderman]

See also bug 350429, trailing "}" fails to log syntax error in some cases.

Comment 8

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060830 windows/mac*/linux