Native regexps: case-insensitive comparison bug

VERIFIED FIXED

Status

()

VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: dmandelin, Assigned: dmandelin)

Tracking

({verified1.9.1})

Trunk
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

(Assignee)

Description

10 years ago
Created attachment 349092 [details] [diff] [review]
Patch

Due to a 1-line typo, native matchers generated with the 'i' flag perform a bogus "to-uppercase" action on non-letters. The result is that in the following code with the -j option turned on, all 4 match operations match.

print("must match:");
print('@'.match(/@/i));
print('`'.match(/`/i));

print("must not match:");
print('@'.match(/`/i));
print('`'.match(/@/i));
Attachment #349092 - Flags: review?(gal)
Comment on attachment 349092 [details] [diff] [review]
Patch

>diff -r d5e92772e8a1 js/src/jsregexp.cpp
>+            if (L'A' <= ch && ch <= L'Z' || L'a' <= ch && ch <= L'z') {

Drive-by: Please parenthesize the && expressions against the || to avoid GCC warnings.

Updated

10 years ago
Flags: blocking1.9.1?

Comment 2

10 years ago
Comment on attachment 349092 [details] [diff] [review]
Patch

See Blake's drive-by nit.
Attachment #349092 - Flags: review?(gal) → review+
(Assignee)

Comment 3

10 years ago
Pushed to TM as 21807:65019ca2ece8.

Comment 4

10 years ago
needs a test
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Comment 5

10 years ago
I think a JS regression test would be appropriate. What dir would it go in? js1_1 because regexps have been around forever, or a new regexp dir, or what? I don't know the location conventions.

Comment 6

10 years ago
ecma_3/RegExp would be fine if it doesn't use any weirdness other than turning jit on/off via jit(true)/jit(false).
(Assignee)

Comment 7

10 years ago
Created attachment 349358 [details] [diff] [review]
Regression test case
Attachment #349358 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #349358 - Flags: review? → review?(bclary)

Comment 8

10 years ago
Comment on attachment 349358 [details] [diff] [review]
Regression test case

note the test passes but nothing appears to be traced from what I can tell. Thanks for the test. Do you want to check it in or would you like me to? I normally check into both CVS trunk and mozilla-central.
Attachment #349358 - Flags: review?(bclary) → review+
(Assignee)

Comment 9

10 years ago
(In reply to comment #8)
> (From update of attachment 349358 [details] [diff] [review])
> note the test passes but nothing appears to be traced from what I can tell.

Correct. The regexp->native compiler shares the js option and some of the machinery with tracing but does not in fact trace. 

> Thanks for the test. Do you want to check it in or would you like me to? I
> normally check into both CVS trunk and mozilla-central.

Hmmm. I think I'll check it in to TraceMonkey for now because that's what I did with the actual fix, and let them be picked up together. But if you want to, certainly feel free to check it in to the others now.

Pushed to TM as 5f3dfe048606.

Updated

10 years ago
Whiteboard: fixed-tm

Updated

10 years ago
Whiteboard: fixed-tm → [fixed-tracemonkey]

Updated

10 years ago
Whiteboard: [fixed-tracemonkey] → fixed-in-tracemonkey

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 11

10 years ago
sync mc with cvs.
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-465862.js,v  <--  regress-465862.js
initial revision: 1.1
Flags: in-testsuite+
Flags: in-litmus-

Comment 12

10 years ago
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.