Closed Bug 755186 Opened 8 years ago Closed 8 years ago

add JSCompartment::global


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: luke)



(Whiteboard: [js:p2])


(3 files)

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.
Whiteboard: [js:p2]
Luke: is this on your radar?  If not, how hard is it?
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.)
Blocks: 762091
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).
Assignee: general → luke
Attachment #634125 - Flags: review?(jorendorff)
(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
Attachment #634130 - Flags: review?(jorendorff)
Blocks: 765873
Summary: add JSCompartment::globalObject → add JSCompartment::global
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


@@ +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?
Attachment #634125 - Flags: review?(jorendorff) → review+
Attachment #634126 - Flags: review?(jorendorff) → review+
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?
Attachment #634130 - Flags: review?(jorendorff) → review+
Agreed on all counts (removed the whole -P case in xpcshell, changed the gc-05.js debug test to not use proto).
Target Milestone: --- → mozilla16
Depends on: 770407
No longer depends on: 770407
Depends on: 789647
You need to log in before you can comment on or make changes to this bug.