997987 - Simplify the path to getting a subject principal

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Bobby Holley (:bholley)]

If somebody does nsContentUtils::GetSubjectPrincipal, the minimal path to the actual work is the following:

nsContentUtils::GetSubjectPrincipal()
nsScriptSecurityManager::GetSubjectPrincipal(nsIPrincipal **)
nsScriptSecurityManager::doGetSubjectPrincipal(nsresult *)
nsScriptSecurityManager::GetSubjectPrincipal(JSContext *, nsresult *)

That's not even including the refcounting, and the branches and additional calls in the case that the ssm decides to return null.

I'm going to clean this up and put everything in nsContentUtils, where it's easy to use.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Stop returning null to indicate "system" in nsScriptSecurityManager. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Remove nsIScriptSecurityManager::GetCxSubjectPrincipal. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove nsIScriptSecurityManager::GetSubjectPrincipal. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Cache the system principal on nsContentUtils and remove nsIScriptSecurityManager::SubjectPrincipalIsSystem. v1

Comment 7

[Bobby Holley (:bholley)]

We can also get bz to sr the whole thing.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8408759 [details] [diff] [review]
Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v1

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

This should really be the first patch in the series, before changing nsIScriptSecurityManager::GetSubjectPrincipal's behaviour.

We change behaviour in so many places, though, that I don't want to sign off on this, sorry.

::: caps/src/nsScriptSecurityManager.cpp
@@ +337,5 @@
>  
>  ///////////////// Security Checks /////////////////
>  
>  bool
>  nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx)

Assertion about the cx being current and/or remove the argument.

@@ +466,5 @@
>              (aFirst->GetIsInBrowserElement() == aSecond->GetIsInBrowserElement()));
>  }
>  
>  NS_IMETHODIMP
>  nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI)

Assertion about the cx being current and/or remove the argument.

@@ +469,5 @@
>  NS_IMETHODIMP
>  nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI)
>  {
>      // Get principal of currently executing script.
>      nsresult rv;

Move this where's it's first assigned

@@ +1088,5 @@
>  
>      //-- Access denied, report an error
>      NS_ConvertUTF8toUTF16 strName("CreateWrapperDenied");
>      nsAutoCString origin;
>      nsresult rv2;

Move down (and make it 'rv', and remove the confused comment about why it'd be 'rv2').

::: content/base/src/DOMParser.cpp
@@ -459,5 @@
>    if (!cx) {
>      rv.Throw(NS_ERROR_UNEXPECTED);
>      return;
>    }
> -

Leave this

::: content/base/src/nsDocument.cpp
@@ -6347,5 @@
> -
> -  if (!subject) {
> -    // Fall back to our principal.  Or should we fall back to the null
> -    // principal?  The latter would just mean no binding loads....
> -    subject = NodePrincipal();

This looks like a behaviour change.

::: content/html/document/src/nsHTMLDocument.cpp
@@ +2848,5 @@
>  
>  void
>  nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv)
>  {
> +  if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) {

Can this happen?

@@ +2849,5 @@
>  void
>  nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv)
>  {
> +  if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) {
> +      rv.Throw(NS_ERROR_DOM_PROP_ACCESS_DENIED);

Two-space

::: docshell/base/nsDocShell.cpp
@@ -8670,5 @@
> -    // the parent before allowing it to load anything into this
> -    // docshell.
> -    nsCOMPtr<nsIPrincipal> subjPrincipal;
> -    rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjPrincipal));
> -    NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal, rv);

So this would return for system principals before? Should it keep doing that?

@@ +8679,5 @@
>          }
>  
> +        if (nsContentUtils::GetSubjectPrincipal()->Subsumes(p)) {
> +            // Same origin, permit load
> +            return NS_OK;;

;;

::: dom/base/nsGlobalWindow.cpp
@@ +6039,5 @@
>    // Try to get a host from the running principal -- this will do the
>    // right thing for javascript: and data: documents.
>  
>    nsresult rv = NS_OK;
> +  nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal();

Behaviour change if called without a current context, I think

@@ -11690,5 @@
>    // Note the direction of this test: We don't allow setTimeouts running with
>    // chrome privileges on content windows, but we do allow setTimeouts running
>    // with content privileges on chrome windows (where they can't do very much,
>    // of course).
> -  rv = ourPrincipal->Subsumes(subjectPrincipal, &subsumes);

What does this do when called with null?

::: dom/src/storage/DOMStorage.cpp
@@ -240,5 @@
>      return false;
>    }
>  
>    // chrome can always use aStorage regardless of permission preferences
> -  if (nsContentUtils::IsCallerChrome()) {

Why change this?

@@ +278,5 @@
>      }
>    }
>  
>    if (aStorage) {
> +    return aStorage->CanAccess(subject);

Name change seems pointless

::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
@@ +785,5 @@
>  
>    // Now we have to set the right opener principal on the new window.  Note
>    // that we have to do this _before_ starting any URI loads, thanks to the
> +  // sync nature of javascript: loads.
> +  nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::GetSubjectPrincipal();

There's a null-check below

::: layout/style/nsCSSStyleSheet.cpp
@@ -1632,5 @@
> -  nsresult rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
> -  NS_ENSURE_SUCCESS(rv, rv);
> -
> -  if (!subjectPrincipal) {
> -    return NS_ERROR_DOM_SECURITY_ERR;

The number of places where we throw exceptions for null is... interesting.

Comment 9

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #8)
> This should really be the first patch in the series, before changing
> nsIScriptSecurityManager::GetSubjectPrincipal's behaviour.

Well, my intention was to do all of the behavior change in one small patch, and then make the big patch a simple mass-conversion. This makes sense from a bisecting perspective, but you're right that it makes it harder to review. So I'm just going to ditch Part 1, and modify Part 3 to preserve the old semantics in all but one place. Hopefully you'll feel more comfortable reviewing this version, since I think Boris is pretty overloaded right now.

> ::: caps/src/nsScriptSecurityManager.cpp

> >  bool
> >  nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx)
> 
> Assertion about the cx being current and/or remove the argument.

Done.

> >  NS_IMETHODIMP
> >  nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI)
> 
> Assertion about the cx being current and/or remove the argument.

Done.

> >  nsScriptSecurityManager::CheckLoadURIFromScript(JSContext *cx, nsIURI *aURI)
> >  {
> >      // Get principal of currently executing script.
> >      nsresult rv;
> 
> Move this where's it's first assigned

Done.

> Move down (and make it 'rv', and remove the confused comment about why it'd
> be 'rv2').

Done.
 
> ::: content/base/src/DOMParser.cpp
> @@ -459,5 @@
> >    if (!cx) {
> >      rv.Throw(NS_ERROR_UNEXPECTED);
> >      return;
> >    }
> > -
> 
> Leave this

Done.

> ::: content/base/src/nsDocument.cpp
> @@ -6347,5 @@
> > -
> > -  if (!subject) {
> > -    // Fall back to our principal.  Or should we fall back to the null
> > -    // principal?  The latter would just mean no binding loads....
> > -    subject = NodePrincipal();
> 
> This looks like a behaviour change.

Great catch! I've fixed it to preserve the old semantics.

> ::: content/html/document/src/nsHTMLDocument.cpp
> @@ +2848,5 @@
> >  
> >  void
> >  nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv)
> >  {
> > +  if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) {
> 
> Can this happen?

I don't know, but I don't want to touch it in this patch.

> 
> @@ +2849,5 @@
> >  void
> >  nsHTMLDocument::SetDesignMode(const nsAString& aDesignMode, ErrorResult& rv)
> >  {
> > +  if (!nsContentUtils::GetSubjectPrincipal()->Subsumes(NodePrincipal())) {
> > +      rv.Throw(NS_ERROR_DOM_PROP_ACCESS_DENIED);
> 
> Two-space

Fixed.


> ::: docshell/base/nsDocShell.cpp
> @@ -8670,5 @@
> > -    // the parent before allowing it to load anything into this
> > -    // docshell.
> > -    nsCOMPtr<nsIPrincipal> subjPrincipal;
> > -    rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjPrincipal));
> > -    NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal, rv);
> 
> So this would return for system principals before? Should it keep doing that?

I'm pretty sure the old behavior is nonsensical, but I've fixed up the patch to behave the same way.

> > +        if (nsContentUtils::GetSubjectPrincipal()->Subsumes(p)) {
> > +            // Same origin, permit load
> > +            return NS_OK;;
> 
> ;;

Fixed.

> ::: dom/base/nsGlobalWindow.cpp
> @@ +6039,5 @@
> >    // Try to get a host from the running principal -- this will do the
> >    // right thing for javascript: and data: documents.
> >  
> >    nsresult rv = NS_OK;
> > +  nsCOMPtr<nsIPrincipal> principal = nsContentUtils::GetSubjectPrincipal();
> 
> Behaviour change if called without a current context, I think

Same as the above.


> 
> @@ -11690,5 @@
> >    // Note the direction of this test: We don't allow setTimeouts running with
> >    // chrome privileges on content windows, but we do allow setTimeouts running
> >    // with content privileges on chrome windows (where they can't do very much,
> >    // of course).
> > -  rv = ourPrincipal->Subsumes(subjectPrincipal, &subsumes);
> 
> What does this do when called with null?

If if ourPrincipal is System, it will return true. Otherwise it will return false. Luckily, those are the same semantics we'll get when null -> nsSystemPrincipal.

> 
> ::: dom/src/storage/DOMStorage.cpp
> @@ -240,5 @@
> >      return false;
> >    }
> >  
> >    // chrome can always use aStorage regardless of permission preferences
> > -  if (nsContentUtils::IsCallerChrome()) {
> 
> Why change this?

To avoid computing the subject principal twice.

> @@ +278,5 @@
> >      }
> >    }
> >  
> >    if (aStorage) {
> > +    return aStorage->CanAccess(subject);
> 
> Name change seems pointless

It just happened because I computed it freshly above, and then didn't need it below. I can fix it to shrink the diff I guess, though it'll force a line break.
 
> ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
> @@ +785,5 @@
> >  
> >    // Now we have to set the right opener principal on the new window.  Note
> >    // that we have to do this _before_ starting any URI loads, thanks to the
> > +  // sync nature of javascript: loads.
> > +  nsCOMPtr<nsIPrincipal> subjectPrincipal = nsContentUtils::GetSubjectPrincipal();
> 
> There's a null-check below

Fixed.

> 
> ::: layout/style/nsCSSStyleSheet.cpp
> @@ -1632,5 @@
> > -  nsresult rv = securityManager->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
> > -  NS_ENSURE_SUCCESS(rv, rv);
> > -
> > -  if (!subjectPrincipal) {
> > -    return NS_ERROR_DOM_SECURITY_ERR;
> 
> The number of places where we throw exceptions for null is... interesting.

Given the title of this function and how it's used, the old behavior is almost certainly a bug. So I'm not going to fix this one, though I can if you really want me to.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove nsIScriptSecurityManager::GetSubjectPrincipal. v2

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8408758 [details] [diff] [review]
Part 2 - Remove nsIScriptSecurityManager::GetCxSubjectPrincipal. v1

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

r=me. Do I own CAPS now?

::: content/base/src/nsContentUtils.cpp
@@ +6104,5 @@
>  nsContentUtils::GetContentSecurityPolicy(JSContext* aCx,
>                                           nsIContentSecurityPolicy** aCSP)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +  MOZ_ASSERT(aCx == GetCurrentJSContext());

Followup to remove the argument.

::: js/xpconnect/src/Sandbox.cpp
@@ +216,2 @@
>      if (NS_FAILED(rv))
>          return false;

(Is this an uncatchable exception?)

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8410641 [details] [diff] [review]
Part 3 - Remove usage of nsIScriptSecurityManager::GetSubjectPrincipal. v2

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

r=me, I think.

::: caps/src/nsScriptSecurityManager.cpp
@@ +337,5 @@
>  
>  ///////////////// Security Checks /////////////////
>  
>  bool
>  nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx)

Followup to remove the argument

@@ +391,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsScriptSecurityManager::CheckSameOrigin(JSContext* cx,
>                                           nsIURI* aTargetURI)

Assert !cx || cx == GetCurrentContext()

@@ +1104,5 @@
>      } else {
>          strName.AppendLiteral("ForOrigin");
>      }
>      nsXPIDLString errorMsg;
>      // We need to keep our existing failure rv and not override it

Remove the comment

::: content/base/src/DOMParser.cpp
@@ -400,5 @@
> -  if (rv.Failed()) {
> -    return nullptr;
> -  }
> -
> -  // We're called from JS; there better be a subject principal, really.

Assert that, perhaps?

::: content/base/src/nsContentUtils.cpp
@@ +1596,4 @@
>    nsCOMPtr<nsIScriptObjectPrincipal> scriptObject =
>      do_QueryInterface(aWindow->IsOuterWindow() ?
>                        aWindow->GetCurrentInnerWindow() : aWindow);
>    NS_ENSURE_TRUE(scriptObject, false);

Does it matter that this changes behaviour?

::: dom/base/nsGlobalWindow.cpp
@@ +6053,5 @@
>  
>    // Try to get a host from the running principal -- this will do the
>    // right thing for javascript: and data: documents.
>  
>    nsresult rv = NS_OK;

Move down

::: dom/events/DataTransfer.cpp
@@ +629,1 @@
>            NS_ENSURE_SUCCESS(rv, rv);

This is dead

::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp
@@ +1251,5 @@
>  NS_IMETHODIMP
>  txMozillaXSLTProcessor::Initialize(nsISupports* aOwner, JSContext* cx,
>                                     JSObject* obj, const JS::CallArgs& args)
>  {
> +    return Init(nsContentUtils::GetSubjectPrincipal());

Assert GetCurrentContext() is not null?

::: security/manager/ssl/src/nsCrypto.cpp
@@ -2050,5 @@
> -    aRv.Throw(nrv);
> -    return nullptr;
> -  }
> -  if (MOZ_UNLIKELY(!principals)) {
> -    aRv.Throw(NS_ERROR_UNEXPECTED);

Behaviour change... Maybe just assert we have a current context?

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8410644 [details] [diff] [review]
Part 4 - Remove nsIScriptSecurityManager::GetSubjectPrincipal. v2

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

r=me

::: caps/src/nsScriptSecurityManager.cpp
@@ -1038,5 @@
> -nsIPrincipal*
> -nsScriptSecurityManager::GetSubjectPrincipal(JSContext *cx,
> -                                             nsresult* rv)
> -{
> -    *rv = NS_OK;

§#?!@#

::: content/base/src/nsContentUtils.cpp
@@ +2334,5 @@
>  // static
>  nsIPrincipal*
>  nsContentUtils::GetSubjectPrincipal()
>  {
> +  JSContext *cx = GetCurrentJSContext();

* to the left (three times in this function)

@@ +2342,3 @@
>  
> +  JSCompartment *compartment = js::GetContextCompartment(cx);
> +  MOZ_ASSERT(!!compartment);

No need for the !!

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8408761 [details] [diff] [review]
Part 5 - Cache the system principal on nsContentUtils and remove nsIScriptSecurityManager::SubjectPrincipalIsSystem. v1

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

r=me. There's a bunch of places where we start supporting UniversalXPConnect and didn't before, though; I'd like bz to sign that off.

::: caps/src/nsScriptSecurityManager.cpp
@@ +78,5 @@
>  JSRuntime       *nsScriptSecurityManager::sRuntime   = 0;
>  bool nsScriptSecurityManager::sStrictFileOriginPolicy = true;
>  
>  bool
>  nsScriptSecurityManager::SubjectIsPrivileged()

Followup to remove?

::: content/base/src/nsContentUtils.cpp
@@ +380,5 @@
>      return NS_ERROR_FAILURE;
>    NS_ADDREF(sSecurityManager);
>  
> +  sSecurityManager->GetSystemPrincipal(&sSystemPrincipal);
> +  if (!sSystemPrincipal)

Assert

@@ +4392,5 @@
>    if (NS_SUCCEEDED((*aResourcePrincipal)->Subsumes(aExtraPrincipal, &subsumes)) &&
>        subsumes) {
>      return false;
>    }
> +  NS_ADDREF(*aResourcePrincipal = sSystemPrincipal);

Is this a leak? I think this is a leak.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +90,5 @@
>      return isChrome(js::GetObjectCompartment(obj));
>  }
>  
>  bool
>  AccessCheck::callerIsChrome()

Followup to remove?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8408761 [details] [diff] [review]
Part 5 - Cache the system principal on nsContentUtils and remove nsIScriptSecurityManager::SubjectPrincipalIsSystem. v1

Treating UniversalXPConnect as identical to system across the board makes sense to me.

Comment 17

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #12)
> Comment on attachment 8408758 [details] [diff] [review]
> Part 2 - Remove nsIScriptSecurityManager::GetCxSubjectPrincipal. v1
> 
> Review of attachment 8408758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Do I own CAPS now?
> 
> ::: content/base/src/nsContentUtils.cpp
> @@ +6104,5 @@
> >  nsContentUtils::GetContentSecurityPolicy(JSContext* aCx,
> >                                           nsIContentSecurityPolicy** aCSP)
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> > +  MOZ_ASSERT(aCx == GetCurrentJSContext());
> 
> Followup to remove the argument.

Filed bug 1006671.

> ::: js/xpconnect/src/Sandbox.cpp
> @@ +216,2 @@
> >      if (NS_FAILED(rv))
> >          return false;
> 
> (Is this an uncatchable exception?)

It depends on whether the XHR sets a JS exception or not.

Comment 18

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #13)
> > -  // We're called from JS; there better be a subject principal, really.
> 
> Assert that, perhaps?

With these changes it's moot. There will always be a subject principal whether we're called from JS or not.
 
> ::: content/base/src/nsContentUtils.cpp
> @@ +1596,4 @@
> >    nsCOMPtr<nsIScriptObjectPrincipal> scriptObject =
> >      do_QueryInterface(aWindow->IsOuterWindow() ?
> >                        aWindow->GetCurrentInnerWindow() : aWindow);
> >    NS_ENSURE_TRUE(scriptObject, false);
> 
> Does it matter that this changes behaviour?

Unless I'm misunderstanding you, I don't think it matters. CanCallerAccess will always return true for the System Principla.

> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +6053,5 @@
> >  
> >    // Try to get a host from the running principal -- this will do the
> >    // right thing for javascript: and data: documents.
> >  
> >    nsresult rv = NS_OK;
> 
> Move down

Done.

> ::: dom/events/DataTransfer.cpp
> @@ +629,1 @@
> >            NS_ENSURE_SUCCESS(rv, rv);
> 
> This is dead

Fixed.

> 
> ::: dom/xslt/xslt/txMozillaXSLTProcessor.cpp
> @@ +1251,5 @@
> >  NS_IMETHODIMP
> >  txMozillaXSLTProcessor::Initialize(nsISupports* aOwner, JSContext* cx,
> >                                     JSObject* obj, const JS::CallArgs& args)
> >  {
> > +    return Init(nsContentUtils::GetSubjectPrincipal());
> 
> Assert GetCurrentContext() is not null?

Ok.

 
> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ -2050,5 @@
> > -    aRv.Throw(nrv);
> > -    return nullptr;
> > -  }
> > -  if (MOZ_UNLIKELY(!principals)) {
> > -    aRv.Throw(NS_ERROR_UNEXPECTED);
> 
> Behaviour change... Maybe just assert we have a current context?

Ok.

Comment 19

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #14)
> ::: caps/src/nsScriptSecurityManager.cpp
> @@ -1038,5 @@
> > -nsIPrincipal*
> > -nsScriptSecurityManager::GetSubjectPrincipal(JSContext *cx,
> > -                                             nsresult* rv)
> > -{
> > -    *rv = NS_OK;
> 
> §#?!@#

I don't understand this. But since it's in removed code, I'm assuming it doesn't matter.
 
> ::: content/base/src/nsContentUtils.cpp
> @@ +2334,5 @@
> >  // static
> >  nsIPrincipal*
> >  nsContentUtils::GetSubjectPrincipal()
> >  {
> > +  JSContext *cx = GetCurrentJSContext();
> 
> * to the left (three times in this function)

Fixed.

> 
> @@ +2342,3 @@
> >  
> > +  JSCompartment *compartment = js::GetContextCompartment(cx);
> > +  MOZ_ASSERT(!!compartment);
> 
> No need for the !!

Fixed.

Comment 20

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #15)
> >  bool
> >  nsScriptSecurityManager::SubjectIsPrivileged()

Filed bug 1006692.

> 
> Followup to remove?
> 
> ::: content/base/src/nsContentUtils.cpp
> > +  sSecurityManager->GetSystemPrincipal(&sSystemPrincipal);
> > +  if (!sSystemPrincipal)
> 
> Assert

Ok.

> 
> @@ +4392,5 @@
> >    if (NS_SUCCEEDED((*aResourcePrincipal)->Subsumes(aExtraPrincipal, &subsumes)) &&
> >        subsumes) {
> >      return false;
> >    }
> > +  NS_ADDREF(*aResourcePrincipal = sSystemPrincipal);
> 
> Is this a leak? I think this is a leak.

Good catch! I didn't see the bizarre COMPtr ptr. Fixed.

> ::: js/xpconnect/wrappers/AccessCheck.cpp
> @@ +90,5 @@
> >      return isChrome(js::GetObjectCompartment(obj));
> >  }
> >  
> >  bool
> >  AccessCheck::callerIsChrome()
> 
> Followup to remove?

Bug 1006692.

Comment 21

[Bobby Holley (:bholley)]

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

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bobby Holley (:bholley) from comment #18)
> (In reply to :Ms2ger from comment #13)
> > > -  // We're called from JS; there better be a subject principal, really.
> > 
> > Assert that, perhaps?
> 
> With these changes it's moot. There will always be a subject principal
> whether we're called from JS or not.

I was thinking to assert GetCurrentContext().

> > ::: content/base/src/nsContentUtils.cpp
> > @@ +1596,4 @@
> > >    nsCOMPtr<nsIScriptObjectPrincipal> scriptObject =
> > >      do_QueryInterface(aWindow->IsOuterWindow() ?
> > >                        aWindow->GetCurrentInnerWindow() : aWindow);
> > >    NS_ENSURE_TRUE(scriptObject, false);
> > 
> > Does it matter that this changes behaviour?
> 
> Unless I'm misunderstanding you, I don't think it matters. CanCallerAccess
> will always return true for the System Principla.

It won't in this overload of CanCallerAccess; if scriptObject and GetCurrentContext() are both null, we used to return true, and now return false. Probably doesn't matter, though.

Comment 23

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fd17d782f06a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1862e3f7ca6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ccbcc5e0e689
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d46b25e314d9

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd17d782f06a
https://hg.mozilla.org/mozilla-central/rev/d1862e3f7ca6
https://hg.mozilla.org/mozilla-central/rev/ccbcc5e0e689
https://hg.mozilla.org/mozilla-central/rev/d46b25e314d9