Closed Bug 1472976 Opened Last year Closed Last year

Remove some more JS_GetCompartmentPrincipals calls

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(6 files)

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.
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky) → needinfo?(bugs)
Oh, and I agree with the analysis for all the other cases.
edgar may have recall the custom element specific code.
Flags: needinfo?(bugs) → needinfo?(echen)
(I need to still look at this too)
Flags: needinfo?(bugs)
smaug, edgar: needinfo ping? It would be nice to get this fixed.
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.
Flags: needinfo?(bugs)
And for (6), it shouldn't matter much. Chrome code shouldn't set onfoos on content.
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.
Depends on: 1477923
Flags: needinfo?(echen)
Priority: -- → P3
The callback and the global here must be same-compartment, but the callback might be a CCW.
Attachment #8998748 - Flags: review?(bugs)
This also removes an outdated comment, nsScriptSecurityManager::doGetObjectPrincipal was removed in bug 1464374.
Attachment #8998749 - Flags: review?(bugs)
Attachment #8998750 - Flags: review?(mrbkap) → review+
Attachment #8998752 - Flags: review?(mrbkap) → review+
Attachment #8998755 - Flags: review?(mrbkap) → review+
Attachment #8998747 - Flags: review?(bugs) → review+
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 <jdemooij@mozilla.com>
># 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
Attachment #8998749 - Flags: review?(bugs) → review+
Attachment #8998748 - Flags: review?(bugs) → review+
Olli, I assume comment 16 was just an accidental copy/paste or did you mean to leave a review comment?
yeah, accidental
Pushed by jandemooij@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.