try { x = function() {} for (y = 0; y < 64; y++) { x += x } } catch (e) {} x.replace(x, x, "gy") crashes js opt shell on m-c changeset 33064e13c3fd without any CLI arguments at js::RegExpShared::compile when the testcase is passed in as a CLI argument. s-s and assuming sec-critical because weird memory address 0x913fb008 is being accessed. autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 116117:d229008d60ac user: Sean Stangl date: Wed Dec 12 17:23:04 2012 -0800 summary: Bug 808245, Part 4/6 - Compile RegExpShared at execution time. r=dvander

This is a great fuzz result. The problem is that the code currently assumes that the RegExp JSAtom source, stored in the RegExpShared, is kept rooted by at least one RegExpObject. This assumption is incorrect in the context of our "flags" extension to str_replace(), which converts a string to a regular expression without rooting it. The solution is to have the RegExpGuard hold a root to the source. This gets a bit tricky, because Bug 820124 wants there to be a RegExpGuard in the RegExpStatics -- so there will have to be a second "heap-safe" RegExpGuard-like class just for that case.

Are you the right owner for this sg:crit regression? If not, please re-assign appropriately.

Yes, I'm the right owner. I have a fix already, but I'm going to manually verify all the RegExp rooting sites to make sure this bug doesn't occur again.

https://hg.mozilla.org/mozilla-central/rev/7d9055884036 Needs landing on aurora.

JSBugMon: This bug has been automatically verified fixed.

Comment on attachment 698851 [details] [diff] [review] Root RegExp source for the lifetime of RegExpShared [Approval Request Comment] Bug caused by (feature/regressing bug #): 808245 User impact if declined: Unlikely crashes Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None

Comment on attachment 698851 [details] [diff] [review] Root RegExp source for the lifetime of RegExpShared Low risk patch for a Fx 20 sec-crit regression.Considering where we are in the cycle this is manageable risk, hence approving it on aurora.

Debugging Bug 820124 revealed that a small issue exists in this patch, which can lead to random crashes of a similar variety to the original bug. The HeapPtr must be a raw pointer.

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 44dcffe8792b).

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0dbf0686ae Will also need landing on aurora after it hits m-c. I'll assume that the a=bajaj from above is valid for this fix, and land it tomorrow.

> Will also need landing on aurora after it hits m-c. I'll assume that the > a=bajaj from above is valid for this fix, and land it tomorrow. Request for approval for aurora landing will have to be made again because this is a separate patch. :|

JSBugMon: This bug has been automatically verified fixed.

Comment on attachment 701381 [details] [diff] [review] follow-up fix [Approval Request Comment] Bug caused by (feature/regressing bug #): 826581 (previous patch) User impact if declined: The first patch in this bug, which fixed the issue originally identified, introduced an extremely subtle bug involving prebarriers called during destructors. This can manifest as extremely unlikely crashes that are difficult to debug and obscure as to their cause. Testing completed (on m-c, etc.): m-c and significant fuzzing as part of Bug 820124, which identified the issue. Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None

I've got this in my queue for Aurora landing once it reopens. Clearing checkin-needed so this doesn't show up in my queries.