Closed Bug 748119 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox15 --- fixed
firefox-esr10 --- unaffected


(Reporter: decoder, Assigned: billm)



(Keywords: assertion, sec-critical, testcase, Whiteboard: js-triage-needed, [sg:critical][advisory-tracking+])


(3 files)

The attached test asserts on mozilla-central revision 17af008937e3 (options -m -n -a).
Assignee: general → wmccloskey
Attached patch TI patchSplinter Review
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.
Attachment #620919 - Flags: review?(bhackett1024)
Attached patch verifier patchSplinter Review
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.
Attachment #620922 - Flags: review?(bhackett1024)
Attachment #620919 - Flags: review?(bhackett1024) → review+
Attachment #620922 - Flags: review?(bhackett1024) → review+
JSBugMon: This bug has been automatically verified fixed.
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?
Keywords: sec-critical
Whiteboard: js-triage-needed → js-triage-needed, [sg:critical]
This only affect incremental GC, which is still disabled by default.
Depends on: 770268
No longer depends on: 770268
Whiteboard: js-triage-needed, [sg:critical] → js-triage-needed, [sg:critical][advisory-tracking+]
Group: core-security
Automatically extracted testcase for this bug was committed:
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.