821340 - "Assertion failure: gc::IsMarked(x), at ../jsweakmap.h:140" with CountHeap

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


var g = newGlobal('new-compartment');
var dbg = Debugger(g);
function test(code, expected) {
    var actual = '';
    g.h = function () { actual += dbg.getNewestFrame().environment.type; }
    g.eval(code);
}
test("h();", 'object');
gczeal(4);
var count2 = countHeap();

Comment 1

[Jason Orendorff [:jorendorff]]

Reproduced.

Comment 2

[Christian Holler (:decoder)]

Updated wrong bug. nevermind.

Comment 3

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

This is a known problem where we try to mark stuff during a non-marking trace of weakmaps (from CountHeap in this case).

Comment 4

[Andrew McCreight [:mccr8]]

Let me know if I do anything to help, Bill.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   114221:2fd089d5fff4
user:        Jon Coppeard
date:        Fri Oct 12 15:26:07 2012 +0100
summary:     Bug 790338 - Fix issues with gray marking r=billm

This iteration took 55.588 seconds to run.

Comment 6

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

Attachment: patch

For posterity, here's why this happened. The test has a live weakmap that maps a dead key to a dead value. At the end, we check that the dead value is collected. Before every CC, we're doing ForgetSkippable and it's calling UnmarkGray on a bunch of stuff that eventually leads to the live weakmap. Then it calls UnmarkGray on the dead value, causing it not to be collected.

One fix would be to only UnmarkGray the value if the key is black. However, we're already planning to do a weakmap fixup pass in the cycle collector. It's a lot easier to rely on that and never trace through the weakmap values during UnmarkGray.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 693187 [details] [diff] [review]
patch

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

Thanks for investigating and fixing this!

::: js/src/gc/Marking.cpp
@@ +1506,5 @@
> +     * We set eagerlyTraceWeakMaps to false because the cycle collector will fix
> +     * up any color mismatches involving weakmaps when it runs.
> +     */
> +    UnmarkGrayTracer(JSRuntime *rt)
> +      : tracingShape(false), previousShape(NULL)

nit: your indentation is inconsistent with the other constructor. I don't know which is right.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/95566cb18e2e

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95566cb18e2e

Should this have a test?

Comment 10

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

Bug 790338 (the suggested regressor) only landed in Firefox 20, marking as such.

Comment 11

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

Added the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4d09f70c69

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c4d09f70c69