Bug 1453339 - Make it harder to mess up Promise::All. r=peterv, a=abillings
authorBoris Zbarsky <bzbarsky@mit.edu>
Fri, 13 Apr 2018 19:31:42 -0400
changeset 783682 eb825e4db72f892b87e53fec8619d7643a17a990
parent 783681 7cb7bafe04e4ab600d49a90302e8455748199431
child 783683 bf6f0553eaeb4b41e4d9c70744b3818a42c27115
push id106755
push userbmo:jlorenzo@mozilla.com
push dateTue, 17 Apr 2018 15:40:10 +0000
reviewerspeterv, abillings
bugs1453339
milestone60.0
Bug 1453339 - Make it harder to mess up Promise::All. r=peterv, a=abillings 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
@@ -404,18 +404,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;
@@ -428,17 +426,17 @@ nsFrameLoader::FireWillChangeProcessEven
                                      eventInit);
   event->SetTrusted(true);
   bool dummy;
   mOwnerContent->DispatchEvent(event, &dummy);
 
   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
@@ -129,47 +129,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
@@ -396,27 +396,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();