Closed Bug 600884 Opened 11 years ago Closed 11 years ago

JM: "Assertion failure: LookupLoop(traceMonitor, tree->ip, tree->globalObj, tree->globalShape, tree->argc) == tree->first,"

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

RESOLVED DUPLICATE of bug 600889
Tracking Status
blocking2.0 --- betaN+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: gkw, Assigned: igor)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [sg:dupe 600889][hardblocker])

function z(zz) {
    if (zz) this.e = null
}
for each(let a in [Number(), 0, 0, 0, 0, 0, new String(), new String(), 0, 0]) {
    try {
        let y = new z(a)
        gc()
    } catch (e) {}
}

asserts js debug shell on TM changeset 98c134cf59ef with -m and -j at Assertion failure: LookupLoop(traceMonitor, tree->ip, tree->globalObj, tree->globalShape, tree->argc) == tree->first,

Setting s-s because this involves gc.
blocking2.0: --- → ?
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54627:81881086131a
user:        Brian Hackett
date:        Wed Sep 29 06:27:34 2010 -0700
summary:     Improved JM call path, bug 587707. r=lw,dvander
Blocks: 587707
I don't think this was caused by 81881086131a.

The assertion is TM looking for a tree fragment which was disconnected by TraceMonitor::sweep because it contains a dead GC thing (the shape made by 'this.e = null').  It looks like two traces get created here.  The first in a 'new String()' iteration, where the 'if (zz)' test passes, branch taken and shape used.  That falls off trace shortly when the branch isn't taken, and a second trace is later recorded, which has the first trace as its 'tree->first'.  During recording of the second trace the first trace is GC'ed, because the shape it contains is no longer reachable, and we assert.

81881086131a doesn't touch any of this.  If I use an earlier revision then the shape isn't collected, presumably because it is still reachable from the C stack via the conservative collector (also, mildly perturbing the testcase in unrelated ways makes this assert go away).  The shape is garbage though, and should be collected.

I don't know what the right fix is.
(In reply to comment #2)
> I don't think this was caused by 81881086131a.
> 
> The assertion is TM looking for a tree fragment which was disconnected by
> TraceMonitor::sweep because it contains a dead GC thing (the shape made by
> 'this.e = null').  It looks like two traces get created here.  The first in a
> 'new String()' iteration, where the 'if (zz)' test passes, branch taken and
> shape used.  That falls off trace shortly when the branch isn't taken, and a
> second trace is later recorded, which has the first trace as its 'tree->first'.
>  During recording of the second trace the first trace is GC'ed, because the
> shape it contains is no longer reachable, and we assert.

At some earlier point, shapes (then sprops) and other GC'ed immediates were kept in trace-JITted code and marked, making them strong refs. Now it seems they are treated as weak refs and tested in TM::sweep to see whether they're not marked.

> 81881086131a doesn't touch any of this.  If I use an earlier revision then the
> shape isn't collected, presumably because it is still reachable from the C
> stack via the conservative collector (also, mildly perturbing the testcase in
> unrelated ways makes this assert go away).  The shape is garbage though, and
> should be collected.

This can't be the full story, as Shapes are not GC-things, so not scanned via conservatively interpreted stack word maybe-refs.

> I don't know what the right fix is.

I don't see the assertbotch on tm tip, with -j or -m -j. Was it just a bogus assertion?

/be
(In reply to comment #3)
> This can't be the full story, as Shapes are not GC-things, so not scanned via
> conservatively interpreted stack word maybe-refs.

Yeah, the shape can only get marked via one of the 'new z(a)' objects allocated in either of the 'new String()' iterations.  If either of those objects still has a lingering reference on the C stack, the shape will not be collected.

> I don't see the assertbotch on tm tip, with -j or -m -j. Was it just a bogus
> assertion?

I get the failure on tip (rev 2824ef10a50f) on OS X 10.6 under x86.  Since the conservative GC is involved this bug is pretty fragile.
Any ill effects on optimized builds, under valgrind, etc.?

I'm still wondering if this isn't a bogus assertion.

/be
(In reply to comment #5)
> Any ill effects on optimized builds, under valgrind, etc.?
> 
> I'm still wondering if this isn't a bogus assertion.
> 
> /be

It might be bogus, I don't know the TM code well enough to tell.  At the assert, tree->first points to the fragment which referenced the shape, and which has now gone through TrashTree.  TrashTree doesn't seem to free the fragment though (how are TreeFragments destroyed?), and I don't know if it will continue to work OK.  Optimized build doesn't crash on me.
blocking2.0: ? → betaN+
Whiteboard: [sg:critical]
Need some TM trace-trees expert to weigh in. Andreas, dvander? IIRC Igor was patching around related code.

/be
Yeah, this looks related to Igor's fragment sweeping changes.
Assignee: general → igor
Igor, any thoughts here?
Igor, ping?
I believe this is related to the bug 600889. Gal, could you comment there on some issues regarding your suggestion.
Variations assert at Assertion failure: tree->first == LookupLoop(traceMonitor, ip, tree->globalObj, tree->globalShape, tree->argc), but usually get reduced to the assert in this bug.
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
(In reply to comment #11)
> I believe this is related to the bug 600889. Gal, could you comment there on
> some issues regarding your suggestion.

Indeed, this bug WFM on tip and the first rev where it works is:

changeset:   58587:5177ee4c10d6
user:        Igor Bukanov <igor@mir2.org>
date:        Mon Oct 04 23:08:11 2010 +0200
summary:     Bug 600889 - TraceMonitor::sweep() should abort recording if it trashes the recorder tree. r=gal
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 600889
Group: core-security
Whiteboard: [sg:critical][hardblocker] → [sg:dupe 600889][hardblocker]
A testcase for this bug was already added in the original bug (bug 600889).
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.