684947 - "Assertion failure: thing->compartment() == rt->gcCheckCompartment || thing->compartment() == rt->atomsCompartment,"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

(evalcx(''))[''] = Proxy.create((function(x) {
  return {
    r: function() {
      for (e in x) {}
    }
  }
})(new(function() {
  yield
})("", /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

[Brian Hackett [Laid off!]]

What platform is this?  I can't repro.

Comment 2

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

(In reply to Brian Hackett from comment #1)
> What platform is this?  I can't repro.

Mac OS X 10.6.

Comment 3

[David Mandelin [:dmandelin]]

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

[Brian Hackett [Laid off!]]

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

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

Attachment: patch

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

[Luke Wagner [:luke]]

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

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

whodathunkit

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e872ae00cdc

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e872ae00cdc
https://hg.mozilla.org/mozilla-central/rev/a0eade7fa486

Comment 9

[u279076]

qa- as nothing to do for QA fix verification -- please set to qa+ if this is not the case.

Comment 10

[LegNeato]

Do we need this ported to Fx9? Looks like we shipped with this in Fx8 to no (major) ill effects...

Comment 11

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

This is a bad assertion, so it won't have any effect on release builds.