684525 - "Assertion failure: shape->compartment() == compartment()," or "Assertion failure: compartment mismatched,"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function f(code) {
    g(code.replace(), "new-compartment")
}
function g(a, b) {
    evalcx(a, newGlobal(b))
}
f("\
  let([]=#0=*) {\
    eval(\"''.e()\")\
  }\
")

asserts js debug shell on m-c changeset 472716252ea3 with -m and -a and the patch in bug 683966 comment 1 at Assertion failure: compartment mismatched,

A variant,

function f(code) {
    g(code, "new-compartment")
}
function g(a, b) {
    evalcx(a, newGlobal(b))
}
f("\
  let([]=#0=*) {\
    eval(\"''.e()\")\
  }\
")

asserts at Assertion failure: shape->compartment() == compartment(),

Comment 1

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

I found a simpler testcase:

evalcx("\
  let([]=#0=*) {\
    eval(\"''.e()\")\
  }\
", newGlobal("new-compartment"));

I think this is a TI bug. Somehow script->where.global (which I think is new) is not set properly. It's in a different compartment from script, which seems bad.

Comment 2

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

Actually, it looks like the problem is here, in JSScript::NewScriptFromCG:

    if (script->compileAndGo) {
        GlobalScope *globalScope = cg->compiler()->globalScope;
        if (globalScope->globalObj && globalScope->globalObj->isGlobal())
            script->where.global = globalScope->globalObj->asGlobal();
        else if (cx->globalObject->isGlobal())
>>          script->where.global = cx->globalObject->asGlobal();
    }

My understanding is that we're not supposed to be relying on cx->globalObject in this way, but I could be wrong.

Comment 3

[Brian Hackett [Laid off!]]

Yeah, this is the problem.  What are the circumstances when cx->globalObject can be relied on for anything?  This is a TI bug --- TI needs to know about the global which compileAndGo scripts run against, and the parser does not always have this information available (no clue why).

This will get fixed with the next merge from JM (should be later today).  That makes computing the script's global lazier, not occurring until the script has executed at least once and we can determine the global for compileAndGo scripts from the scope chain.

Comment 4

[Luke Wagner [:luke]]

cx->globalObject is very dangerous b/c it is almost never correct to use it.  Pretend it was named cx->globalObjectIfNothingElseIsRunning.  A JSContext has no long-term association with any global object, so the only valid question one may ask is "what is the 'current' global object" via GetGlobalForScopeChain.  It sounds like this isn't what you want, though.

Comment 5

[Brendan Eich [:brendan]]

cx->globalObject should die. There's a bug on this, IINM.

Brian, re: comment 3: you should be able to get the object presented to the compiler as the compile-n-go global object, without any reliability problems. See compiler.globalScope init in js::Compiler::compileScript in jsparse.cpp.

/be

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Brendan Eich [:brendan] from comment #5)
> cx->globalObject should die. There's a bug on this, IINM.
> 
> Brian, re: comment 3: you should be able to get the object presented to the
> compiler as the compile-n-go global object, without any reliability
> problems. See compiler.globalScope init in js::Compiler::compileScript in
> jsparse.cpp.
> 
> /be

Yeah, but compiler.globalScope is not always set for compileAndGo scripts (or is set to a non-global object), that's the case where cx->globalObject was being incorrectly used instead.  (rev 554045e04d89 removes this stuff entirely, http://hg.mozilla.org/mozilla-central/diff/554045e04d89/js/src/jsscript.cpp)

Comment 7

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

Did this get fixed somehow? m-c changeset 09935ede3c77 seems to no longer assert with the testcases.

Comment 8

[Brian Hackett [Laid off!]]

Yeah, a fix for this came in as part of rev 554045e04d89, on a merge from JM.