Closed Bug 808067 Opened 11 years ago Closed 11 years ago

IonMonkey: Assertion failure: needsBarrier_, at ../../jscompartment.h:185


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected


(Reporter: decoder, Assigned: billm)



(4 keywords, Whiteboard: [jsbugmon:update][adv-main18-])


(2 files)

The following testcase asserts on mozilla-central revision 556b9cfb269f (run with --ion-eager):

function TestCase(n, d, e, a)
  this.reason = '';
function reportCompare (expected, actual, description) {
  var output = "";
  var testcase = new TestCase("unknown-test-name", description, expected, actual);
  testcase.reason = output;
var summary = 'return with argument and lazy generator detection';
expect = "generator function foo returns a value";
actual = (function (j)  {}).message;
reportCompare(expect, actual, summary + ": 1");
reportCompare(expect, actual, summary + ": 2");
var strings = [ (0), ];
for (var i = 0; i < strings.length; i++)
  reportCompare(expect, actual, summary + (5e1) + strings[i]);
S-s until investigated because this is GC related.
Blocks: IonFuzz
Whiteboard: [jsbugmon:update,bisect]
Assignee: general → wmccloskey
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   112050:23eb7d58bf90
user:        Bill McCloskey
date:        Thu Nov 01 08:08:21 2012 -0700
summary:     Bug 807535 - Avoid toggling Ion write barrier too often (r=sstangl)

This iteration took 93.383 seconds to run.
Attached patch fixSplinter Review
I assumed that ResetIncrementalGC didn't need to update the Ion barriers because it would be done when we finished the GC. However, after ResetIncremental GC, we'll start a new GC that might be over a different set of compartments. So we do need to update the barriers here.
Attachment #677890 - Flags: review?(sstangl)
Setting sec-critical because this sounds kind of bad. Feel free to adjust as needed.
I found a related bug here as well. When we execute AutoGCSlice::~AutoGCSlice in the last GC slice, all the compartments have had their states set to NoGC. Therefore, GCCompartmentsIter won't find anything and so we won't disable write barriers anywhere.

In practice I don't think this affects prior releases because we always disable barriers at the beginning of a slice. However, with the patch in bug 807535, it causes problems for IonMonkey.

We can just iterate over all compartments here and everything should be fine, I think.
Attachment #677897 - Flags: review?
Attachment #677897 - Flags: review? → review?(jcoppeard)
Attachment #677890 - Flags: review?(sstangl) → review+
Blocks: 807535
No longer blocks: 807913
Attachment #677897 - Flags: review?(jcoppeard) → review+
These patches will require nominations for sec-approval.
reproduced on Assertion failure: *ptr == 0x3D, with Nightly/19  Windows 7 on
Why do these require sec-approval? They only affect nightly.
(In reply to Bill McCloskey (:billm) from comment #11)
> Why do these require sec-approval? They only affect nightly.

Good point, apologies, I mis-spoke there - I must have confused this with another bug.
Backed out again in case some random orange was caused by these patches:
(In reply to Bill McCloskey (:billm) from comment #17)
> Backed out again in case some random orange was caused by these patches:

It didn't make a difference this time, since m-cMerge (the tool used to mark bugs after merging) intentionally doesn't deal with s-s bugs automatically; but for future occasions, please can you use a commit message like:
  "Backout/Back out/Backed out <changeset> (<and ideally also bug id>) reason"; since:
  "Bug 808067 - Back out again due to possible orange (r=orange)"

...looks like another normal landing to the tool. (Backouts also don't need the r=foo; the mercurial hook doesn't require an r= for any type of landing, just a bug number if the message doesn't start with "Back..." or "Merge..." etc).

hit Assertion failure: needsBarrier_
stack JSCompartment::barrierTracer() js::ion::MarkFromIon(JSCompartment*, JS::Value*) XUL@0x244568f
on Windows 7 and OSX for 2012-11-05 Nightly builds on
Tracking for FF19, and nominating for FF18 given the fact that bug 807535 will likely reland on Aurora soon.
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 677890 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 807535, which I want to land on Aurora
User impact if declined: crashes
Testing completed (on m-c, etc.): On m-c for a day.
Risk to taking this patch (and alternatives if risky): Low, I think. The earlier backout was apparently unnecessary.
String or UUID changes made by this patch: None.
Attachment #677890 - Flags: approval-mozilla-aurora?
Comment on attachment 677897 [details] [diff] [review]
fix for related bug

[Approval Request Comment]
(See above)
Attachment #677897 - Flags: approval-mozilla-aurora?
Attachment #677890 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #677897 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main18-]
Can this be put in testsuite?
Flags: in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.