Closed Bug 1017310 Opened 5 years ago Closed 5 years ago

Associate JS compartments with add-on chrome XBL

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
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.
Attachment #8430453 - Flags: review?(bobbyholley)
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?
Attachment #8430453 - Flags: review?(bobbyholley) → review+
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.
Keywords: leave-open
Attached patch xblSplinter Review
I think this incorporates all the feedback from a few weeks ago.
Attachment #8437336 - Flags: review?(bobbyholley)
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))
Attachment #8437336 - Flags: review?(bobbyholley) → review+
Attached patch comments (obsolete) — Splinter Review
I'm still pretty unfamiliar with this code. Could you please make sure these comments make sense Bobby?
Attachment #8444019 - Flags: review?(bobbyholley)
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?
Attachment #8444019 - Flags: review?(bobbyholley)
Attached patch comments v2Splinter Review
Attachment #8444019 - Attachment is obsolete: true
Attachment #8444755 - Flags: review?(bobbyholley)
Comment on attachment 8444755 [details] [diff] [review]
comments v2

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

r=bholley. Thanks for following up on this.
Attachment #8444755 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/f446926ce971
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.