1472976 - Remove some more JS_GetCompartmentPrincipals calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

See bug 1465700. There are 6 callers left but:

* The one in XPCWrappedNativeScope::GetPrincipal is easy to convert to JS::GetRealmPrincipals because it's just a global object.

* nsContentUtils::ObjectPrincipal has 19 callers, I *think* most of these pass a global object and none of them pass wrappers so this can probably assert !IsWrapper. I still need to do a more thorough audit, though.

Comment 1

[Jan de Mooij [:jandem]]

nsContentUtils::ObjectPrincipal audit below. The ones I'm not sure about are (1), (6), (20).

(1) IsCustomElementsEnabled:

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/base/CustomElementRegistry.cpp#299

I think we can call this on Xrays here: https://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/TestJSImplGenBinding.cpp#40543

(2) nsGlobalWindowInner::IsPrivilegedChromeWindow - xpc::WindowOrNull called before this already asserts !IsWrapper.
(3) nsIGlobalObject::PrincipalOrNull - a global object.
(4) CallerSubsumes in BindingUtils.cpp - does UncheckedUnwrap.
(5) CacheStorage::DefineCaches - a global object (there's a MOZ_DIAGNOSTIC_ASSERT).

(6) JSEventHandler::HandleEvent - this passes GetTypedEventHandler().Ptr()->CallbackPreserveColor(), could be a wrapper?

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/events/JSEventHandler.cpp#125-126

(7) IDBFactory::CreateForMainThreadJS - the one caller (IndexedDatabaseManager::DefineIndexedDB) asserts it's a DOM global.
(8) nsJSThunk::EvaluateScript - a global object.
(9) SpeechRecognition::IsAuthorized - called by ConstructorEnabled in generated bindings, passing the global object.
(10) TCPSocket::ShouldTCPSocketExist - as above (+ PrefableDisablers calls this with a global).
(11) Promise::ReportRejectedPromise - asserts !IsWrapper(aPromise).
(12) ScriptLoader::FillCompileOptionsForRequest - the 3 callers of this pass a global object as aScopeChain.
(13) AutoJSAPI::ReportException - a global.
(14) (15) (16) URLMainThread::CreateObjectURL, URLMainThread::RevokeObjectURL - these pass a global.
(17) nsXPCComponents_Utils::GetObjectPrincipal - unwraps first.
(18) (19) URLForGlobal, nsPerformanceStatsService::GetPerformanceGroups - a global object.

(20) nsGlobalWindowInner::DoResolve (#ifdef USE_CONTROLLERS_SHIM) - this first guards !xpc::IsXrayWrapper(aObj), does that rule out all wrappers we can see there:

https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/dom/base/nsGlobalWindowInner.cpp#3006-3007

The MOZ_ASSERT(JS_IsGlobalObject(aObj)) down below suggests this might be true.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> (20) nsGlobalWindowInner::DoResolve (#ifdef USE_CONTROLLERS_SHIM) - this first guards !xpc::IsXrayWrapper(aObj), does that rule out all wrappers we can see there:

In this case, aObj is guaranteed to be either a window global or an Xray for a WindowProxy.  So we're good here.

> (1) IsCustomElementsEnabled:

Good catch.  The caller spit out by the CEReactions code in Codegen.py will happily pass an Xray here.  On the other hand, the calls in the webidl exposure bits will make sure to pass the Xray target.  I have no idea why this method is inconsistent in terms of which objects it examines; presumably it shouldn't be.  Olli, do you know what the desired behavior is here?

> (6) JSEventHandler::HandleEvent - this passes GetTypedEventHandler().Ptr()->CallbackPreserveColor(), could be a wrapper?

Also good catch.  This could be a wrapper for sure.  The question is what the behavior should be in that case: do we want to consider the chrome-or-not nature of the underlying object, or something else?  This is also a question for Olli, I think.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and I agree with the analysis for all the other cases.

Comment 4

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

edgar may have recall the custom element specific code.

Comment 5

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

(I need to still look at this too)

Comment 6

[Jan de Mooij [:jandem]]

smaug, edgar: needinfo ping? It would be nice to get this fixed.

Comment 7

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

Based on our webidl documentation, JSObject passed to Func= stuff is the global of the relevant content object. And that is the behavior CustomElementRegistry::IsCustomElementEnabled expects.

Comment 8

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

And for (6), it shouldn't matter much. Chrome code shouldn't set onfoos on content.

Comment 9

[Jan de Mooij [:jandem]]

smaug and I discussed (1) a bit more on IRC; unwrapping there should be okay.

(6) can probably use the CallbackObject's callback global (bug 1477923 is adding this) instead.

There are no new callers since the audit in comment 1 so hopefully when bug 1477923 lands this will be straight-forward.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Unwrap the object before getting its principal in CustomElementRegistry::IsCustomElementEnabled

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Use the callback global instead of the callback in JSEventHandler::HandleEvent

The callback and the global here must be same-compartment, but the callback might be a CCW.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in nsContentUtils::ObjectPrincipal

This also removes an outdated comment, nsScriptSecurityManager::doGetObjectPrincipal was removed in bug 1464374.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in XPCWrappedNativeScope::GetPrincipal

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Use xpc::GetRealmPrincipal instead of xpc::GetCompartmentPrincipal in xpc::GetObjectPrincipal

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove unused xpc::AccessCheck::getPrincipal method

Comment 16

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

Comment on attachment 8998749 [details] [diff] [review]
Part 3 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in nsContentUtils::ObjectPrincipal

># HG changeset patch
># User Jan de Mooij <[e-mail]>
># Parent  e04691be47561fdd481c99644acb76631ff8958f
>Bug 1472976 part 3 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in nsContentUtils::ObjectPrincipal. r=smaug
>
>This also removes an outdated comment, nsScriptSecurityManager::doGetObjectPrincipal was removed in bug 1464374.
>
>diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
>--- a/dom/base/nsContentUtils.cpp
>+++ b/dom/base/nsContentUtils.cpp
>@@ -3092,20 +3092,20 @@ nsIPrincipal*
> nsContentUtils::ObjectPrincipal(JSObject* aObj)
> {
>   MOZ_ASSERT(NS_IsMainThread());
> 
> #ifdef DEBUG
>   JS::AssertObjectBelongsToCurrentThread(aObj);
> #endif
> 
>-  // This is duplicated from nsScriptSecurityManager. We don't call through there
>-  // because the API unnecessarily requires a JSContext for historical reasons.
>-  JS::Compartment* compartment = js::GetObjectCompartment(aObj);
>-  JSPrincipals* principals = JS_GetCompartmentPrincipals(compartment);
>+  MOZ_DIAGNOSTIC_ASSERT(!js::IsCrossCompartmentWrapper(aObj));
>+
>+  JS::Realm* realm = js::GetNonCCWObjectRealm(aObj);
>+  JSPrincipals* principals = JS::GetRealmPrincipals(realm);
>   return nsJSPrincipals::get(principals);
> }
> 
> // static
> nsresult
> nsContentUtils::NewURIWithDocumentCharset(nsIURI** aResult,
>                                           const nsAString& aSpec,
>                                           nsIDocument* aDocument,
>diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h
>--- a/dom/base/nsContentUtils.h
>+++ b/dom/base/nsContentUtils.h
>@@ -660,17 +660,19 @@ public:
>   // from the main thread and assumes an existing compartment.
>   static nsIPrincipal* SubjectPrincipal(JSContext* aCx);
> 
>   // Returns the subject principal. Guaranteed to return non-null. May only
>   // be called when nsContentUtils is initialized.
>   static nsIPrincipal* SubjectPrincipal();
> 
>   // 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.
>+  // the main thread for objects from the main thread's JSRuntime. The object
>+  // must not be a cross-compartment wrapper, because CCWs are not associated
>+  // with a single realm.
>   static nsIPrincipal* ObjectPrincipal(JSObject* aObj);
> 
>   static nsresult GenerateStateKey(nsIContent* aContent,
>                                    nsIDocument* aDocument,
>                                    nsACString& aKey);
> 
>   /**
>    * Create a new nsIURI from aSpec, using aBaseURI as the base.  The

Comment 17

[Jan de Mooij [:jandem]]

Olli, I assume comment 16 was just an accidental copy/paste or did you mean to leave a review comment?

Comment 18

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

yeah, accidental

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/088210d062b7
part 1 - Unwrap the object before getting its principal in CustomElementRegistry::IsCustomElementEnabled. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/650a29193146
part 2 - Use the callback global instead of the callback in JSEventHandler::HandleEvent. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e40d14b0c5a
part 3 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in nsContentUtils::ObjectPrincipal. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f26ac88d55c
part 4 - Use JS::GetRealmPrincipals instead of JS_GetCompartmentPrincipals in XPCWrappedNativeScope::GetPrincipal. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/efacd0e6ef9b
part 5 - Use xpc::GetRealmPrincipal instead of xpc::GetCompartmentPrincipal in xpc::GetObjectPrincipal. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3a7d213794
part 6 - Remove unused xpc::AccessCheck::getPrincipal method. r=mrbkap

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/088210d062b7
https://hg.mozilla.org/mozilla-central/rev/650a29193146
https://hg.mozilla.org/mozilla-central/rev/9e40d14b0c5a
https://hg.mozilla.org/mozilla-central/rev/7f26ac88d55c
https://hg.mozilla.org/mozilla-central/rev/efacd0e6ef9b
https://hg.mozilla.org/mozilla-central/rev/3c3a7d213794