Closed Bug 998002 Opened 6 years ago Closed 5 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: bholley, Assigned: nl)

References

Details

(Whiteboard: [mentor=bholley,lang=c++])

Attachments

(1 file, 5 obsolete files)

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.
How is GetSubjectPrincipal infallible? (In fact, it might crash currently if called too late
during shutdown process)
(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.
Whiteboard: [mentor=bholley,lang=c++]
Assignee: nobody → nicklebedev37
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
Attachment #8420056 - Flags: review?(bobbyholley)
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();
Attachment #8420056 - Flags: review?(bobbyholley)
Addressed review comments.
Attachment #8420056 - Attachment is obsolete: true
Attachment #8420240 - Flags: review?(bobbyholley)
Fixed stupid type in the method nsContentUtils::ObjectPrincipal :)
Attachment #8420240 - Attachment is obsolete: true
Attachment #8420240 - Flags: review?(bobbyholley)
Attachment #8420245 - Flags: review?(bobbyholley)
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...
Attachment #8420245 - Flags: review?(bobbyholley) → review-
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.
(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().
Fixed compilation error.
Attachment #8420245 - Attachment is obsolete: true
Attachment #8420511 - Flags: review?(bobbyholley)
Attachment #8420511 - Flags: review?(bobbyholley) → review+
Nice!
Keywords: checkin-needed
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
Keywords: checkin-needed
Looks like ObjectPrincipal() is called off the main thread: https://tbpl.mozilla.org/php/getParsedLog.php?id=39472593&tree=Try
Hm. Luke, what's the deal here? Is this intentional?
Flags: needinfo?(luke)
Yep, we have an object and need its principal and we're parsing off the main thread.
Flags: needinfo?(luke)
(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)?
Flags: needinfo?(luke)
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.
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.
Attachment #8420511 - Attachment is obsolete: true
Attachment #8421838 - Flags: review?(bobbyholley)
(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?
Flags: needinfo?(luke)
(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 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. ;-)
Attachment #8421838 - Flags: review?(bobbyholley) → review+
Keywords: checkin-needed
I don't see a new Try link since comment 22 requested one?
Keywords: checkin-needed
(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.
(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.
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
(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.
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/51e11d4c451c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.