228087 - bug with braces in JavaScript regular expressions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[bex]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031208
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6b) Gecko/20031208

the link is a sample page. Basicly, something changed from Mozilla 1.5 to 1.6
about the way braces '{}' are handled inside regular expressions.  Escaping them
will force it to work, but it shouldn't be necessarry. I know JavaScript handles
braces in a special way, but does the RegEx engine?

Reproducible: Always

Steps to Reproduce:
1. go to the web page
2. click the 'run test' button
3. view the error in the JavaScript console

Actual Results:  
after clicking 'run test', it only pops up 2 alert boxes. The expressions were
not parsed properly.

Expected Results:  
it should pop up 4 alert boxes.

Comment 1

[Brendan Eich [:brendan]]

Isn't there an existing bug on this?  Wish we had a new email address for Phil.

/be

Comment 2

[Phil Schwartau]

It looks like the RegExp rewrite in bug 85721 has changed Mozilla's
behavior on the regexps bex has provided here. Compare:


------------------- RC5a release of SpiderMonkey ------------------- 
js> re = /{1.*}/g;
/{1.*}/g

js> re = /{1[.!}]*}/g;
/{1[.!}]*}/g


-------------------- Current SpiderMonkey shell --------------------
js> re = /{1.*}/g;
25: SyntaxError: invalid quantifier {:
25: re = /{1.*}/g;
25: .....^

js> re = /{1[.!}]*}/g;
29: SyntaxError: invalid quantifier {:
29: re = /{1[.!}]*}/g;
29: .....^


Note that these regexps are illegal per ECMA-262 Edition 3,
which prohibits the use of unescaped braces in regexp patterns
unless they form part of a quantifier.

However, as part of the RegExp rewrite, we decided to follow Perl
and IE on this, and allow unescaped braces even if they are not
part of a quantifier.

See bug 188206 comment 12; also bug 223273 comment 29 and following.

It looks like the RegExp parser is mistakenly identifying bex's
use of braces as quantifiers (hence the SyntaxErrors), instead of
the non-quantifier, literal use of unescaped braces that we permit.

Comment 3

[Phil Schwartau]

Testcase added to JS testsuite:

      mozilla/js/tests/ecma_3/RegExp/regress-228087.js

Currently failing in both the SpiderMonkey and Rhino shell.
I have filed bug 228336 against Rhino on this issue.

Again - this test passes in the RC5a release of SpiderMonkey,
so the current bug is a regression from previous behavior.

Comment 4

[Phil Schwartau]

Note: both testcases for bug 188206 and bug 223273 contained tests for
literal use of unescaped braces, but they were not exhaustive enough -
otherwise this bug would have been caught sooner.

Comment 5

[Brendan Eich [:brendan]]

Argh.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix [Checked in: Comment 9]

Trivial, and I'm using #if 0 for now, but eventually the code will go away,
once someone figures out why it was added.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 137867 [details] [diff] [review]
proposed fix
[Checked in: Comment 9]

r=shaver

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 137867 [details] [diff] [review]
proposed fix
[Checked in: Comment 9]

Looking for 1.6 branch approval.

/be

Comment 9

[Brendan Eich [:brendan]]

Fixed in trunk and 1.6 branch.

/be

Comment 10

[Igor Bukanov]

The fix does not address all test cases in ecma_3/RegExp/regress-228087.js, in
particular, SM still fails with patterns like /c{3 }/, session 6 of the test.

As far as I can see in Perl anything after { that does not match

\d+(,(\d*)?)?\} leads to treating of { as literal symbol.

Comment 11

[Brendan Eich [:brendan]]

Attachment: untested patch

Someone please test for me so this can get into 1.6.  I'll try to get time to
test it later today, but no promises.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 138712 [details] [diff] [review]
untested patch

Going for both testing/reviewing buddies on this one.

/be

Comment 13

[Robert Accettura [:raccettura]]

Will this latest patch make it in time for 1.6f?  

Asa states:
> Today we took (hopefully) the last of the 1.6 changes so I'll be testing the 
> builds over the weekend, working on release documentation, and looking forward 
> to a Mozilla 1.6 early next week.
http://weblogs.mozillazine.org/asa/archives/004655.html

This also seems like a somewhat substantial bug.

Comment 14

[Brendan Eich [:brendan]]

Got time to test this, it passes all the new js/tests tests, and regresses the
now-bogus test ecma_3/RegExp/regress-188206.js as expected (that test wants
brutal SyntaxError exceptions for "Invalid use of regexp quantifiers", contrary
to this bug and backward compatibility.

/be

Comment 15

[chris hofmann]

monday morning builds will be the candidates so lets tet this in if its good.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 138712 [details] [diff] [review]
untested patch

Oops, missed a core file in js/tests -- why doesn't the testsuite driver flag
core dumps as failures?

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Apply this, diff -w next for review.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

The essential change is to put all the {min,max} quantifier parsing code inside
the if (JS7_ISDEC(c)) that notices {min (where min is a decimal literal). 
Then, only if we find the closing } right after either min, or the comma after
min, or min,max do we create a REOP_QUANT node.  The old code created the node
early, and the previous fix-attempt patch here left that bogo-node around when
"failing" to parse a quantifier, trying to fall back so the braced mess parsed
as a flat string to match....

/be

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 138844 [details] [diff] [review]
diff -w version of last patch

This does, indeed, pass the testsuite with the one new expected regression (the
test that wants a SyntaxError thrown for "abused" braces in regexps).

Hope it's not too late for 1.6.

/be

Comment 20

[chris hofmann]

we are shooting to use the build from tomorrow morning as the 1.6 release builds.

Comment 21

[Brendan Eich [:brendan]]

Fixed on trunk and in the 1.6 branch.

Igor, feel free to comment and stamp r+ or whatever.  I'm closing this bug,
though, since the fix looks good according to code review and testing.

/be

Comment 22

[Igor Bukanov]

Comment on attachment 138844 [details] [diff] [review]
diff -w version of last patch

Review is OK, I will apply the similar treatment to Rhino. Sorry for delay with
it: vacation etc.

Comment 23

[Bob Clary [:bc] (inactive)]

fixed by bug 289628