Bug in regexp interpreter in recursive function

VERIFIED FIXED in Firefox 12

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Thomas Lanteigne, Assigned: bhackett)

Tracking

({regression})

10 Branch
mozilla14
x86
Mac OS X
regression
Points:
---

Firefox Tracking Flags

(firefox11+ wontfix, firefox12+ verified, firefox13+ verified, firefox14+ verified)

Details

(Whiteboard: [qa!])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
- 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
(Reporter)

Comment 2

6 years ago
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 :)
Created attachment 598240 [details]
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
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
Ever confirmed: true
Keywords: regression

Updated

6 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
(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?
(Assignee)

Comment 6

6 years ago
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
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 11

6 years ago
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.
(Assignee)

Comment 13

6 years ago
Created attachment 604391 [details] [diff] [review]
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.
Attachment #604391 - Flags: review?
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 15

6 years ago
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+
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c529cbd1e0f
https://hg.mozilla.org/releases/mozilla-aurora/rev/699889e9bce7
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.
status-firefox11: --- → wontfix
status-firefox12: --- → fixed
status-firefox13: --- → affected
tracking-firefox13: --- → ?
Target Milestone: --- → mozilla14

Updated

6 years ago
Assignee: general → bhackett1024
https://hg.mozilla.org/mozilla-central/rev/4c529cbd1e0f

(Marking fixed, but still needs aurora13 nomination)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-firefox14: --- → fixed
tracking-firefox13: ? → +
tracking-firefox14: --- → +
(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
status-firefox13: affected → fixed
Whiteboard: [qa+]

Comment 21

5 years ago
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.
status-firefox12: fixed → verified
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.
status-firefox13: fixed → verified

Comment 23

5 years ago
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
status-firefox14: fixed → verified
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.