Bug 5602 - REGRESSION: RegExp("[^\\s$]+", "g") returns extra matches
Summary: REGRESSION: RegExp("[^\\s$]+", "g") returns extra matches
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on:
Blocks: 5571
  Show dependency treegraph
 
Reported: 2005-11-02 15:04 PST by mitz
Modified: 2005-11-04 00:21 PST (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 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.
Comment 1 mitz 2005-11-02 15:05:57 PST
Created attachment 4568 [details]
Testcase?
Comment 2 Darin Adler 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.
Comment 3 Darin Adler 2005-11-02 20:17:47 PST
Created attachment 4572 [details]
patch to set lastIndex properly, fixes test case
Comment 4 Geoffrey Garen 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?
Comment 5 Geoffrey Garen 2005-11-02 21:59:05 PST
Created attachment 4577 [details]
Patch to fix suspected typo in Darin's patch
Comment 6 Darin Adler 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.
Comment 7 Geoffrey Garen 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.