728021 - Bug in regexp interpreter in recursive function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Thomas Lanteigne]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

Run this in firebug:

var i=0;

function relog()
{
  console.log( " " + i + ": " + /^[a-z0-9\.]+$/gi.test( "Views.ItemControlView" ) );
  i++;
if( i < 100 )
    relog();
}

relog();



Actual results:

the regexp evaluation starts to return false when it should always return true;


Expected results:

the evaluation of the regexp should always be true regardless of the size of the call stack.

Comment 1

[Thomas Lanteigne]

- we've tried this in Firefox 9.0.1 and it was not repro
- we've tried in Firefox Beta 11.0 and it was repro

Comment 2

[Thomas Lanteigne]

I just noticed something, if I take off the "global"  flag in my regexp, the problem goes away, I hope it helps to figure out the problem :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: web page testcase

OK, I do see this in beta but not on trunk...

Comment 4

[Boris Zbarsky [:bzbarsky]]

m-c fix range seems to be: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a230265bad5&tochange=c713003d3226

which is odd, because nothing in there jumps out at me.

Original breakage range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46a6d0fd13d5&tochange=9545b88eed82

Lots of JS changes there...  ccing some people who may know what's going on here.  It sounds like we're ending up caching the regexp object when we shouldn't....

Comment 5

[David Mandelin [:dmandelin]]

(In reply to Boris Zbarsky (:bz) from comment #4)
> m-c fix range seems to be:
> http://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=9a230265bad5&tochange=c713003d3226
> 
> which is odd, because nothing in there jumps out at me.

This backout of bug 712714 was probably hard to see:

http://hg.mozilla.org/mozilla-central/rev/7ab4f1ebc7cc

Brian, do you think it's worth backporting the backout to fix this bug, or would that be risky?

Comment 6

[Brian Hackett [Laid off!]]

That cset is actually backing out the backout, aka relanding it.  I don't think bug 712714 is involved here if the problem is visible on beta (Fx 11), as 712714 landed for Fx 12.

Comment 7

[David Mandelin [:dmandelin]]

(In reply to Brian Hackett (:bhackett) from comment #6)
> That cset is actually backing out the backout, aka relanding it.  I don't
> think bug 712714 is involved here if the problem is visible on beta (Fx 11),
> as 712714 landed for Fx 12.

Thanks. I bisected more. Turns out it's this one:

changeset:   78625:dfd8ef9e5049
user:        Brian Hackett <[e-mail]>
date:        Wed Oct 12 08:17:44 2011 -0700
summary:     Faster handling when calling native methods on regexp literals, bug 692657. r=cdleary

Comment 8

[Brian Hackett [Laid off!]]

This works fine for me in the shell.  Can someone write some exact STR for this?  Is firebug required?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Brian, did you see comment 3?  Are you testing a beta shell?

Firebug is not required to reproduce in browser using the comment 3 testcase.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And I see this in a beta shell too.

Comment 11

[Brian Hackett [Laid off!]]

I can't repro in a beta shell.  Can you paste the testcase and cset you are on?

Comment 12

[Boris Zbarsky [:bzbarsky]]

The testcase is the exact code in comment 0, with console.log() replaced by print().

I see this with changeset 3ad684ba34d0 in my beta repo.  I also see it with changeset 7eaebc022f31, which is the current beta tip.

Note that I only see it in shell if I run -m -n.  If I run just -m or interp the bug is not present.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: patch

Disable regexp cloning optimization for regexps with the global or sticky flags set.  Executing these can have side effects on the lastIndex property which are observable in future matches, so that not cloning the regexp has a visible effect.  This also affects tip, but bug 712714 inadvertently disabled the optimization for this testcase.

Comment 14

[David Mandelin [:dmandelin]]

Comment on attachment 604391 [details] [diff] [review]
patch

Review of attachment 604391 [details] [diff] [review]:
-----------------------------------------------------------------

Nice detailed patch comment.

Comment 15

[Brian Hackett [Laid off!]]

Comment on attachment 604391 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Potential incorrect regexp behavior in certain cases.
Risk to taking this patch (and alternatives if risky): None, disables optimization in certain cases.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 604391 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora 12, but denying for Beta 11 since this isn't critical enough to land between our final beta and release.

Comment 17

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c529cbd1e0f
https://hg.mozilla.org/releases/mozilla-aurora/rev/699889e9bce7

Comment 18

[Ed Morley [:emorley]]

Posting this in case myself/whomever merges next doesn't remember/spot this, but this missed the uplift, so will need landing on aurora13, since it's only fixed on 14 and the now beta12.

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4c529cbd1e0f

(Marking fixed, but still needs aurora13 nomination)

Comment 20

[David Mandelin [:dmandelin]]

(In reply to Ed Morley [:edmorley] from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/4c529cbd1e0f
> 
> (Marking fixed, but still needs aurora13 nomination)

Thanks for catching that. Tracking what's really where seems to be very confusing sometimes.

http://hg.mozilla.org/releases/mozilla-aurora/rev/ae7c1ff7e8d8

Comment 21

[Vlad [QA]]

Verified Fiexed on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0) Gecko/20100101 Firefox/12.0 beta 3

I have load the test page from the attachament and the evaluation of the regexp is always true.

Comment 22

[Simona Badau, Desktop QA]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0

Verified that the evaluation of the regexp is always true on Fireffox 13 beta 3 using the STR from the Description and the test case from Comment 3.

Comment 23

[Vlad [QA]]

I've verified this on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0 beta 6 (1st Firefox 14 beta)

The evaluation of the regexp is always true.
Setting resolution and also the flag to Verified.