Bug 1547923 part 6. Make nsIGlobalObject::GetGlobalJSObject always expose to active JS. r=mccr8
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 02 May 2019 21:36:15 +0000
changeset 531318 a0971c8a28a9fc786dc897cb6df29955d23a98e6
parent 531317 9437efdc6fc3bc29853cda4ddd0986316097e10d
child 531319 1622d158818f55c1a0a3d97ff7b259977a58134f
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1547923, 9061976
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1547923 part 6. Make nsIGlobalObject::GetGlobalJSObject always expose to active JS. r=mccr8 See callsite audit in https://bugzilla.mozilla.org/attachment.cgi?id=9061976 for details on why the remaining GetGlobalJSObject callers should switch to the "always expose" behavior. Differential Revision: https://phabricator.services.mozilla.com/D29707
dom/base/Document.cpp
dom/base/nsGlobalWindowInner.cpp
dom/base/nsGlobalWindowInner.h
dom/base/nsGlobalWindowOuter.cpp
dom/base/nsGlobalWindowOuter.h
dom/base/nsIGlobalObject.h
dom/bindings/BindingUtils.h
dom/script/ScriptSettings.cpp
js/xpconnect/wrappers/XrayWrapper.cpp
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -6745,17 +6745,16 @@ nsINode* Document::AdoptNode(nsINode& aA
   if (!sameDocument) {
     newScope = GetWrapper();
     if (!newScope && GetScopeObject() && GetScopeObject()->HasJSGlobal()) {
       // Make sure cx is in a semi-sane compartment before we call WrapNative.
       // It's kind of irrelevant, given that we're passing aAllowWrapping =
       // false, and documents should always insist on being wrapped in an
       // canonical scope. But we try to pass something sane anyway.
       JSObject* globalObject = GetScopeObject()->GetGlobalJSObject();
-      JS::ExposeObjectToActiveJS(globalObject);
       JSAutoRealm ar(cx, globalObject);
       JS::Rooted<JS::Value> v(cx);
       rv = nsContentUtils::WrapNative(cx, ToSupports(this), this, &v,
                                       /* aAllowWrapping = */ false);
       if (rv.Failed()) return nullptr;
       newScope = &v.toObject();
     }
   }
--- a/dom/base/nsGlobalWindowInner.cpp
+++ b/dom/base/nsGlobalWindowInner.cpp
@@ -1569,19 +1569,17 @@ nsresult nsGlobalWindowInner::EnsureScri
 nsIScriptContext* nsGlobalWindowInner::GetScriptContext() {
   nsGlobalWindowOuter* outer = GetOuterWindowInternal();
   if (!outer) {
     return nullptr;
   }
   return outer->GetScriptContext();
 }
 
-JSObject* nsGlobalWindowInner::GetGlobalJSObject() {
-  return FastGetGlobalJSObject();
-}
+JSObject* nsGlobalWindowInner::GetGlobalJSObject() { return GetWrapper(); }
 
 JSObject* nsGlobalWindowInner::GetGlobalJSObjectPreserveColor() const {
   return FastGetGlobalJSObject();
 }
 
 void nsGlobalWindowInner::TraceGlobalJSObject(JSTracer* aTrc) {
   TraceWrapper(aTrc, "active window global");
 }
--- a/dom/base/nsGlobalWindowInner.h
+++ b/dom/base/nsGlobalWindowInner.h
@@ -243,16 +243,18 @@ class nsGlobalWindowInner final : public
     return GetWrapper();
   }
 
   // nsIGlobalObject
   JSObject* GetGlobalJSObject() override;
   JSObject* GetGlobalJSObjectPreserveColor() const override;
 
   // nsIScriptGlobalObject
+  // If this ever starts exposing to active JS, we can remove various
+  // ExposeObjectToActiveJS calls.
   JSObject* FastGetGlobalJSObject() const { return GetWrapperPreserveColor(); }
 
   void TraceGlobalJSObject(JSTracer* aTrc);
 
   virtual nsresult EnsureScriptEnvironment() override;
 
   virtual nsIScriptContext* GetScriptContext() override;
 
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -1501,19 +1501,17 @@ nsresult nsGlobalWindowOuter::EnsureScri
   NS_ENSURE_SUCCESS(rv, rv);
 
   mContext = context;
   return NS_OK;
 }
 
 nsIScriptContext* nsGlobalWindowOuter::GetScriptContext() { return mContext; }
 
-JSObject* nsGlobalWindowOuter::GetGlobalJSObject() {
-  return FastGetGlobalJSObject();
-}
+JSObject* nsGlobalWindowOuter::GetGlobalJSObject() { return GetWrapper(); }
 
 JSObject* nsGlobalWindowOuter::GetGlobalJSObjectPreserveColor() const {
   return FastGetGlobalJSObject();
 }
 
 bool nsGlobalWindowOuter::WouldReuseInnerWindow(Document* aNewDocument) {
   // We reuse the inner window when:
   // a. We are currently at our original document.
--- a/dom/base/nsGlobalWindowOuter.h
+++ b/dom/base/nsGlobalWindowOuter.h
@@ -230,16 +230,18 @@ class nsGlobalWindowOuter final : public
     return EnsureInnerWindow() ? GetWrapper() : nullptr;
   }
 
   // nsIGlobalJSObjectHolder
   JSObject* GetGlobalJSObject() override;
   JSObject* GetGlobalJSObjectPreserveColor() const override;
 
   // nsIScriptGlobalObject
+  // If this ever starts exposing to active JS, we can remove various
+  // ExposeObjectToActiveJS calls.
   JSObject* FastGetGlobalJSObject() const { return GetWrapperPreserveColor(); }
 
   virtual nsresult EnsureScriptEnvironment() override;
 
   virtual nsIScriptContext* GetScriptContext() override;
 
   void PoisonOuterWindowProxy(JSObject* aObject);
 
--- a/dom/base/nsIGlobalObject.h
+++ b/dom/base/nsIGlobalObject.h
@@ -69,22 +69,23 @@ class nsIGlobalObject : public nsISuppor
    * from being dispatched.
    *
    * We pair this with checks during processing the promise microtask queue
    * that pops up the slow script dialog when the Promise queue is preventing
    * a window from going away.
    */
   bool IsDying() const { return mIsDying; }
 
-  // GetGlobalJSObject may return a gray object.  If this ever changes so that
-  // it stops doing that, please simplify the code in FindAssociatedGlobal in
-  // BindingUtils.h that does JS::ExposeObjectToActiveJS on the return value of
-  // GetGlobalJSObject.  Also, in that case the JS::ExposeObjectToActiveJS in
-  // AutoJSAPI::InitInternal can probably be removed.  And also the similar
-  // calls in XrayWrapper and nsGlobalWindow.
+  /**
+   * Return the JSObject for this global, if it still has one.  Otherwise return
+   * null.
+   *
+   * If non-null is returned, then the returned object will have been already
+   * exposed to active JS, so callers do not need to do it.
+   */
   virtual JSObject* GetGlobalJSObject() = 0;
 
   /**
    * Return the JSObject for this global _without_ exposing it to active JS.
    * This may return a gray object.
    *
    * This method is appropriate to use in assertions (so there is less of a
    * difference in GC/CC marking between debug and optimized builds) and in
--- a/dom/bindings/BindingUtils.h
+++ b/dom/bindings/BindingUtils.h
@@ -1665,19 +1665,17 @@ inline JSObject* FindAssociatedGlobal(JS
   JSObject* global = p->GetGlobalJSObject();
   if (!global) {
     // nsIGlobalObject doesn't have a JS object anymore,
     // fallback to the current global.
     return JS::CurrentGlobalOrNull(cx);
   }
 
   MOZ_ASSERT(JS_IsGlobalObject(global));
-  // This object could be gray if the nsIGlobalObject is the only thing keeping
-  // it alive.
-  JS::ExposeObjectToActiveJS(global);
+  JS::AssertObjectIsNotGray(global);
   return global;
 }
 
 template <typename T,
           bool hasAssociatedGlobal = NativeHasMember<T>::GetParentObject>
 struct FindAssociatedGlobalForNative {
   static JSObject* Get(JSContext* cx, JS::Handle<JSObject*> obj) {
     MOZ_ASSERT(js::IsObjectInContextCompartment(obj, cx));
--- a/dom/script/ScriptSettings.cpp
+++ b/dom/script/ScriptSettings.cpp
@@ -292,17 +292,17 @@ void AutoJSAPI::InitInternal(nsIGlobalOb
 #ifdef DEBUG
   bool haveException = JS_IsExceptionPending(aCx);
 #endif  // DEBUG
 
   mCx = aCx;
   mIsMainThread = aIsMainThread;
   mGlobalObject = aGlobalObject;
   if (aGlobal) {
-    JS::ExposeObjectToActiveJS(aGlobal);
+    JS::AssertObjectIsNotGray(aGlobal);
   }
   mAutoNullableRealm.emplace(mCx, aGlobal);
 
   ScriptSettingsStack::Push(this);
 
   mOldWarningReporter.emplace(JS::GetWarningReporter(aCx));
 
   JS::SetWarningReporter(aCx, WarningOnlyErrorReporter);
--- a/js/xpconnect/wrappers/XrayWrapper.cpp
+++ b/js/xpconnect/wrappers/XrayWrapper.cpp
@@ -1647,16 +1647,18 @@ bool DOMXrayTraits::resolveOwnProperty(J
   if (IsArrayIndex(index)) {
     nsGlobalWindowInner* win = AsWindow(cx, wrapper);
     // Note: As() unwraps outer windows to get to the inner window.
     if (win) {
       nsCOMPtr<nsPIDOMWindowOuter> subframe = win->IndexedGetter(index);
       if (subframe) {
         subframe->EnsureInnerWindow();
         nsGlobalWindowOuter* global = nsGlobalWindowOuter::Cast(subframe);
+        // FastGetGlobalJSObject is non-virtual so calling it and then manually
+        // exposing is faster than calling the virtual GetGlobalJSObject()...
         JSObject* obj = global->FastGetGlobalJSObject();
         if (MOZ_UNLIKELY(!obj)) {
           // It's gone?
           return xpc::Throw(cx, NS_ERROR_FAILURE);
         }
         ExposeObjectToActiveJS(obj);
         desc.value().setObject(*obj);
         FillPropertyDescriptor(desc, wrapper, true);