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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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;
}
gcPreserveCode();
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");
gcslice(0);
gcslice(1);
gc();
var strings = [ (0), ];
for (var i = 0; i < strings.length; i++)
  reportCompare(expect, actual, summary + (5e1) + strings[i]);

Comment 1

[Christian Holler (:decoder)]

S-s until investigated because this is GC related.

Comment 2

[Christian Holler (:decoder)]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

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.

Comment 6

[Andrew McCreight [:mccr8]]

Setting sec-critical because this sounds kind of bad. Feel free to adjust as needed.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix for related bug

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.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

These patches will require nominations for sec-approval.

Comment 9

[Bob Clary [:bc] (inactive)]

reproduced on Assertion failure: *ptr == 0x3D, with Nightly/19  Windows 7 on http://www.myshared.ru/slide/154048/

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Why do these require sec-approval? They only affect nightly.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5e4097dd20
https://hg.mozilla.org/integration/mozilla-inbound/rev/f458467cc2cf

Comment 14

[Ed Morley [:emorley]]

Backed out for make check failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f458467cc2cf

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dcc0beba62c

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, stupid mistake. I hadn't tested any opt builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef966ef53b23

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Also, I failed to check in the test the second time:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed1c5d55d104

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Backed out again in case some random orange was caused by these patches:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60c4b0129f99

Comment 18

[Ed Morley [:emorley]]

(In reply to Bill McCloskey (:billm) from comment #17)
> Backed out again in case some random orange was caused by these patches:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/60c4b0129f99

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).

:-)

Comment 19

[Bob Clary [:bc] (inactive)]

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 http://thisisluckyme.com/diablo/

Comment 20

[Alex Keybl [:akeybl]]

Tracking for FF19, and nominating for FF18 given the fact that bug 807535 will likely reland on Aurora soon.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bdd3bfd15630

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bdd3bfd15630

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 677890 [details] [diff] [review]
fix

[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.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 677897 [details] [diff] [review]
fix for related bug

[Approval Request Comment]
(See above)

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f32f5bf29668

Comment 26

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 28

[u279076]

Can this be put in testsuite?