The default bug view has changed. See this FAQ.

add JSCompartment::global

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p2])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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?
(Assignee)

Comment 2

5 years ago
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.)
(Assignee)

Updated

5 years ago
Blocks: 762091
(Assignee)

Comment 3

5 years ago
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).
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #634125 - Flags: review?(jorendorff)
(Assignee)

Comment 4

5 years ago
Created attachment 634126 [details] [diff] [review]
s/JS_NewCompartmentAndGlobalObject/JS_NewGlobalObject/
Attachment #634126 - Flags: review?(jorendorff)
(Assignee)

Comment 5

5 years ago
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
Attachment #634130 - Flags: review?(jorendorff)
Blocks: 765873
(Assignee)

Updated

5 years ago
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

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?
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+
(Assignee)

Comment 8

5 years ago
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
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/68c396f305f4
https://hg.mozilla.org/mozilla-central/rev/6966d9832704
https://hg.mozilla.org/mozilla-central/rev/80b8680bda1c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 770407
(Assignee)

Updated

5 years ago
No longer depends on: 770407
Depends on: 789647
You need to log in before you can comment on or make changes to this bug.