1132128 - Global RegExp loop overload

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Robin Masters]

Attachment: Example

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.35 Safari/537.36 OPR/28.0.1750.15 (Edition beta)

Steps to reproduce:

Bug report by Robin Masters (software engineer for Ranshuijsen van Loon, The Netherlands)

Run the attached Example without opening Firebug



Actual results:

Using the RegExp global in a long iteration results in mismatched. At some point RegExp global does not get updated anymore and same results are returned.
http://i.imgur.com/8KO95T5.png

NOTE:
When Firebug is opened this problem doesn't occur:  http://i.imgur.com/EX6ph5d.png



Expected results:

No matches found.

Comment 1

[Matthias Versen [:Matti]]

Firebug disables AFAIK the jit and I can confirm that running Firefox in the safemode (=disabled jit) also "fixes" the behavior.

Last good revision: 2f198e81ed98
First bad revision: 18f408a5984e
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f198e81ed98&tochange=18f408a5984e

Comment 2

[Till Schneidereit [:till]]

There are two Regexp-related landings in that regression window for bug 1038592. Needinfo'ing nbp as he reviewed them.

Comment 3

[Alice0775 White]

Pushlog:https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5c73b82d7723&tochange=abff854401bd
Last Good:d538719acebe
First Bad:d591725fbf82

Comment 4

[André Bargull [:anba]]

Simplified shell test case, run with "--ion-eager --ion-offthread-compile=off":
---
for (var i = 0; i < 100; i++) { 
  callRegExpTest(i);
}

function callRegExpTest(i) {
  var s = "" + i;
  var re = /(\d+)/;
  re.test(s);
  assertEq(RegExp.$1, s);
}
---

Comment 5

[Nicolas B. Pierron [:nbp]]

I will look at this issue in 2 weeks.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> I will look at this issue in 2 weeks.

For now can we just make canRecoverOnBailout() return false for the MRegExp* instructions? It's a pretty bad correctness bug that affects real-world code.

We're likely already too late to get a patch approved for beta though...

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > I will look at this issue in 2 weeks.
> 
> For now can we just make canRecoverOnBailout() return false for the MRegExp*
> instructions?

r=me for the work-around, but leave this bug opened with the ni?.

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> r=me for the work-around, but leave this bug opened with the ni?.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b118b959ffd

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Workaround

Approval Request Comment
[Feature/regressing bug #]: Bug 1038592.
[User impact if declined]: Correctness bugs, broken websites.
[Describe test coverage new/current, TreeHerder]: Patch is on m-i; fixes the test.
[Risks and why]: No risk, just disables a feature for now.
[String/UUID change made/needed]: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43d763ba54e8
https://hg.mozilla.org/releases/mozilla-beta/rev/8597521cb8bd

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Calling Fx38 fixed via the workaround landing on it too, but feel free to set it back to affected if you intend to uplift the real fix as well.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #12)
> Calling Fx38 fixed via the workaround landing on it too, but feel free to
> set it back to affected if you intend to uplift the real fix as well.

Just read the backlog, I will just close this bug and open a follow-up.
Thanks.