Bug 1453339 - Make it harder to mess up Promise::All. r=peterv
☠☠ backed out by bb61d1086230 ☠ ☠
authorBoris Zbarsky <bzbarsky@mit.edu>
Thu, 12 Apr 2018 17:03:49 -0400
changeset 466898 91406356569ce2b32ed48486cc516f490eec1ab8
parent 466897 6ac542a489afe6dde28b2b43b01336667506b0f7
child 466899 a1d4a3e6c77a29e7e63d5e88f5b4859b113df943
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();