883472 - GenerationalGC: Assertion failure: t->arenaHeader()->allocatedDuringIncremental, at jsgcinlines.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

The upcoming attached testcase asserts js debug shell on m-c changeset 5ddb1bf96261 with --baseline-eager at Assertion failure: t->arenaHeader()->allocatedDuringIncremental, at jsgcinlines.h

Comment 2

[Terrence Cole [:terrence]]

I was not able to reproduce locally, but did take a look briefly on Gary's machine. This assertion is failing because we attempt to call Arena::thingSize(512), which is clearly going to give us random garbage. The busted AllocKind is coming off of a Class in theory, but the clasp isn't in any register when it gets passed to us. I wasn't sure where to look on the stack because the immediate caller of NewBuiltinClassInstance appears to be a BaselineIC.

The gdb/clang combo on Gary's machine doesn't have a working step, tui mode, or really any of the features I'd need to get to the bottom of this in a reasonable amount of time, so unless I can get this reproducing locally, there isn't much I can do.

Comment 3

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

Turning this into a baseline bug.

Comment 4

[Jan de Mooij [:jandem]]

I'm unable to reproduce this with the info in comment 0 and comment 1.

Terrence, the attached stack trace shows this happens under BaselineCompiler::emit_JSOP_NEWOBJECT. Is that different from what you were seeing?

Bug 880805 is the same assert.

Comment 5

[Christian Holler (:decoder)]

Why is this s-s if it's generational GC? Also, Langfuzz found this already in bug 880805, or is there a reason why this could be a different bug?

Comment 6

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

(In reply to Christian Holler (:decoder) from comment #5)
> Why is this s-s if it's generational GC? Also, Langfuzz found this already
> in bug 880805, or is there a reason why this could be a different bug?

Terrence mentioned it wasn't GGC at fault (at least in-person), that's why he cc'ed the baseline folks. Also, this required --baseline-eager and I didn't know if it was related to bug 880805's --ion-eager.

Comment 7

[Terrence Cole [:terrence]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Terrence, the attached stack trace shows this happens under
> BaselineCompiler::emit_JSOP_NEWOBJECT. Is that different from what you were
> seeing?
> 
> Bug 880805 is the same assert.

I did not look at the op, but that is reasonable, given where it is crashing.

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> (In reply to Christian Holler (:decoder) from comment #5)
> > Why is this s-s if it's generational GC? Also, Langfuzz found this already
> > in bug 880805, or is there a reason why this could be a different bug?

Sorry, I forgot to uncheck.

> Terrence mentioned it wasn't GGC at fault (at least in-person), that's why
> he cc'ed the baseline folks. Also, this required --baseline-eager and I
> didn't know if it was related to bug 880805's --ion-eager.

Not so much not GGC, rather that the baseline barriers are likely at fault. Kannan would be the right person to look into it if he can reproduce easily. If not, I'll try to set up a mac box next week (optimistically) to debug on.

Comment 8

[Kannan Vijayan [:djvj]]

Do I need to enable generational GC to reproduce this?  Also, is it specifically MacOSX or does Linux x86-64 also show the same issue?

Comment 9

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

(In reply to Kannan Vijayan [:djvj] from comment #8)
> Do I need to enable generational GC to reproduce this?  Also, is it
> specifically MacOSX or does Linux x86-64 also show the same issue?

Christian hit this in Linux, I hit it on Mac. And I'm pretty sure we needed --enable-exact-rooting --enable-gcgenerational to trigger this.

Comment 10

[Terrence Cole [:terrence]]

Ah, interesting: I was not able to reproduce bug 880805 either.

Comment 11

[Kannan Vijayan [:djvj]]

I've managed to replicate this on my machine.  Will take a look at it.

Comment 12

[Kannan Vijayan [:djvj]]

After some extremely helpful guidance from terrence on irc, the issue seems to be the following:

In, ArenaLists::allocateFromArenaInline, in jsgc.cpp, new allocations into zones during incremental GC set the "allocatedDuringIncremental" flag:

            if (JS_UNLIKELY(zone->wasGCStarted())) {
                if (zone->needsBarrier()) {
                    aheader->allocatedDuringIncremental = true;
                    zone->rt->gcMarker.delayMarkingArena(aheader);
                } else if (zone->isGCSweeping()) {
                    PushArenaAllocatedDuringSweep(zone->rt, aheader);
                }
            }

However, this is within a conditional that checks |zone->needsBarrier()|.

When a minor collection runs, the following happens:

    MinorCollectionTracer(JSRuntime *rt, Nursery *nursery)
      : JSTracer(),
        nursery(nursery),
        session(rt, MinorCollecting),
        tenuredSize(0),
        head(NULL),
        tail(&head),
        savedNeedsBarrier(rt->needsBarrier()),
        disableStrictProxyChecking(rt)
    {
        JS_TracerInit(this, rt, Nursery::MinorGCCallback);
        eagerlyTraceWeakMaps = TraceWeakMapKeysValues;

        rt->gcNumber++;
        rt->setNeedsBarrier(false);
        for (ZonesIter zone(rt); !zone.done(); zone.next())
            zone->saveNeedsBarrier(false);
    }

Here, |saveNeedsBarrier| sets |zone->needsBarrier| to false, and stores its old value in |zone->savedNeedsBarrier|.  This presumably causes the |allocatedDuringIncremental=true| in |allocateFromArenaInline| to not execute when ggc is runing, leading to the assertion here being hit.

This is the best I can figure from my incomplete knowledge of gc guts, and with much help from terrence.  He indicates that billm and he are coming up with a patch that should address it.

Waiting on that.  I don't hink Jan's input is needed at this point, so clearing the needinfo from him.

Comment 13

[Terrence Cole [:terrence]]

Attachment: v0

Thanks for the help tracking this down, Kannan!

As is usual, Bill asked a very cogent question that none of us thought of: why do we need to prevent pre-barriers during minor GC? After looking at the callbacks, we think that the minor GC callbacks should be fine with this. Thus, the best way forward is to just loosen the offending assertions a bit.

Gary, I still can't reproduce: does this patch work for you?

Comment 14

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

Comment on attachment 765089 [details] [diff] [review]
v0

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

::: js/src/gc/Nursery.cpp
@@ +282,1 @@
>          static_cast<Cell *>(t)->markIfUnmarked();

You should be able to remove this whole block now (starting with the "Pre barriers are disabled" comment).

Comment 15

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

Comment on attachment 765089 [details] [diff] [review]
v0

Yes, this fixes the bug for me.

Comment 16

[Terrence Cole [:terrence]]

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

Comment 17

[Ed Morley [:emorley]]

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