998002 - Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

These are infallible, and used all over the place. This would make things nicer, and be clearer to boot.

Let's wait until bug 997987 lands to do this.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

How is GetSubjectPrincipal infallible? (In fact, it might crash currently if called too late
during shutdown process)

Comment 2

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #1)
> How is GetSubjectPrincipal infallible?

It never returns non-null. The patches in bug 997987 make this more explicit.

> (In fact, it might crash currently if
> called too late
> during shutdown process)

After bug 913138, that's not likely.

Comment 3

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal.

Renamed all occurrences of mentioned methods. But i left as is the methods with similar names which exist in nsScriptSecurityManager.h [1] and in xpconnect stuff, e.g. [2]. Please let me know if it is ok.  
Btw, i'll add try results soon.

[1] mxr.mozilla.org/mozilla-central/source/caps/include/nsScriptSecurityManager.h#122

[2] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#2429

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8420056 [details] [diff] [review]
Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal.

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

This all looks good! I'm cancelling review because I want to take a quick look at the new version with the fixes below, but it should be a quick r+ after that.

::: content/base/public/nsContentUtils.h
@@ +483,5 @@
>    // Returns the principal of the given JS object. This should never be null
>    // for any object in the XPConnect runtime.
>    //
>    // In general, being interested in the principal of an object is enough to
>    // guarantee that the return value is non-null.

Can you change this comment to:

// Returns the prinipal of the given JS object. This may only be called on
// the main thread for objects from the main thread's JSRuntime.

And add:

MOZ_ASSERT(NS_IsMainThread());
MOZ_ASSERT(JS_GetObjectRuntime(obj) = CycleCollectedJSRuntime::Get());

To the top of the implementation of nsContentUtils::ObjectPrincipal()?

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1387,4 @@
>      // is.
>      TimeDuration duration = TimeStamp::NowLoRes() - self->mSlowScriptCheckpoint;
>      bool chrome =
> +      nsContentUtils::IsSystemPrincipal(nsContentUtils::SubjectPrincipal());

Please replace this with:

bool chrome = nsContentUtils::IsCallerChrome();

Comment 5

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal.

Addressed review comments.

Comment 6

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principalrename_security_principal.

Fixed stupid type in the method nsContentUtils::ObjectPrincipal :)

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8420245 [details] [diff] [review]
Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principalrename_security_principal.

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

::: content/base/src/nsContentUtils.cpp
@@ +2332,5 @@
>  nsIPrincipal*
> +nsContentUtils::ObjectPrincipal(JSObject* aObj)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(JS_GetObjectRuntime(aObj) = CycleCollectedJSRuntime::Get());

Er, this should be ==, not =. Sorry for the typo in my earlier comment.

Did this compile? Assuming you're doing a debug build, I'd think it wouldn't...

Comment 8

[Nick Lebedev [:nl]]

yes, sorry, it doesn't compile. my bad.
Even after correcting = to == i'm seeing conversion compilation error: "error C2446: '==' : no conversion from 'mozilla::CycleCollectedJSRuntime *' to 'JSRuntime *'". Will investigate it and add new version of patch tomorrow.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Nick Lebedev [:nl] from comment #8)
> yes, sorry, it doesn't compile. my bad.
> Even after correcting = to == i'm seeing conversion compilation error:
> "error C2446: '==' : no conversion from 'mozilla::CycleCollectedJSRuntime *'
> to 'JSRuntime *'". Will investigate it and add new version of patch tomorrow.

Oh yeah, sorry. It should be CycleCollectedJSRuntime::Get()->Runtime().

Comment 10

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal. v2.

Fixed compilation error.

Comment 11

[Bobby Holley (:bholley)]

Nice!

Comment 12

[Carsten Book [:Tomcat]]

Hi,

could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 13

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

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

Comment 14

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

Looks like ObjectPrincipal() is called off the main thread: https://tbpl.mozilla.org/php/getParsedLog.php?id=39472593&tree=Try

Comment 15

[Bobby Holley (:bholley)]

Hm. Luke, what's the deal here? Is this intentional?

Comment 16

[Luke Wagner [:luke]]

Yep, we have an object and need its principal and we're parsing off the main thread.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #16)
> Yep, we have an object and need its principal and we're parsing off the main
> thread.

Hm. Principals in general aren't threadsafe. Is it just being used as a purely opaque identifier? If so, we need to be super careful not to perform any operations (including refcounting) on it. You're really not supposed to call into the security infrastructure OMT.

It looks like the code is careful about not refcounting off the main thread, but that's a pretty big footgun. Could we instead pass around JSPrincipals* pointers, and only convert them into nsIPrincipal instances when we actually need them (presumably, on the main thread)?

Comment 18

[Bobby Holley (:bholley)]

Meanwhile, we should unblock Nick.

Nick, while we sort this out, can you just replace the calls to nsContentUtils::ObjectPrincipal() in AsmJSCacheOpenEntryFor{Read,Write} with:

nsJSPrincipals::get(JS_GetCompartmentPrincipals(js::GetObjectCompartment(aGlobal)))?

(You may need to #include "nsJPrincipals.h" if it isn't included already).

That'll let us land the assertion you added (which is important), and sort the AsmJS stuff out separately.

Comment 19

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal. v3.

Reworked logic of getting principal object for methods AsmJSCacheOpenEntryFor(Write|Read).

I've also started small subset of builds/tests on try server, will add them soon.

Comment 20

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #17)
> Hm. Principals in general aren't threadsafe. Is it just being used as a
> purely opaque identifier? If so, we need to be super careful not to perform
> any operations (including refcounting) on it. You're really not supposed to
> call into the security infrastructure OMT.

Yes, we are explicitly very careful with this and this all came up during the asmjscache review.  In general, caching has to hop between threads due to e10s, off-main-thread-parsing, and the QuotaManager's IO thread, so we are very careful about this and nsIPrincipals is no exception.  (It also has good main-thread assertions, as I learned.)

> Could we instead pass around JSPrincipals*
> pointers, and only convert them into nsIPrincipal instances when we actually
> need them (presumably, on the main thread)?

One problem is that workers, afaics, only appear to have an nsIPrincipal.  (See GetPrincipalForAsmJSCacheOp() in dom/workers/RuntimeService.cpp.)  But maybe they do and I'm just not finding it with my crude grepping?

Comment 21

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #20) 
> > Could we instead pass around JSPrincipals*
> > pointers, and only convert them into nsIPrincipal instances when we actually
> > need them (presumably, on the main thread)?
> 
> One problem is that workers, afaics, only appear to have an nsIPrincipal. 
> (See GetPrincipalForAsmJSCacheOp() in dom/workers/RuntimeService.cpp.)  But
> maybe they do and I'm just not finding it with my crude grepping?

Every concrete nsIPrincipal implementation in Gecko inherits nsJSPrincipals, which means that you can do nsJSPrincipals::get() to cast any nsIPrincipal to a JSPrincipals.

Would that solve the problem here? If so, I think we should do that, to better enforce that this is used as an opaque handle.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 8421838 [details] [diff] [review]
Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal. v3.

Looks great! Please post the try link (or let someone know if you don't have access to try) so that we can make sure that this assertion doesn't fire anywhere else.

Thanks for the quick patch updates, and sorry that this didn't turn out to be quite as easy as it seemed. ;-)

Comment 23

[Nick Lebedev [:nl]]

Attachment: Rename nsContentUtils::Get{Subject,Object}Principal to nsContentUtils::{Subject,Object}Principal. v4.

Rebased the patch.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I don't see a new Try link since comment 22 requested one?

Comment 25

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #21)
> Would that solve the problem here? If so, I think we should do that, to
> better enforce that this is used as an opaque handle.

I'm not sure if it would solve anything per se, but if that's what you need to do, ok.

Comment 26

[Nick Lebedev [:nl]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> I don't see a new Try link since comment 22 requested one?

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

please let me know if i need to run more tests/platforms.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I'm not familiar enough with this code to know how platform-independent it is or isn't. Better to ask Luke or Bobby if it's enough.

Comment 28

[Bobby Holley (:bholley)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> I'm not familiar enough with this code to know how platform-independent it
> is or isn't. Better to ask Luke or Bobby if it's enough.

This try push should be sufficient I think.

Comment 29

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #25)
> (In reply to Bobby Holley (:bholley) from comment #21)
> > Would that solve the problem here? If so, I think we should do that, to
> > better enforce that this is used as an opaque handle.
> 
> I'm not sure if it would solve anything per se, but if that's what you need
> to do, ok.

It would make it much harder for someone to accidentally do something verboten with the principal OMT, because nsJSPrincipals doesn't have any tempting methods hanging off of it.

Are we to the point where we get fatal assertions when trying to addref/release an nsIPrincipal OMT? If you ran into those, that'd be a good sign.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e11d4c451c

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51e11d4c451c