Bug 1453339. Make it harder to mess up Promise::All. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 13 Apr 2018 19:31:42 -0400
changeset 467370 438494d2d17bec92e4f4e38661a85b60680ab087
parent 467369 43862b2ee72eec858f509ce3b9cb5c6fa96194ba
child 467371 a466495618d0468b0fd43f9c10fed8ae8112b6d9
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1453339
milestone61.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 1453339. Make it harder to mess up Promise::All. r=peterv MozReview-Commit-ID: UO4wssYHj7
dom/base/nsFrameLoader.cpp
dom/cache/Cache.cpp
dom/promise/Promise.cpp
dom/promise/Promise.h
layout/style/FontFaceSet.cpp
--- a/dom/base/nsFrameLoader.cpp
+++ b/dom/base/nsFrameLoader.cpp
@@ -390,18 +390,16 @@ nsFrameLoader::SwapBrowsersAndNotify(nsF
 already_AddRefed<Promise>
 nsFrameLoader::FireWillChangeProcessEvent()
 {
   AutoJSAPI jsapi;
   if (NS_WARN_IF(!jsapi.Init(mOwnerContent->GetOwnerGlobal()))) {
     return nullptr;
   }
   JSContext* cx = jsapi.cx();
-  GlobalObject global(cx, mOwnerContent->GetOwnerGlobal()->GetGlobalJSObject());
-  MOZ_ASSERT(!global.Failed());
 
   // Set our mBrowserChangingProcessBlockers property to refer to the blockers
   // list. We will synchronously dispatch a DOM event to collect this list of
   // blockers.
   nsTArray<RefPtr<Promise>> blockers;
   mBrowserChangingProcessBlockers = &blockers;
 
   GroupedHistoryEventInit eventInit;
@@ -413,17 +411,17 @@ nsFrameLoader::FireWillChangeProcessEven
                                      NS_LITERAL_STRING("BrowserWillChangeProcess"),
                                      eventInit);
   event->SetTrusted(true);
   mOwnerContent->DispatchEvent(*event);
 
   mBrowserChangingProcessBlockers = nullptr;
 
   ErrorResult rv;
-  RefPtr<Promise> allPromise = Promise::All(global, blockers, rv);
+  RefPtr<Promise> allPromise = Promise::All(cx, blockers, rv);
   return allPromise.forget();
 }
 
 void
 nsFrameLoader::AddProcessChangeBlockingPromise(Promise& aPromise, ErrorResult& aRv)
 {
   if (NS_WARN_IF(!mBrowserChangingProcessBlockers)) {
     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
--- a/dom/cache/Cache.cpp
+++ b/dom/cache/Cache.cpp
@@ -625,17 +625,17 @@ Cache::AddAll(const GlobalObject& aGloba
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
 
   RefPtr<FetchHandler> handler =
     new FetchHandler(mActor->GetWorkerHolder(), this,
                      Move(aRequestList), promise);
 
-  RefPtr<Promise> fetchPromise = Promise::All(aGlobal, fetchList, aRv);
+  RefPtr<Promise> fetchPromise = Promise::All(aGlobal.Context(), fetchList, aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
   fetchPromise->AppendNativeHandler(handler);
 
   return promise.forget();
 }
 
--- a/dom/promise/Promise.cpp
+++ b/dom/promise/Promise.cpp
@@ -130,47 +130,50 @@ Promise::Reject(nsIGlobalObject* aGlobal
     return nullptr;
   }
 
   return CreateFromExisting(aGlobal, p);
 }
 
 // static
 already_AddRefed<Promise>
-Promise::All(const GlobalObject& aGlobal,
+Promise::All(JSContext* aCx,
              const nsTArray<RefPtr<Promise>>& aPromiseList, ErrorResult& aRv)
 {
-  nsCOMPtr<nsIGlobalObject> global;
-  global = do_QueryInterface(aGlobal.GetAsSupports());
+  JS::Rooted<JSObject*> globalObj(aCx, JS::CurrentGlobalOrNull(aCx));
+  if (!globalObj) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }
+
+  nsCOMPtr<nsIGlobalObject> global = xpc::NativeGlobal(globalObj);
   if (!global) {
     aRv.Throw(NS_ERROR_UNEXPECTED);
     return nullptr;
   }
 
-  JSContext* cx = aGlobal.Context();
-
-  JS::AutoObjectVector promises(cx);
+  JS::AutoObjectVector promises(aCx);
   if (!promises.reserve(aPromiseList.Length())) {
-    aRv.NoteJSContextException(cx);
+    aRv.NoteJSContextException(aCx);
     return nullptr;
   }
 
   for (auto& promise : aPromiseList) {
-    JS::Rooted<JSObject*> promiseObj(cx, promise->PromiseObj());
+    JS::Rooted<JSObject*> promiseObj(aCx, promise->PromiseObj());
     // Just in case, make sure these are all in the context compartment.
-    if (!JS_WrapObject(cx, &promiseObj)) {
-      aRv.NoteJSContextException(cx);
+    if (!JS_WrapObject(aCx, &promiseObj)) {
+      aRv.NoteJSContextException(aCx);
       return nullptr;
     }
     promises.infallibleAppend(promiseObj);
   }
 
-  JS::Rooted<JSObject*> result(cx, JS::GetWaitForAllPromise(cx, promises));
+  JS::Rooted<JSObject*> result(aCx, JS::GetWaitForAllPromise(aCx, promises));
   if (!result) {
-    aRv.NoteJSContextException(cx);
+    aRv.NoteJSContextException(aCx);
     return nullptr;
   }
 
   return CreateFromExisting(global, result);
 }
 
 void
 Promise::Then(JSContext* aCx,
--- a/dom/promise/Promise.h
+++ b/dom/promise/Promise.h
@@ -106,33 +106,36 @@ public:
 
   // WebIDL
 
   nsIGlobalObject* GetParentObject() const
   {
     return mGlobal;
   }
 
-  // Do the equivalent of Promise.resolve in the current compartment of aCx.
-  // Errorrs are reported on the ErrorResult; if aRv comes back !Failed(), this
-  // function MUST return a non-null value.
+  // Do the equivalent of Promise.resolve in the compartment of aGlobal.  The
+  // compartment of aCx is ignored.  Errors are reported on the ErrorResult; if
+  // aRv comes back !Failed(), this function MUST return a non-null value.
   static already_AddRefed<Promise>
   Resolve(nsIGlobalObject* aGlobal, JSContext* aCx,
           JS::Handle<JS::Value> aValue, ErrorResult& aRv);
 
-  // Do the equivalent of Promise.reject in the current compartment of aCx.
-  // Errorrs are reported on the ErrorResult; if aRv comes back !Failed(), this
-  // function MUST return a non-null value.
+  // Do the equivalent of Promise.reject in the compartment of aGlobal.  The
+  // compartment of aCx is ignored.  Errors are reported on the ErrorResult; if
+  // aRv comes back !Failed(), this function MUST return a non-null value.
   static already_AddRefed<Promise>
   Reject(nsIGlobalObject* aGlobal, JSContext* aCx,
          JS::Handle<JS::Value> aValue, ErrorResult& aRv);
 
+  // Do the equivalent of Promise.all in the current compartment of aCx.  Errors
+  // are reported on the ErrorResult; if aRv comes back !Failed(), this function
+  // MUST return a non-null value.
   static already_AddRefed<Promise>
-  All(const GlobalObject& aGlobal,
-      const nsTArray<RefPtr<Promise>>& aPromiseList, ErrorResult& aRv);
+  All(JSContext* aCx, const nsTArray<RefPtr<Promise>>& aPromiseList,
+      ErrorResult& aRv);
 
   void
   Then(JSContext* aCx,
        // aCalleeGlobal may not be in the compartment of aCx, when called over
        // Xrays.
        JS::Handle<JSObject*> aCalleeGlobal,
        AnyCallback* aResolveCallback, AnyCallback* aRejectCallback,
        JS::MutableHandle<JS::Value> aRetval,
--- a/layout/style/FontFaceSet.cpp
+++ b/layout/style/FontFaceSet.cpp
@@ -341,27 +341,17 @@ FontFaceSet::Load(JSContext* aCx,
       return nullptr;
     }
     if (!promises.AppendElement(promise, fallible)) {
       aRv.Throw(NS_ERROR_FAILURE);
       return nullptr;
     }
   }
 
-  nsIGlobalObject* globalObject = GetParentObject();
-  if (!globalObject) {
-    aRv.Throw(NS_ERROR_FAILURE);
-    return nullptr;
-  }
-
-  JS::Rooted<JSObject*> jsGlobal(aCx, globalObject->GetGlobalJSObject());
-  GlobalObject global(aCx, jsGlobal);
-
-  RefPtr<Promise> result = Promise::All(global, promises, aRv);
-  return result.forget();
+  return Promise::All(aCx, promises, aRv);
 }
 
 bool
 FontFaceSet::Check(const nsAString& aFont,
                    const nsAString& aText,
                    ErrorResult& aRv)
 {
   FlushUserFontSet();