632964 - String.match will stop matching once it encounters a very long match

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marcel Poirier]

User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 1.1.4322; .NET CLR 2.0.50727; .NET CLR 3.0.04506.30; .NET CLR 3.0.04506.648; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729)
Build Identifier: Firefox 4.0b11

When doing String.match, the matching process will stop once a long string (252352 characters in length) is encountered - view sample html attachment. So if the regex should of matched on 3 items, if item 2 is a long String then the array returned by String.match will only contain the first item.

In the sample HTML attached, if you remove 1 character from the long script tag, then the String.match will work as expected.

Reproducible: Always

Comment 1

[Marcel Poirier]

Attachment: Sample html page that shows the problem.

Comment 2

[(mostly gone) XtC4UaLL [:xtc4uall]]

This regressed between Beta 3 <-> 4.
Further Digging on TM-Branch reveals Range:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=f84b470314a8&tochange=ab1c626399bb (Nightlys 20100811 <-> 20100812).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like a YARR problem.  And indeed, Safari 5 shows the same behavior.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Marcel, thanks for the testcase here!

Comment 5

[Oliver Hunt]

This does appear to be fixed in ToT JavaScriptCore, unfortunately I have no idea if it was a deliberate fix, or a fix that simply occurred as part of some other change.

It should be possible to narrow down the range for the fix using the webkit nightlies -- I'd be willing to help narrow down the revision if someone got it down to two nightlies.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Looks like r72487 has the bug and r72783 does not.  There seem to be no nightlies between those at http://nightly.webkit.org/builds/trunk/mac/1

Comment 7

[Oliver Hunt]

My testing seems to indicate r72489 make this bug go away, which was the switch away from PCRE and on to the YARR interpreter as the fallback non-jit regexp engine.

That implies a bug in pcre, are you using a JSC specific version of pcre?  or are you falling back on something else entirely?

Comment 8

[Oliver Hunt]

bz: any progress?

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't know the answer to your questions from comment 7; cdleary should, I think, which is why I cced him.  I do believe we fall back on PCRE, but I don't know whether it's a JSC specific version or not...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Or maybe dmandelin, who's also cced and has the bug assigned to him.  Convenient, that!

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #7)
> That implies a bug in pcre, are you using a JSC specific version of pcre?  or
> are you falling back on something else entirely?

Yeah, unfortunately we won't be able to follow you guys and make the switch to YARR-interp for FF4. We took the JSC fork of PCRE along with YARR-JIT. I'm checking this out a bit while dmandelin is out.

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: More reduced test case.

Note that removing the character class alternate in the capturing group appears to avoid this bug.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

Horray, easy fix. We actually call into match error because we've hit a recursion limit (given by the hard-coded matchLimit constant of 1e6). We just need to report it properly.

The original site probably wants to use a regexp of the form: /s([\s\S]*?)e/gi if the capturing group is actually needed. Since there's no "DOTALL" regexp option in JS (I think this is the m flag in Perl, but it's been a while since I've written Perl) you have to use a universal character class like [\s\S] to get the effect you're looking for.

On my machine:

/s(\s|.)*?e/gi: 193ms
/s([\s\S]*?)e/gi: 5ms
/s(?:[\s\S]*?)e/gi: 5ms

Stealing assignment.

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Handle RE-limit error properly.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/bddea4962013

Comment 16

[Daniel C]

(In reply to comment #11)
> unfortunately we won't be able to follow you guys and make the switch to
> YARR-interp for FF4.

Is there a bug for that?

Comment 17

[David Mandelin [:dmandelin]]

(In reply to comment #16)
> (In reply to comment #11)
> > unfortunately we won't be able to follow you guys and make the switch to
> > YARR-interp for FF4.
> 
> Is there a bug for that?

Bug 625600.

Comment 18

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/bddea4962013

Comment 19

[Marcel Poirier]

So, will this fix be available in FF4?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes, it's available in today's nightlies even.