Last Comment Bug 755186 - add JSCompartment::global
: add JSCompartment::global
Status: RESOLVED FIXED
[js:p2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: cpg 789647
Blocks: 687724 762091 765873
  Show dependency treegraph
 
Reported: 2012-05-14 22:09 PDT by Luke Wagner [:luke]
Modified: 2012-09-07 19:05 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm JS_NewGlobalObject (15.42 KB, patch)
2012-06-18 11:56 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review
s/JS_NewCompartmentAndGlobalObject/JS_NewGlobalObject/ (15.92 KB, patch)
2012-06-18 11:57 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review
add JSCompartment::global() and assert that it commutes with the parent chain (6.02 KB, patch)
2012-06-18 12:03 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-05-14 22:09:28 PDT
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 [:njn] 2012-06-03 16:56:50 PDT
Luke: is this on your radar?  If not, how hard is it?
Comment 2 Luke Wagner [:luke] 2012-06-03 21:33:53 PDT
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] 2012-06-18 11:56:40 PDT
Created attachment 634125 [details] [diff] [review]
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] 2012-06-18 11:57:30 PDT
Created attachment 634126 [details] [diff] [review]
s/JS_NewCompartmentAndGlobalObject/JS_NewGlobalObject/
Comment 5 Luke Wagner [:luke] 2012-06-18 12:03:25 PDT
Created attachment 634130 [details] [diff] [review]
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] 2012-06-22 12:29:02 PDT
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] 2012-06-22 12:42:17 PDT
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] 2012-06-22 17:52:42 PDT
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

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