Last Comment Bug 728021 - Bug in regexp interpreter in recursive function
: Bug in regexp interpreter in recursive function
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 10 Branch
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla14
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on:
Blocks: 692657
  Show dependency treegraph
 
Reported: 2012-02-16 14:45 PST by Thomas Lanteigne
Modified: 2012-06-07 06:31 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified
+
verified
+
verified


Attachments
web page testcase (198 bytes, text/html)
2012-02-17 07:49 PST, Boris Zbarsky [:bz]
no flags Details
patch (1.24 KB, patch)
2012-03-09 06:15 PST, Brian Hackett (:bhackett)
dmandelin: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Review

Description Thomas Lanteigne 2012-02-16 14:45:58 PST
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 2012-02-16 14:48:29 PST
- 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 2012-02-17 07:42:11 PST
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 [:bz] 2012-02-17 07:49:03 PST
Created attachment 598240 [details]
web page testcase

OK, I do see this in beta but not on trunk...
Comment 4 Boris Zbarsky [:bz] 2012-02-17 08:01:47 PST
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] 2012-03-06 14:29:53 PST
(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 (:bhackett) 2012-03-06 14:37:17 PST
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] 2012-03-07 18:21:36 PST
(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
Comment 8 Brian Hackett (:bhackett) 2012-03-08 06:15:26 PST
This works fine for me in the shell.  Can someone write some exact STR for this?  Is firebug required?
Comment 9 Boris Zbarsky [:bz] 2012-03-08 08:17:44 PST
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 [:bz] 2012-03-08 08:18:11 PST
And I see this in a beta shell too.
Comment 11 Brian Hackett (:bhackett) 2012-03-08 08:36:28 PST
I can't repro in a beta shell.  Can you paste the testcase and cset you are on?
Comment 12 Boris Zbarsky [:bz] 2012-03-08 20:31:22 PST
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 (:bhackett) 2012-03-09 06:15:10 PST
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.
Comment 14 David Mandelin [:dmandelin] 2012-03-09 14:04:38 PST
Comment on attachment 604391 [details] [diff] [review]
patch

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

Nice detailed patch comment.
Comment 15 Brian Hackett (:bhackett) 2012-03-10 06:38:10 PST
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] 2012-03-12 15:43:50 PDT
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 18 Ed Morley [:emorley] 2012-03-13 17:28:35 PDT
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] 2012-03-14 14:42:42 PDT
https://hg.mozilla.org/mozilla-central/rev/4c529cbd1e0f

(Marking fixed, but still needs aurora13 nomination)
Comment 20 David Mandelin [:dmandelin] 2012-03-15 16:56:02 PDT
(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] 2012-04-02 01:31:01 PDT
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 B [:simonab] 2012-05-09 01:12:16 PDT
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] 2012-06-07 06:31:05 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.