755186 - add JSCompartment::global

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

With c-p-g we can unambiguously add JSCompartment::globalObject.  This has many uses.  The initial patch should add the field and assert commutativity invariants:
  obj->compartment()->globalObject() == obj->global()
  fp->compartment()->globalObject() == fp->global()
Later, we can redefine the rhs in terms of the lhs.

Comment 1

[Nicholas Nethercote [inactive]]

Luke: is this on your radar?  If not, how hard is it?

Comment 2

[Luke Wagner [:luke]]

It's not a hardness thing, it's a waiting-to-be-really-sure-that-cpg-is-100%-fully-stuck-so-we-don't-make-backout-harder thing.  (My fears are almost totally gone (bholley has continued to rock it), but, for the moment, I'm watching bug 757639 and bug 758415 and also waiting to see what happens on Aurora.)

Comment 3

[Luke Wagner [:luke]]

Attachment: rm JS_NewGlobalObject

This patch just removes JS_NewGlobalObject.  This forces newGlobal to always create a new compartment which required some minor fixes in debugger test-cases.  On actual compartment-transparency bug exposed was bug 764307 (see comment in Debugger.cpp).

Comment 4

[Luke Wagner [:luke]]

Attachment: s/JS_NewCompartmentAndGlobalObject/JS_NewGlobalObject/

Comment 5

[Luke Wagner [:luke]]

Attachment: add JSCompartment::global() and assert that it commutes with the parent chain

(Later we can remove JSObject::getParent altogether.)

With this last patch, we should always be able to know the global of any GC-thing via thing->compartment()->global().  Let the reaping begin.

Green on try

Comment 6

[Jason Orendorff [:jorendorff]]

Comment on attachment 634125 [details] [diff] [review]
rm JS_NewGlobalObject

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

r=me with the Debugger.cpp change reverted (and other minor comments).

::: js/src/jsapi-tests/testDebugger.cpp
@@ +211,5 @@
>           "dbg.onNewScript = function (s) {\n"
>           "    hits += Number(s instanceof Debugger.Script);\n"
>           "};\n");
>  
> +    // Since g is a debuggee, g1.eval should trigger newScript, regardless of

g.eval

@@ +217,4 @@
>      //
>      // (Not all scripts are permanently associated with specific global
>      // objects, but eval scripts are, so we deliver them only to debuggers that
>      // are watching that particular global.)

The disclaimer-y part of the comment is no longer true. It can just say:

// Scripts are associated with the global where they're compiled,
// so we deliver them only to debuggers that are watching that
// particular global.

@@ +224,1 @@
>      return ok;

Just return testIndirectEval(g, "Math.abs(0)", 1);
or inline it; anyway expectedHits doesn't need to be a parameter.

::: js/src/vm/Debugger.cpp
@@ +3626,5 @@
> +    Value protov;
> +    if (!refobj->isWrapper())
> +        protov = ObjectOrNullValue(refobj->getProto());
> +    else
> +        protov = ObjectOrNullValue(UnwrapObject(refobj)->getProto());

This change seems unrelated to everything else. Separate bug for sure.

I don't think the change is right as-is; unwrapping can cross compartment boundaries, so it seems like at least this would want to unwrap, getProto, then rewrap for refobj's compartment -- before applying wrapDebuggeeValue for the debugger.

This would make more sense as part of a broader fix for bug 764307, though, with the unwrap-getProto-rewrap part factored out into common code used by this, __proto__'s getter, and Object.getPrototypeOf.

::: js/xpconnect/shell/xpcshell.cpp
@@ +1201,5 @@
> +                JSAutoEnterCompartment ac;
> +                if (!ac.enter(cx, gobj))
> +                    return false;
> +
> +                if (!JS_SplicePrototype(cx, gobj, obj))

Is this going to work? obj needs to be wrapped, at least, right?

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 634130 [details] [diff] [review]
add JSCompartment::global() and assert that it commutes with the parent chain

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

I am so happy about this.

::: js/src/jscntxtinlines.h
@@ +228,5 @@
> +      : context(cx), compartment(cx->compartment)
> +    {
> +        if (cx->compartment) {
> +            GlobalObject *global = GetGlobalForScopeChain(cx);
> +            JS_ASSERT(cx->compartment->global() == *global);

/me sees operator==(const JSObject &, const JSObject &)

Huh. Well OK.

::: js/src/jscompartment.h
@@ +129,2 @@
>      JSRuntime                    *rt;
>      JSPrincipals                 *principals;

Micro-nit: Keep rt and principals at the top?

Comment 8

[Luke Wagner [:luke]]

Agreed on all counts (removed the whole -P case in xpcshell, changed the gc-05.js debug test to not use proto).

https://hg.mozilla.org/integration/mozilla-inbound/rev/68c396f305f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6966d9832704
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b8680bda1c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68c396f305f4
https://hg.mozilla.org/mozilla-central/rev/6966d9832704
https://hg.mozilla.org/mozilla-central/rev/80b8680bda1c