Attach XPCWrappedNativeScope directly to the compartment private

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(19 attachments)

9.67 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.74 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
9.96 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.56 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.22 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.92 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.09 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.37 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.78 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
15.41 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.42 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
18.10 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.74 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.43 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.73 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
2.02 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
21.01 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.46 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
Assignee

Description

7 years ago
Currently we sometimes do a linear lookup of scopes, which is awful, and known to cause performance problems. With CPG, we can make this much much simpler and faster.
Assignee

Comment 2

7 years ago
Last one was borked. Pushed again:

https://tbpl.mozilla.org/?tree=Try&rev=ea6f0e48fe29

And it's green.
Assignee

Comment 4

7 years ago
It doesn't do anything useful at this point.
Attachment #669233 - Flags: review?(mrbkap)
Assignee

Comment 6

7 years ago
The compartment set goes away in later patches anyway. This is to allow us to
create compartment privates for things like XUL prototype document globals
without having to put them in the set and trace expandos and such.
Attachment #669235 - Flags: review?(mrbkap)
Assignee

Comment 7

7 years ago
This can happen after the fact where it needs to.
Attachment #669237 - Flags: review?(mrbkap)
Assignee

Comment 8

7 years ago
This will allow us to stop making nsXULPrototypeDocument and nsXBLDocumentInfo
globals XPConnect globals.
Attachment #669238 - Flags: review?(mrbkap)
Assignee

Comment 9

7 years ago
We already weren't creating a scope for them. They're generally pretty isolated
and are just used for holding functions that get cloned into other scopes.
Apparently mccr8 once found an edge from an nsXULPrototypeDocument scope into
another scope, but I don't think that should really break anything.
Attachment #669240 - Flags: review?(mrbkap)
Assignee

Comment 12

7 years ago
xpc_NewSystemInheritingJSObject is totally unnecessary now that the system bit
has been removed (\o/). Furthermore, the mJSPrototypeObject optimization is
really dumb. it complicates tracing significantly, and we don't actually use it
in any critical places: XPCWrappedNative and slim wrapper creation use a different
prototype, so this is used only for the creation of tearoff reflectors (seldom/
never used), XPCWrappedNativeProto objects, and the nohelper prototype on the
scope (once per scope).

We could actually just pass NULL to JS_NewObject and let it deal. However, this
would actually trigger a dynamic lookup for the prototype object of the
associated JSClass, which isn't what we want. So we just explicitly pass in
Object.prototype.
Attachment #669243 - Flags: review?(mrbkap)
Assignee

Comment 16

7 years ago
A lot of these flags don't make sense in this context.
Attachment #669249 - Flags: review?(mrbkap)
Assignee

Comment 20

7 years ago
This change means we no longer have to keep around a set of XPConnect compartments.
We keep the compartment private around for non-xpconnecty stuff like about:memory
instrumentation that needs to happen on non-xpconnect compartments.
Attachment #669253 - Flags: review?(mrbkap)
Read these patches while offline, so you get all my comments at once:

https://hg.mozilla.org/try/rev/a9f64e38090d

    2.21 +    if (!newGlob)
    2.22 +        return NS_OK;;

Double semicolon.
(Fixed in a later patch.)

    3.15 -    rv = xpc::CreateGlobalObject(cx, &SandboxClass, principal,
    3.16 -                                 options.wantXrays, &sandbox, &compartment);
    3.17 +    sandbox = xpc::CreateGlobalObject(cx, &SandboxClass, principal, options.wantXrays);
    3.18 +    if (!sandbox)
    3.19 +        return NS_ERROR_FAILURE;
    3.20      NS_ENSURE_SUCCESS(rv, rv);

Should remove the "NS_ENSURE_SUCCESS(rv, rv);".

    5.17 +    JSObject *global =  xpc::CreateGlobalObject(ccx, clasp, principal, false);

Nit: double space.

https://hg.mozilla.org/try/rev/74ff4fc23e6d

    1.24 +    if (set.has(compartment))
    1.25 +        set.remove(compartment);

No need for the if, js::HashSet appears perfectly happy if you vall remove() with something that isn't in the set.

https://hg.mozilla.org/try/rev/e4291b01f47f

    5.27 +    // Our XPCWrappedNativeScope. This is non-null if and only if this is an
    5.28 +    // XPConnect compartment.
    5.29 +    XPCWrappedNativeScope *scope;

It's not at all obvious to me how this invariant is enforced :/

     2.8      // Create the global.
     2.9      JSObject *global =  xpc::CreateGlobalObject(ccx, clasp, principal);
    2.10      if (!global)
    2.11          return NS_ERROR_FAILURE;
    2.12 +    XPCWrappedNativeScope *scope = GetCompartmentPrivate(global)->scope;

https://hg.mozilla.org/try/rev/396bd68d6911

At least in Gecko, ObjectScope() implies that it never returns null; if it can return null, it would be called GetObjectScope(). (Also, the commit message already claims it's called GetObjectScope().)

At the very least, you need to document when it can be called, and when it'll return null. (Looks like we'll crash if we don't have a compartment private?)

    6.17 +    if (!priv->scope)
    6.18          priv->scope = new XPCWrappedNativeScope(cx, aGlobal);
    6.19 -        scope = priv->scope;
    6.20 -    }
    6.21      else {

You've got a braced else, keep the braces for the if branch.

https://hg.mozilla.org/try/rev/83f96832a459

    2.21 +        if (scope->mWaiverWrapperMap)
    2.22 +          scope->mWaiverWrapperMap->Reparent(cx, newInnerWindow->mJSObject);

{}
Attachment #669232 - Flags: review?(mrbkap) → review+
Attachment #669233 - Flags: review?(mrbkap) → review+
Attachment #669234 - Flags: review?(mrbkap) → review+
Attachment #669235 - Flags: review?(mrbkap) → review+
Comment on attachment 669237 [details] [diff] [review]
Part 5 - Stop passing wantXrays directly to CreateGlobalObject. v1

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

It would be nice to have some sort of protection against someone settings wantsXrays after we've already started running code against a compartment. Do you think it's worth adding an assertion?
Attachment #669237 - Flags: review?(mrbkap) → review+
Comment on attachment 669238 [details] [diff] [review]
Part 6 - Allow compartment privates to be lazily created when using them just for storing about:memory URIs. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +211,5 @@
>      return true;
>  }
>  
> +namespace xpc {
> +CompartmentPrivate::~CompartmentPrivate()

Extra newline above the function, please.
Attachment #669238 - Flags: review?(mrbkap) → review+
Attachment #669240 - Flags: review?(mrbkap) → review+
Attachment #669241 - Flags: review?(mrbkap) → review+
Attachment #669242 - Flags: review?(mrbkap) → review+
Attachment #669243 - Flags: review?(mrbkap) → review+
Comment on attachment 669244 [details] [diff] [review]
Part 11 - Create XPCWrappedNativeScopes immediately after global creation. v1

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

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +88,5 @@
>      XPCWrappedNativeScope* scope = FindInJSObjectScope(cx, aGlobal, true);
> +    if (!scope) {
> +        CompartmentPrivate *priv = EnsureCompartmentPrivate(aGlobal);
> +        priv->scope = new XPCWrappedNativeScope(cx, aGlobal);
> +        scope = priv->scope;

Don't we want to do this every time we create a wrapped native scope? If so, I think the creation/setting priv->scope should be in a helper.

::: js/xpconnect/src/xpcprivate.h
@@ +4323,5 @@
>  
>      CompartmentPrivate()
>          : wantXrays(false)
>          , universalXPConnectEnabled(false)
> +        , scope(NULL)

nullptr?
Attachment #669244 - Flags: review?(mrbkap) → review+
Comment on attachment 669247 [details] [diff] [review]
Part 12 - Replace usage of XPCWrappedNativeScope::FindInJSObjectScope(ccx, obj) with GetObjectScope(obj). v1

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

I guess an object can have no scope if it comes from a non-xpconnect global? Bummer!
Attachment #669247 - Flags: review?(mrbkap) → review+
Comment on attachment 669248 [details] [diff] [review]
Part 13 - Remove FindInJSObjectScope and friends. v1

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

\o/
Attachment #669248 - Flags: review?(mrbkap) → review+
Comment on attachment 669249 [details] [diff] [review]
Part 14 - Don't pass XPCONNECT_GLOBAL_FLAGS for xbl and xul document gSharedGlobalClass. v1

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

I don't actually know what JSCLASS_IMPLEMENTS_BARRIERS means, but I assume that if it was OK before, it's OK now.

::: content/xbl/src/nsXBLDocumentInfo.cpp
@@ +166,5 @@
>  
>  JSClass nsXBLDocGlobalObject::gSharedGlobalClass = {
>      "nsXBLPrototypeScript compilation scope",
> +    JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS |
> +    JSCLASS_IMPLEMENTS_BARRIERS | JSCLASS_GLOBAL_FLAGS_WITH_SLOTS(0),

I'd prefer JSCLASS_GLOBAL_FLAGS to _WITH_SLOTS(0).
Attachment #669249 - Flags: review?(mrbkap) → review+
Comment on attachment 669250 [details] [diff] [review]
Part 15 - Decide whether we need to trace DOM stuff based on more relevant information than JSCLASS_XPCONNECT_GLOBAL. v1

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

r=me with my question answered.

::: js/src/jsapi.cpp
@@ +2210,5 @@
>      return &obj->global();
>  }
>  
> +extern JS_PUBLIC_API(JSBool)
> +JS_IsGlobalObject(JSRawObject obj)

Can't we simply check if obj has a parent rather than adding the new interface here?
Attachment #669250 - Flags: review?(mrbkap) → review+
Attachment #669251 - Flags: review?(mrbkap) → review+
Comment on attachment 669252 [details] [diff] [review]
Part 17 - Removed the unused reserved slot for XPConnect globals. v1

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

::: dom/bindings/DOMJSClass.h
@@ +15,5 @@
>  // globals and non-globals.
>  #define DOM_OBJECT_SLOT 0
>  
> +// All DOM globals must have a slot at DOM_PROTOTYPE_SLOT.
> +#define DOM_PROTOTYPE_SLOT (JSCLASS_GLOBAL_SLOT_COUNT)

Seems silly to keep the parens here.
Attachment #669252 - Flags: review?(mrbkap) → review+
Comment on attachment 669253 [details] [diff] [review]
Part 18 - Hoist XPConnect-y stuff out of the compartment private and into the XPCWrappedNativeScope. v1

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

::: js/xpconnect/src/dombindings.cpp
@@ +587,1 @@
>              return NULL;

While you're here, want to nullptr-ize this code?

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +100,1 @@
>                                     newMap(XPC_WRAPPER_MAP_SIZE);

Indentation seems funny here.
Attachment #669253 - Flags: review?(mrbkap) → review+
Comment on attachment 669254 [details] [diff] [review]
Part 19 - Remove the XPConnect Compartment Set. v1

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

Looks good!
Attachment #669254 - Flags: review?(mrbkap) → review+
Assignee

Comment 33

7 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #23)
> It would be nice to have some sort of protection against someone settings
> wantsXrays after we've already started running code against a compartment.
> Do you think it's worth adding an assertion?

IMO it's not a huge concern - it's all XPConnect-internal, and I think randomly flipping bits on the compartment private is scary enough that reviewers will notice. Also, the risk here isn't any different from the status quo.

> I guess an object can have no scope if it comes from a non-xpconnect global?
> Bummer!

Yeah, that's why we need the compartment private / XPCWNScope split. :-(

(In reply to Blake Kaplan (:mrbkap) from comment #28)
> >  JSClass nsXBLDocGlobalObject::gSharedGlobalClass = {
> >      "nsXBLPrototypeScript compilation scope",
> > +    JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS |
> > +    JSCLASS_IMPLEMENTS_BARRIERS | JSCLASS_GLOBAL_FLAGS_WITH_SLOTS(0),
> 
> I'd prefer JSCLASS_GLOBAL_FLAGS to _WITH_SLOTS(0).

Aesthetically yes, but _WITH_SLOTS(0) is actually necessary in order to reserve all the implicit global slots. See the implementation in jsapi.h.

> > +extern JS_PUBLIC_API(JSBool)
> > +JS_IsGlobalObject(JSRawObject obj)
> 
> Can't we simply check if obj has a parent rather than adding the new
> interface here?

Yeah, but I was getting tired of that. That JS_IsGlobalObject(obj) == !JS_GetParent(obj) is clear to a SpiderMonk, but less so to newcomers, and even SpiderMonks have to do an extra mental step to translate it. More to the point, JS parents are going away entirely, so we'll need this API eventually anyway. ;-)

> ::: js/xpconnect/src/dombindings.cpp
> @@ +587,1 @@
> >              return NULL;
> 
> While you're here, want to nullptr-ize this code?

This file is not long for this earth. Actually, it appears to be gone already. Time to rebase. :-)
Assignee

Comment 34

7 years ago
>     5.27 +    // Our XPCWrappedNativeScope. This is non-null if and only if
> this is an
>     5.28 +    // XPConnect compartment.
>     5.29 +    XPCWrappedNativeScope *scope;
> 
> It's not at all obvious to me how this invariant is enforced :/

We always set up the pointer when creating an XPCWrappedNativeScope, and the compartment private and XPCWrappedNativeScope die at the same time (during the GC finalization phase after the global is collected).

 
> At least in Gecko, ObjectScope() implies that it never returns null; if it
> can return null, it would be called GetObjectScope(). (Also, the commit
> message already claims it's called GetObjectScope().)

Yeah, I wanted to call it GetScope, but that conflicted with XPCWrappedNative::GetScope. I'll rename it to GetObjectScope like you suggest.

> At the very least, you need to document when it can be called, and when
> it'll return null. (Looks like we'll crash if we don't have a compartment
> private?)

Documented. Returns null if and only if the object is from a non-xpconnect compartment. It's up to the callers to decide whether that might happen (and consequently whether a null check is warranted).

I've addressed the rest of the comments in a patch on top of the massive stack (to save rebasing pain).
Assignee

Comment 36

7 years ago
One stupid mismerge caused orange on m-4 and xpcshell. I've fixed the bug and verified locally. Theoretically, there could be other orange later in the m-4 of xpcshell runs that was hidden by the crashes, but given that this had a green try run in comment 2 (before addressing review comments), that seems unlikely. Pushing to inbound.

Note - if backing this out, for the love of god please don't try to do it patch by patch [1]. ;-)


https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b011b2c888d1

http://ehsanakhgari.org/blog/2010-09-09/backing-out-multiple-consecutive-changesets-mercurial
Assignee

Comment 37

7 years ago
Looks like the lack of a null check for the argument of JS_IsGlobalObject is causing intermittent m-o orange. Pushed a followup fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a09013468b3e
I landed a very tiny part of this (JS_IsGlobalObject) on beta so that I could land some other patches I have that depend on it.

https://hg.mozilla.org/releases/mozilla-beta/rev/b438aea194f7
You need to log in before you can comment on or make changes to this bug.