1017310 - Associate JS compartments with add-on chrome XBL

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I agreed to spin this off from bug 990729. The goal here is to get add-on XBL code that's not running in content to be in a separate compartment.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: xbl-scope-renaming

This patch renames some things to make it clear that when the code talks about XBL scopes, it really means content XBL scopes.

I left alone some stuff. We're going to be messing around with GetXBLScopeOrGlobal in this bug, so I left the name alone. I also left alone AllowXBLScope since it has a fairly specific meaning related to remote XUL.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8430453 [details] [diff] [review]
xbl-scope-renaming

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

r=me once we sort that out.

::: js/xpconnect/src/xpcprivate.h
@@ +1161,5 @@
>      // This distinction is useful primarily because, if true, we know that we
>      // have no way of distinguishing XBL script from content script for the
>      // given scope. In these (unsupported) situations, we just always claim to
>      // be XBL.
>      bool mAllowXBLScope;

I think this should also be mAllowContentXBLScope, and don't quite follow the reasoning in comment 1. Can you elaborate?

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c9bbbb85c1

I pushed this early so it doesn't rot. I made the change to AllowXBLScope as well. I don't remember what my original rationale was for not changing it.

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1c9bbbb85c1

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: xbl

I think this incorporates all the feedback from a few weeks ago.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8437336 [details] [diff] [review]
xbl

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

Can you add comments to the remaining calls to GetXBLScopeOrGlobal explaining why we don't need to worry about addon scopes in those places? I've audited them and it looks good, but we should verbosely document this (especially since the original conversation happened IRL, and isn't in a bug anywhere).

Looks good otherwise. r=bholley

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +290,5 @@
> +
> +    RootedObject scope(cx);
> +    if (nativeScope->UseContentXBLScope())
> +        scope = nativeScope->EnsureContentXBLScope(cx);
> +    else if (addonId && CompartmentPerAddon())

There are cases (like remote XUL) where UseContentXBLScope() is true, but we're not chrome either. So this should be:

else if (addonId && CompartmentPerAddon() && AccessCheck::isChrome(contentScope))

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: comments

I'm still pretty unfamiliar with this code. Could you please make sure these comments make sense Bobby?

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8444019 [details] [diff] [review]
comments

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

::: dom/bindings/BindingUtils.h
@@ +1403,5 @@
>      return parent;
>    }
>  
>    // If useXBLScope is true, it means that the canonical reflector for this
> +  // native object should live in the XBL scope. Note that we never put

I'd say "content XBL scope" here, given the new lingo. And IIUC, we never have addon scopes for content, right?

::: dom/xbl/nsBindingManager.cpp
@@ +640,5 @@
>        // content in order to view the full array of methods defined in the
>        // binding, some of which may not be exposed on the prototype of
> +      // untrusted content. We don't need to consider add-on scopes here
> +      // because they don't expose any more methods than the window
> +      // scope--they're chrome-only.

This code only applies to content XBL scopes, which we don't have for addons, right? For the chrome case it never matters, because everything is System Principal and there are never any XrayWrappers involved.

::: dom/xbl/nsXBLBinding.cpp
@@ +924,5 @@
>                                                       : "__XBLClassObjectMap__";
>  
>    // Now, enter the XBL scope, since that's where we need to operate, and wrap
> +  // the proto accordingly. Note that we don't hang the class objects off add-on
> +  // scopes; instead we just use the normal window scope.

Maybe just say that we hang the map off of the content XBL scope for content, and the Window for chrome (addon or not)?

@@ +1121,5 @@
>    // LookupMember, we do a release-mode assertion as belt-and-braces.
>    // We do a release-mode assertion here to be extra safe.
> +  //
> +  // This code is only called for content XBL, so we don't have to worry about
> +  // add-on scopes here.

Can you replace this with a MOZ_RELEASE_ASSERT against this condition?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: comments v2

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8444755 [details] [diff] [review]
comments v2

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

r=bholley. Thanks for following up on this.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f446926ce971

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f446926ce971