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.

Last one was borked. Pushed again: https://tbpl.mozilla.org/?tree=Try&rev=ea6f0e48fe29 And it's green.

It doesn't do anything useful at this point.

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.

This can happen after the fact where it needs to.

This will allow us to stop making nsXULPrototypeDocument and nsXBLDocumentInfo globals XPConnect globals.

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.

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.

\o/

A lot of these flags don't make sense in this context.

This will let us rip out that flag.

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.

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); {}

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?

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.

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?

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!

Comment on attachment 669248 [details] [diff] [review] Part 13 - Remove FindInJSObjectScope and friends. v1 Review of attachment 669248 [details] [diff] [review]: ----------------------------------------------------------------- \o/

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).

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?

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.

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.

Comment on attachment 669254 [details] [diff] [review] Part 19 - Remove the XPConnect Compartment Set. v1 Review of attachment 669254 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

(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. :-)

> 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).

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

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

https://hg.mozilla.org/mozilla-central/rev/a20072899a97 https://hg.mozilla.org/mozilla-central/rev/46dfef5f70f8 https://hg.mozilla.org/mozilla-central/rev/01a59c65c8b8 https://hg.mozilla.org/mozilla-central/rev/4bdc434c6d6e https://hg.mozilla.org/mozilla-central/rev/2a49f864998f https://hg.mozilla.org/mozilla-central/rev/ed27c3a1eca3 https://hg.mozilla.org/mozilla-central/rev/baf3468777a0 https://hg.mozilla.org/mozilla-central/rev/f7bfef3cd5e4 https://hg.mozilla.org/mozilla-central/rev/1a4fb31ef6a8 https://hg.mozilla.org/mozilla-central/rev/23d2c7d33c79 https://hg.mozilla.org/mozilla-central/rev/9c24c122583b https://hg.mozilla.org/mozilla-central/rev/f060ac1febfb https://hg.mozilla.org/mozilla-central/rev/847b5f4208be https://hg.mozilla.org/mozilla-central/rev/884c4954bbb3 https://hg.mozilla.org/mozilla-central/rev/d93c7a10520c https://hg.mozilla.org/mozilla-central/rev/7f16e5250101 https://hg.mozilla.org/mozilla-central/rev/a2e844887084 https://hg.mozilla.org/mozilla-central/rev/a2b8c01bcbbd https://hg.mozilla.org/mozilla-central/rev/dcd3f731be3d https://hg.mozilla.org/mozilla-central/rev/b011b2c888d1 https://hg.mozilla.org/mozilla-central/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