Last Comment Bug 684947 - "Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->compartment() == rt->atomsCompartment,"
: "Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->...
js-triage-done [qa-]
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
-- critical (vote)
: mozilla10
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: jsfunfuzz
  Show dependency treegraph
Reported: 2011-09-06 12:02 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2011-12-14 13:50 PST (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

stack (6.72 KB, text/plain)
2011-09-06 12:02 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
patch (1.60 KB, patch)
2011-09-26 12:29 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description User image Gary Kwong [:gkw] [:nth10sd] 2011-09-06 12:02:57 PDT
Created attachment 558546 [details]

(evalcx(''))[''] = Proxy.create((function(x) {
  return {
    r: function() {
      for (e in x) {}
})(new(function() {
})("", /x/)), (b = 6))
gc(function() {})

asserts js debug shell on m-c changeset db9e99d537f2 with the patch from bug 683966 with -m and -a at Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->compartment() == rt->atomsCompartment,

Even though this concerns evalcx, gc seems to be involved so locking s-s just to be safe.
Comment 1 User image Brian Hackett (:bhackett) 2011-09-08 11:19:39 PDT
What platform is this?  I can't repro.
Comment 2 User image Gary Kwong [:gkw] [:nth10sd] 2011-09-08 11:30:25 PDT
(In reply to Brian Hackett from comment #1)
> What platform is this?  I can't repro.

Mac OS X 10.6.
Comment 3 User image David Mandelin [:dmandelin] 2011-09-23 16:15:33 PDT
I think this one might be a bogus assert. We are asserting on a compartment check during GC mark. The object being marked is a sandbox, the compartment of which does not match the GC compartment. We get to that object through generator_trace, via this line of code:

     * Currently, generators are not mjitted. Still, (overflow) args can be
     * pushed by the mjit and need to be conservatively marked. Technically, the
     * formal args and generator slots are safe for exact marking, but since the
     * plan is to eventually mjit generators, it makes sense to future-proof
     * this code and save someone an hour later.
    MarkStackRangeConservatively(trc, gen->floatingStack, fp->formalArgsEnd());

I suspect we are conservatively marking something we don't have to mark, and getting to the sandbox that way.

Bill, can you check this out?
Comment 4 User image Brian Hackett (:bhackett) 2011-09-23 17:45:24 PDT
Wouldn't the bug be in the sandbox marking itself, then?  I don't see how conservatively marking any address range could itself be a correctness bug, since any word the conservative GC finds could also be some random integer value found in the area it needs to be looking at.
Comment 5 User image Bill McCloskey (:billm) 2011-09-26 12:29:40 PDT
Created attachment 562510 [details] [diff] [review]

Dave is right; this assertion is overzealous. Normally a per-compartment GC will never mark outside its compartment once drainMarkStack has begun. But since the generator code does conservative scanning, this invariant can be broken. The patch just skips the assertion during the conservative phase.

I'm not exactly sure how the sandbox pointer ends up on the JS stack. But it only happens with overflow arguments when we're in the mjit code. So I'm going to trust the comment in generator_trace that this is a valid thing to have happen.
Comment 6 User image Luke Wagner [:luke] 2011-09-26 14:00:44 PDT
Comment on attachment 562510 [details] [diff] [review]

Review of attachment 562510 [details] [diff] [review]:

Comment 9 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:12:09 PDT
qa- as nothing to do for QA fix verification -- please set to qa+ if this is not the case.
Comment 10 User image christian 2011-12-14 13:47:16 PST
Do we need this ported to Fx9? Looks like we shipped with this in Fx8 to no (major) ill effects...
Comment 11 User image Bill McCloskey (:billm) 2011-12-14 13:50:28 PST
This is a bad assertion, so it won't have any effect on release builds.

Note You need to log in before you can comment on or make changes to this bug.