770954 - Assertion failure: [barrier verifier] Unmarked edge: value, at jsgc.cpp:4580

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts on mozilla-central revision b6aa44d8f11f (options -m -n -a):


gczeal(4);
var s = new Set;
s.add(-0);
s.add(0);
assertEq(s.delete(-0), true);
assertEq(s.has(0), (true  ));
assertEq(s.has(-0), false);
var m = new Map;
m.set(-0, 'x');
assertEq(m.has(0), false);
assertEq(m.get(0), undefined);
assertEq(m.has(-0), true);
assertEq(m.delete(-0), true);


S-s due to GC-related assertion.

Comment 1

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

Jason, there's a problem here where the write barrier for incremental GC isn't being invoked correctly. In order for incremental GC to be correct, we need to ensure that once a Map entry gets into a state where it won't be traced during GC, then we call a write barrier on. That's not happening here in the m.delete(-0) call.

My initial expectation was that OrderedHashTable would call the destructor on the entry when removing it. That would cause the write barrier to be invoked. However, it seems to call the destructor more lazily, either when the table is resized or destroyed.

Somehow we need to call the destructor immediately upon removing the value, or we have to replace it with some dummy value, which will also cause the barrier to be invoked.

Comment 2

[Jason Orendorff [:jorendorff]]

OK, I'll take this. OrderedHashTable bounced, for unrelated reasons, so this won't affect anyone for a while. Sigh.

Comment 3

[Andrew McCreight [:mccr8]]

(leaving tracking-firefox16 unset for now because the code from bug 743107 isn't in 16 now, but could end up in it at some point)

Comment 4

[Daniel Veditz [:dveditz]]

Let's "track" this for 16 anyway -- at the very least to double check whether bug 743107 did or didn't land in that cycle.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v1

+1 line of code
+12 lines of comments
+25 lines of tests

Comment 6

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

Comment on attachment 643219 [details] [diff] [review]
v1

For the first comment, could you just say something like:
"We no longer plan to mark this value, so force an incremental write barrier on it by overwriting it."

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v2

Not quite as pretty as v1.

The line I added:
    e->value = Value();
doesn't compile in GCC 4.2! The compiler wants a copy constructor for that Value type. I can't see why. This code explicitly calls the default constructor, and then calls e->value.operator=, which (I checked) takes a reference argument, not a copy.

Simpler example that also doesn't compile in g++-4.2:

    struct X {
        X() {}
        explicit X(const X &x) {}
        void operator=(const X &x) {}
    };
    void f() {
        X x;
        x = X();  // error: no matching function for call to 'X::X(X)'
    }

If the 'explicit' tag on RelocatableValue's copy constructor is valuable, I can work around this some other way. Destroying removed values wouldn't be so bad. Another alternative is to do violence to the OrderedHashMap API to require the implementation to provide a makeEmpty() function that clears the whole entry, not just the key.

Comment 8

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

Jason, I wanted to help land this but it bitrotted a little:

$ hg qpush
applying bug-770954-OHT-gc-v2.patch
patching file js/src/builtin/MapObject.cpp
Hunk #1 FAILED at 581
Hunk #2 FAILED at 966
2 out of 2 hunks FAILED -- saving rejects to file js/src/builtin/MapObject.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug-770954-OHT-gc-v2.patch

Comment 9

[Jason Orendorff [:jorendorff]]

Gary, that's because the patch that caused this was backed out. Now it's relanded, and this patch has landed on top of it:

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

Comment 10

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

Great, thanks for the clarification!

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Andrew McCreight [:mccr8]]

Marking 16 as unaffected as the regressing patch was backed out from 16.

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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