Bug 18327 – REGRESSION (r28833): Regex fails on long string

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Christopher Schmidt]

The URL demonstrates that Safari 3.1 (Version 3.1 (5525.13)) and WebKit Nightly r31623 fail to parse the string when its length is over 25056 characters.

This regex is a commonly used way of determining whether a string is valid JSON.

In Safari 3.0, this works, and it works in all other browsers that I have tested it in (IE6/IE7, FF2/FF3, Opera 9.x).

[Christopher Schmidt]

It is possible this is somewhat related to http://bugs.webkit.org/show_bug.cgi?id=16323

[Christopher Schmidt]

Oh, and I wanted to test with other nightlies, but grabbing one from more than a couple weeks ago just fails because Safari is looking for symbols that don't exist in the build; this was as specific as I could get. Sorry about that.

[Alexey Proskuryakov]

Confirmed with r31667 (haven't tried 3.0 to verify that this is a regression though).

[Mark Rowe (bdash)]

<rdar://problem/6172476>

[Darin Adler]

Created attachment 22981 [details]
the linked test case

[Tim Schaub]

Created attachment 23262 [details]
test demonstrating string replace failure

This throws an exception in 3.1.2 (and should not).

[Christopher Schmidt]

(In reply to comment #6)
> Created an attachment (id=23262) [edit]
> test demonstrating string replace failure
> 
> This throws an exception in 3.1.2 (and should not).

A relatively recent JavascriptCore checkout (r35900) fails on this test case like so:

dhcp-208-80-142-180:~ crschmidt$ ./jsc crash.js ; echo $?
jsRegExpExecute failed with result -2
3

 

[David Kilzer (:ddkilzer)]

The bisect-builds script reports:

Works: r28811  Fails: r28843

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> The bisect-builds script reports:
> 
> Works: r28811  Fails: r28843

Most suspicious commit in that range:  <http://trac.webkit.org/changeset/28833>

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Most suspicious commit in that range:  <http://trac.webkit.org/changeset/28833>

Confirmed that the attached test case is hitting the matchLimit from r28833.  The test case that passes is "recursing" 99,999 times.  Each additional character in the matching string adds 4 "recursions", so the failing test case is "recursing" 100,003 times.

I don't know this code, so I'm not sure if there is a way to match more efficiently in this case.

[Darin Adler]

Geoff is working on regular expressions right now.

[Alexey Proskuryakov]

Se also bug 24166 (duplicate?).

[Darin Adler]

(In reply to comment #12)
> See also bug 24166 (duplicate?).

Not a duplicate; another separate limitation.

[Geoffrey Garen]

*** Bug 21485 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

*** Bug 24308 has been marked as a duplicate of this bug. ***

[Geoffrey Garen]

Created attachment 28766 [details]
patch

[Oliver Hunt]

Comment on attachment 28766 [details]
patch

r=me, but add a test case so we don't regress this again

[Geoffrey Garen]

Testcase added.

Committed revision 41849.

[David Levin]

Isn't this fixed?  (Can it be resolved to move it out of the commit queue?)

[David Kilzer (:ddkilzer)]

Marking RESOLVED/FIXED per Comment #18 and Comment #19.  Thanks!

[Pam Greene (IRC:pamg)]

Neither Firefox 3 nor Chromium (trunk) can run this test now. The long test case added in r41849 causes them both to time out. (IE7 finishes it in ~15 seconds, but it gets the wrong answer.)

I can't speak about FF, but V8 deliberately decided not to limit the runtime of long regexps, since the engine is intrinsically interruptible. So they use the normal "This script has been running a long time; do you want to continue?" mechanism rather than a special case.  See http://code.google.com/p/v8/issues/detail?id=287

I'd like to split this one problematic test case into a separate file, which Chromium (and any other platforms that had a different long-running-RE design goal) could then skip individually. Sound reasonable?

[Darin Adler]

(In reply to comment #21)
> I'd like to split this one problematic test case into a separate file, which
> Chromium (and any other platforms that had a different long-running-RE design
> goal) could then skip individually. Sound reasonable?

Seems fine.

[Pam Greene (IRC:pamg)]

Created attachment 29197 [details]
split long-running test case into separate file

[Darin Adler]

Comment on attachment 29197 [details]
split long-running test case into separate file

r=me

[Pam Greene (IRC:pamg)]

Split landed in r42177.