Closed Bug 228087 Opened 21 years ago Closed 21 years ago

bug with braces in JavaScript regular expressions

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.6final

People

(Reporter: bex, Assigned: brendan)

References

(
URL
)

Details

(Keywords: js1.5, Whiteboard: [ have filed bug 228336 against Rhino on this same issue ])

Attachments

(2 files, 2 obsolete files)

proposed fix, v2
5.39 KB, patch
Details | Diff | Splinter Review
diff -w version of last patch
3.70 KB, patch
igor
: review+
shaver
: superreview+
Details | Diff | Splinter Review 
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.
Isn't there an existing bug on this?  Wish we had a new email address for Phil.

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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.
Whiteboard: [ have filed bug 228336 against Rhino on this same issue ]
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.
Argh.

/be
Assignee: general → brendan
Flags: blocking1.6+
Keywords: js1.5
Priority: -- → P1
Target Milestone: --- → mozilla1.6final
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
Attachment #137867 - Flags: review?(shaver)
Comment on attachment 137867 [details] [diff] [review]
proposed fix
[Checked in: Comment 9]

r=shaver
Attachment #137867 - Flags: review?(shaver) → review+
Comment on attachment 137867 [details] [diff] [review]
proposed fix
[Checked in: Comment 9]

Looking for 1.6 branch approval.

/be
Attachment #137867 - Flags: approval1.6?
Attachment #137867 - Flags: approval1.6? → approval1.6+
Fixed in trunk and 1.6 branch.

/be
Status: NEW → RESOLVED
Closed: 21 years ago
Flags: blocking1.6+
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch untested patch (obsolete) — — Splinter Review 
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 on attachment 138712 [details] [diff] [review]
untested patch

Going for both testing/reviewing buddies on this one.

/be
Attachment #138712 - Flags: superreview?(shaver)
Attachment #138712 - Flags: review?(igor)
Attachment #137867 - Attachment description: proposed fix → proposed fix [Checked in: Comment 9]
Attachment #137867 - Attachment is obsolete: true
Flags: blocking1.6+
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.
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
monday morning builds will be the candidates so lets tet this in if its good.
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
Attachment #138712 - Attachment is obsolete: true
Attachment #138712 - Flags: superreview?(shaver)
Attachment #138712 - Flags: review?(igor)
Attached patch proposed fix, v2 — — Splinter Review 
Apply this, diff -w next for review.

/be
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 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
Attachment #138844 - Flags: superreview?(shaver)
Attachment #138844 - Flags: review?(igor)
we are shooting to use the build from tomorrow morning as the 1.6 release builds.
Attachment #138844 - Flags: superreview?(shaver) → superreview+
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
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
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.
Attachment #138844 - Flags: review?(igor) → review+
Flags: testcase+
fixed by bug 289628
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: