Bug 5602

Summary:

REGRESSION: RegExp("[^\\s$]+", "g") returns extra matches

Product:

WebKit

Reporter:

mitz

Component:

JavaScriptCore

Assignee:

Geoffrey Garen <ggaren>

Status:

VERIFIED FIXED

Severity:

Normal

Priority:

Version:

420+

Hardware:

Mac

OS:

OS X 10.4

Bug Depends on:

Bug Blocks:

5571

Attachments:

Description	Flags
Testcase?	none
patch to set lastIndex properly, fixes test case	ggaren: review-
Patch to fix suspected typo in Darin's patch	darin: review+
fast/js/regexp-lastindex.html	none

mitz

Reported 2005-11-02 15:04:55 PST

Scanning with the regular expression [^\\s$] returns extra matches. For example, in the testcase, scanning " abcdefg" returns "abcdefg" and then "fg" (with only one leading space it returns "g"). This regression is at least one of the reasons for bug 5597 and bug 5571 in TOT.

Attachments
Testcase? (299 bytes, text/html) 2005-11-02 15:05 PST, mitz	no flags	Details
patch to set lastIndex properly, fixes test case (1.08 KB, patch) 2005-11-02 20:17 PST, Darin Adler	ggaren: review-	Details Formatted Diff Diff
Patch to fix suspected typo in Darin's patch (1.07 KB, patch) 2005-11-02 21:59 PST, Geoffrey Garen	darin: review+	Details Formatted Diff Diff
fast/js/regexp-lastindex.html (645 bytes, text/html) 2005-11-03 09:54 PST, Geoffrey Garen	no flags	Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

mitz

Comment 1 2005-11-02 15:05:57 PST

Created attachment 4568 [details] Testcase?

Darin Adler

Comment 2 2005-11-02 17:36:09 PST

Looks like the bug is in the text/exec case inside RegExpProtoFuncImp::callAsFunction. It does a match, and then sets lastIndex to lastIndex + match.size(). But that's only accurate if the match is at the start of the string. Probably easy to fix by using endOffset in that place.

Darin Adler

Comment 3 2005-11-02 20:17:47 PST

Created attachment 4572 [details] patch to set lastIndex properly, fixes test case

Geoffrey Garen

Comment 4 2005-11-02 21:56:39 PST

Comment on attachment 4572 [details] patch to set lastIndex properly, fixes test case lastIndex + foundIndex + match.size() should be foundIndex + match.size() -- was that a typo?

Geoffrey Garen

Comment 5 2005-11-02 21:59:05 PST

Created attachment 4577 [details] Patch to fix suspected typo in Darin's patch

Darin Adler

Comment 6 2005-11-03 07:22:18 PST

Comment on attachment 4577 [details] Patch to fix suspected typo in Darin's patch Heh, I wouldn't call my mistake a "typo" exactly. But this new patch looks good. Please include a test case that would have failed with my patch.

Geoffrey Garen

Comment 7 2005-11-03 09:54:02 PST

Created attachment 4583 [details] fast/js/regexp-lastindex.html Testcase that catches the bug in Darin's patch.

Note You need to log in before you can comment on or make changes to this bug.