Closed Bug 728021 Opened 8 years ago Closed 8 years ago

Bug in regexp interpreter in recursive function

Categories

(Core :: JavaScript Engine, defect)

10 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox11 + wontfix
firefox12 + verified
firefox13 + verified
firefox14 + verified

People

(Reporter: boggan, Assigned: bhackett)

References

Details

(Keywords: regression, Whiteboard: [qa!])

Attachments

(2 files)

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.
- 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
Assignee: nobody → general
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
QA Contact: untriaged → general
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 :)
Attached file web page testcase
OK, I do see this in beta but not on trunk...
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....
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
(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?
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.
(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 <bhackett1024@gmail.com>
date:        Wed Oct 12 08:17:44 2011 -0700
summary:     Faster handling when calling native methods on regexp literals, bug 692657. r=cdleary
Blocks: 692657
This works fine for me in the shell.  Can someone write some exact STR for this?  Is firebug required?
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.
And I see this in a beta shell too.
I can't repro in a beta shell.  Can you paste the testcase and cset you are on?
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.
Attached patch patchSplinter Review
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.
Attachment #604391 - Flags: review?
Attachment #604391 - Flags: review? → review?(dmandelin)
Comment on attachment 604391 [details] [diff] [review]
patch

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

Nice detailed patch comment.
Attachment #604391 - Flags: review?(dmandelin) → review+
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.
Attachment #604391 - Flags: approval-mozilla-beta?
Attachment #604391 - Flags: approval-mozilla-aurora?
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.
Attachment #604391 - Flags: approval-mozilla-beta?
Attachment #604391 - Flags: approval-mozilla-beta-
Attachment #604391 - Flags: approval-mozilla-aurora?
Attachment #604391 - Flags: approval-mozilla-aurora+
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.
Target Milestone: --- → mozilla14
Assignee: general → bhackett1024
https://hg.mozilla.org/mozilla-central/rev/4c529cbd1e0f

(Marking fixed, but still needs aurora13 nomination)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(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
Whiteboard: [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.
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.
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.
Status: RESOLVED → VERIFIED
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.