748119 - Assertion failure: IsMarkedOrAllocated(static_cast<Cell *>(*thingp)), at jsgc.cpp:4284

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Test case for shell (run with -n -m -a)

The attached test asserts on mozilla-central revision 17af008937e3 (options -m -n -a).

Comment 1

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

Attachment: TI patch

This test exposed two problems. The first one is that we need read barriers on the TypeObjects and JSObjects that are stored in Types. Here's what can go wrong:

1. Incremental GC starts.
2. Type object that will eventually be swept is read out from the compiler. Some component of it (newScript->fun in this case) is copied to some other location so that it's now reachable.
3. We fail to mark the type object and its newScript->fun component, so potentially it gets collected. Later, it can be accessed.

It seems pretty easy to fix the problem. We just need to invoke a read barrier whenever we read anything out of a Type. I'm guessing that this isn't a very hot path.

Comment 2

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

Attachment: verifier patch

Unfortunately, even with the first patch, we still fail the test.

The barrier verifier is actually doing two checks:
1. If some object has a field that changes while JS is running, then the original value of the field should be marked. This checks the correctness of write barriers.
2. Any object that is reachable at the end of verification was reachable at the beginning, or else it was marked. This is mostly to check that read barriers are invoked correctly.

In this test, the read barrier causes us to mark the original TypeObject that we fetch out of the Type. But we don't transitively mark everything reachable from it. In a real GC that would happen, but the verifier doesn't mimic the GC closely enough. It just notices that that the object that was found via the newScript->fun access from the type object is reachable and unmarked, so it asserts.

To really check #2 above, I should be transitively marking forward from everything marked via a read barrier. Then the assertion would pass. However, this is kind of a pain to do. So I'd like to just remove the #2 assertions. They've been the cause of a lot of false positives, including bug 729571, which is still outstanding.

Brian, if you don't want to review this, I can give it to Igor. It's really just removing a bogus assert, though.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a3737f41b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f8a9d335fe

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/f6f8a9d335fe
https://hg.mozilla.org/mozilla-central/rev/3a3737f41b41

Comment 6

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 7

[Daniel Veditz [:dveditz]]

How far back does this problem go? Does it affect the ESR-10 branch?

I see a reference in the patch to IsIncrementalGCSafe() so maybe this is all new code? Or maybe only that part of it?

Comment 8

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

This only affect incremental GC, which is still disabled by default.

Comment 9

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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