Bug 1170760 part 8. Add subclassing support to Promise::All. r=baku,efaust
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 25 Nov 2015 15:48:09 -0500
changeset 308966 fed3e6ac1affd2773441a28b9170c47eaf80e6d1
parent 308965 694d3b2752a723b5223cdc79c7cfea04d01e6617
child 308967 d282d800811ea5c01de02075b954077c8e1412c8
push id5513
push userraliiev@mozilla.com
push dateMon, 25 Jan 2016 13:55:34 +0000
treeherdermozilla-beta@5ee97dd05b5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku, efaust
bugs1170760
milestone45.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 1170760 part 8. Add subclassing support to Promise::All. r=baku,efaust
dom/promise/Promise.cpp
dom/promise/Promise.h
dom/promise/tests/test_promise_xrays.html
dom/webidl/Promise.webidl
testing/web-platform/tests/js/builtins/Promise-subclassing.html
--- a/dom/promise/Promise.cpp
+++ b/dom/promise/Promise.cpp
@@ -1245,34 +1245,329 @@ NS_IMPL_CYCLE_COLLECTING_ADDREF(AllResol
 NS_IMPL_CYCLE_COLLECTING_RELEASE(AllResolveElementFunction)
 
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(AllResolveElementFunction)
   NS_INTERFACE_MAP_ENTRY(nsISupports)
 NS_INTERFACE_MAP_END
 
 NS_IMPL_CYCLE_COLLECTION(AllResolveElementFunction, mCountdownHolder)
 
-/* static */ already_AddRefed<Promise>
+static const JSClass PromiseAllDataHolderClass = {
+  "PromiseAllDataHolder", JSCLASS_HAS_RESERVED_SLOTS(3)
+};
+
+// Slot indices for objects of class PromiseAllDataHolderClass.
+#define DATA_HOLDER_REMAINING_ELEMENTS_SLOT 0
+#define DATA_HOLDER_VALUES_ARRAY_SLOT 1
+#define DATA_HOLDER_RESOLVE_FUNCTION_SLOT 2
+
+// Slot indices for PromiseAllResolveElement.
+// The RESOLVE_ELEMENT_INDEX_SLOT stores our index unless we've already been
+// called.  Then it stores INT32_MIN (which is never a valid index value).
+#define RESOLVE_ELEMENT_INDEX_SLOT 0
+// The RESOLVE_ELEMENT_DATA_HOLDER_SLOT slot stores an object of class
+// PromiseAllDataHolderClass.
+#define RESOLVE_ELEMENT_DATA_HOLDER_SLOT 1
+
+static bool
+PromiseAllResolveElement(JSContext* aCx, unsigned aArgc, JS::Value* aVp)
+{
+  // Implements
+  // http://www.ecma-international.org/ecma-262/6.0/#sec-promise.all-resolve-element-functions
+  //
+  // See the big comment about compartments in Promise::All "Substep 4" that
+  // explains what compartments the various stuff here lives in.
+  JS::CallArgs args = CallArgsFromVp(aArgc, aVp);
+
+  // Step 1.
+  int32_t index =
+    js::GetFunctionNativeReserved(&args.callee(),
+                                  RESOLVE_ELEMENT_INDEX_SLOT).toInt32();
+  // Step 2.
+  if (index == INT32_MIN) {
+    args.rval().setUndefined();
+    return true;
+  }
+
+  // Step 3.
+  js::SetFunctionNativeReserved(&args.callee(),
+                                RESOLVE_ELEMENT_INDEX_SLOT,
+                                JS::Int32Value(INT32_MIN));
+
+  // Step 4 already done.
+
+  // Step 5.
+  JS::Rooted<JSObject*> dataHolder(aCx,
+    &js::GetFunctionNativeReserved(&args.callee(),
+                                   RESOLVE_ELEMENT_DATA_HOLDER_SLOT).toObject());
+
+  JS::Rooted<JS::Value> values(aCx,
+    js::GetReservedSlot(dataHolder, DATA_HOLDER_VALUES_ARRAY_SLOT));
+
+  // Step 6, effectively.
+  JS::Rooted<JS::Value> resolveFunc(aCx,
+    js::GetReservedSlot(dataHolder, DATA_HOLDER_RESOLVE_FUNCTION_SLOT));
+
+  // Step 7.
+  int32_t remainingElements =
+    js::GetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT).toInt32();
+
+  // Step 8.
+  JS::Rooted<JSObject*> valuesObj(aCx, &values.toObject());
+  if (!JS_DefineElement(aCx, valuesObj, index, args.get(0), JSPROP_ENUMERATE)) {
+    return false;
+  }
+
+  // Step 9.
+  remainingElements -= 1;
+  js::SetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT,
+                      JS::Int32Value(remainingElements));
+
+  // Step 10.
+  if (remainingElements == 0) {
+    return JS::Call(aCx, JS::UndefinedHandleValue, resolveFunc,
+                    JS::HandleValueArray(values), args.rval());
+  }
+
+  // Step 11.
+  args.rval().setUndefined();
+  return true;
+}
+
+
+/* static */ void
 Promise::All(const GlobalObject& aGlobal, JS::Handle<JS::Value> aThisv,
-             const Sequence<JS::Value>& aIterable, ErrorResult& aRv)
+             JS::Handle<JS::Value> aIterable,
+             JS::MutableHandle<JS::Value> aRetval, ErrorResult& aRv)
 {
+  // Implements http://www.ecma-international.org/ecma-262/6.0/#sec-promise.all
+  nsCOMPtr<nsIGlobalObject> global =
+    do_QueryInterface(aGlobal.GetAsSupports());
+  if (!global) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return;
+  }
+
   JSContext* cx = aGlobal.Context();
 
-  nsTArray<RefPtr<Promise>> promiseList;
+  // Steps 1-5: nothing to do.  Note that the @@species bits got removed in
+  // https://github.com/tc39/ecma262/pull/211
+
+  // Step 6.
+  PromiseCapability capability(cx);
+  NewPromiseCapability(cx, global, aThisv, true, capability, aRv);
+  // Step 7.
+  if (aRv.Failed()) {
+    return;
+  }
+
+  MOZ_ASSERT(aThisv.isObject(), "How did NewPromiseCapability succeed?");
+  JS::Rooted<JSObject*> constructorObj(cx, &aThisv.toObject());
+
+  // After this point we have a useful promise value in "capability", so just go
+  // ahead and put it in our retval now.  Every single return path below would
+  // want to do that anyway.
+  aRetval.set(capability.PromiseValue());
+  if (!MaybeWrapValue(cx, aRetval)) {
+    aRv.NoteJSContextException();
+    return;
+  }
+
+  // The arguments we're going to be passing to "then" on each loop iteration.
+  // The second one we know already; the first one will be created on each
+  // iteration of the loop.
+  JS::AutoValueArray<2> callbackFunctions(cx);
+  callbackFunctions[1].set(capability.mReject);
+
+  // Steps 8 and 9.
+  JS::ForOfIterator iter(cx);
+  if (!iter.init(aIterable, JS::ForOfIterator::AllowNonIterable)) {
+    capability.RejectWithException(cx, aRv);
+    return;
+  }
+
+  if (!iter.valueIsIterable()) {
+    ThrowErrorMessage(cx, MSG_PROMISE_ARG_NOT_ITERABLE,
+                      "Argument of Promise.all");
+    capability.RejectWithException(cx, aRv);
+    return;
+  }
+
+  // Step 10 doesn't need to be done, because ForOfIterator handles it
+  // for us.
+
+  // Now we jump over to
+  // http://www.ecma-international.org/ecma-262/6.0/#sec-performpromiseall
+  // and do its steps.
 
-  for (uint32_t i = 0; i < aIterable.Length(); ++i) {
-    JS::Rooted<JS::Value> value(cx, aIterable.ElementAt(i));
-    RefPtr<Promise> nextPromise = Promise::Resolve(aGlobal, aThisv, value, aRv);
+  // Substep 4. Create our data holder that holds all the things shared across
+  // every step of the iterator.  In particular, this holds the
+  // remainingElementsCount (as an integer reserved slot), the array of values,
+  // and the resolve function from our PromiseCapability.
+  //
+  // We have to be very careful about which compartments we create things in
+  // here.  In particular, we have to maintain the invariant that anything
+  // stored in a reserved slot is same-compartment with the object whose
+  // reserved slot it's in.  But we want to create the values array in the
+  // Promise reflector compartment, because that array can get exposed to code
+  // that has access to the Promise reflector (in particular code from that
+  // compartment), and that should work, even if the Promise reflector
+  // compartment is less-privileged than our caller compartment.
+  //
+  // So the plan is as follows: Create the values array in the promise reflector
+  // compartment.  Create the PromiseAllResolveElement function and the data
+  // holder in our current compartment.  Store a cross-compartment wrapper to
+  // the values array in the holder.  This should be OK because the only things
+  // we hand the PromiseAllResolveElement function to are the "then" calls we do
+  // and in the case when the reflector compartment is not the current
+  // compartment those are happening over Xrays anyway, which means they get the
+  // canonical "then" function and content can't see our
+  // PromiseAllResolveElement.
+  JS::Rooted<JSObject*> dataHolder(cx);
+  dataHolder = JS_NewObjectWithGivenProto(cx, &PromiseAllDataHolderClass,
+                                          nullptr);
+  if (!dataHolder) {
+    capability.RejectWithException(cx, aRv);
+    return;
+  }
 
-    MOZ_ASSERT(!aRv.Failed());
+  JS::Rooted<JSObject*> reflectorGlobal(cx, global->GetGlobalJSObject());
+  JS::Rooted<JSObject*> valuesArray(cx);
+  { // Scope for JSAutoCompartment.
+    JSAutoCompartment ac(cx, reflectorGlobal);
+    valuesArray = JS_NewArrayObject(cx, 0);
+  }
+  if (!valuesArray) {
+    // It's important that we've exited the JSAutoCompartment by now, before
+    // calling RejectWithException and possibly invoking capability.mReject.
+    capability.RejectWithException(cx, aRv);
+    return;
+  }
 
-    promiseList.AppendElement(Move(nextPromise));
+  // The values array as a value we can pass to a function in our current
+  // compartment, or store in the holder's reserved slot.
+  JS::Rooted<JS::Value> valuesArrayVal(cx, JS::ObjectValue(*valuesArray));
+  if (!MaybeWrapObjectValue(cx, &valuesArrayVal)) {
+    capability.RejectWithException(cx, aRv);
+    return;
   }
 
-  return Promise::All(aGlobal, promiseList, aRv);
+  js::SetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT,
+                      JS::Int32Value(1));
+  js::SetReservedSlot(dataHolder, DATA_HOLDER_VALUES_ARRAY_SLOT,
+                      valuesArrayVal);
+  js::SetReservedSlot(dataHolder, DATA_HOLDER_RESOLVE_FUNCTION_SLOT,
+                      capability.mResolve);
+
+  // Substep 5.
+  CheckedInt32 index = 0;
+
+  // Substep 6.
+  JS::Rooted<JS::Value> nextValue(cx);
+  while (true) {
+    bool done;
+    // Steps a, b, c, e, f, g.
+    if (!iter.next(&nextValue, &done)) {
+      capability.RejectWithException(cx, aRv);
+      return;
+    }
+
+    // Step d.
+    if (done) {
+      int32_t remainingCount =
+        js::GetReservedSlot(dataHolder,
+                            DATA_HOLDER_REMAINING_ELEMENTS_SLOT).toInt32();
+      remainingCount -= 1;
+      if (remainingCount == 0) {
+        JS::Rooted<JS::Value> ignored(cx);
+        if (!JS::Call(cx, JS::UndefinedHandleValue, capability.mResolve,
+                      JS::HandleValueArray(valuesArrayVal), &ignored)) {
+          capability.RejectWithException(cx, aRv);
+        }
+        return;
+      }
+      js::SetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT,
+                          JS::Int32Value(remainingCount));
+      // We're all set for now!
+      return;
+    }
+
+    // Step h.
+    { // Scope for the JSAutoCompartment we need to work with valuesArray.  We
+      // mostly do this for performance; we could go ahead and do the define via
+      // a cross-compartment proxy instead...
+      JSAutoCompartment ac(cx, valuesArray);
+      if (!JS_DefineElement(cx, valuesArray, index.value(),
+                            JS::UndefinedHandleValue, JSPROP_ENUMERATE)) {
+        // Have to go back into the caller compartment before we try to touch
+        // capability.mReject.  Luckily, capability.mReject is guaranteed to be
+        // an object in the right compartment here.
+        JSAutoCompartment ac2(cx, &capability.mReject.toObject());
+        capability.RejectWithException(cx, aRv);
+        return;
+      }
+    }
+
+    // Step i.  Sadly, we can't take a shortcut here even if
+    // capability.mNativePromise exists, because someone could have overridden
+    // "resolve" on the canonical Promise constructor.
+    JS::Rooted<JS::Value> nextPromise(cx);
+    if (!JS_CallFunctionName(cx, constructorObj, "resolve",
+                             JS::HandleValueArray(nextValue),
+                             &nextPromise)) {
+      // Step j.
+      capability.RejectWithException(cx, aRv);
+      return;
+    }
+
+    // Step k.
+    JS::Rooted<JSObject*> resolveElement(cx);
+    JSFunction* resolveFunc =
+      js::NewFunctionWithReserved(cx, PromiseAllResolveElement,
+                                  1 /* nargs */, 0 /* flags */, nullptr);
+    if (!resolveFunc) {
+      capability.RejectWithException(cx, aRv);
+      return;
+    }
+
+    resolveElement = JS_GetFunctionObject(resolveFunc);
+    // Steps l-p.
+    js::SetFunctionNativeReserved(resolveElement,
+                                  RESOLVE_ELEMENT_INDEX_SLOT,
+                                  JS::Int32Value(index.value()));
+    js::SetFunctionNativeReserved(resolveElement,
+                                  RESOLVE_ELEMENT_DATA_HOLDER_SLOT,
+                                  JS::ObjectValue(*dataHolder));
+
+    // Step q.
+    int32_t remainingElements =
+      js::GetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT).toInt32();
+    js::SetReservedSlot(dataHolder, DATA_HOLDER_REMAINING_ELEMENTS_SLOT,
+                        JS::Int32Value(remainingElements + 1));
+
+    // Step r.  And now we don't know whether nextPromise has an overridden
+    // "then" method, so no shortcuts here either.
+    callbackFunctions[0].setObject(*resolveElement);
+    JS::Rooted<JSObject*> nextPromiseObj(cx);
+    JS::Rooted<JS::Value> ignored(cx);
+    if (!JS_ValueToObject(cx, nextPromise, &nextPromiseObj) ||
+        !JS_CallFunctionName(cx, nextPromiseObj, "then", callbackFunctions,
+                             &ignored)) {
+      // Step s.
+      capability.RejectWithException(cx, aRv);
+    }
+
+    // Step t.
+    index += 1;
+    if (!index.isValid()) {
+      // Let's just claim OOM.
+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
+      capability.RejectWithException(cx, aRv);
+    }
+  }
 }
 
 /* static */ already_AddRefed<Promise>
 Promise::All(const GlobalObject& aGlobal,
              const nsTArray<RefPtr<Promise>>& aPromiseList, ErrorResult& aRv)
 {
   nsCOMPtr<nsIGlobalObject> global =
     do_QueryInterface(aGlobal.GetAsSupports());
--- a/dom/promise/Promise.h
+++ b/dom/promise/Promise.h
@@ -181,19 +181,20 @@ public:
 
   already_AddRefed<Promise>
   Then(JSContext* aCx, AnyCallback* aResolveCallback,
        AnyCallback* aRejectCallback, ErrorResult& aRv);
 
   already_AddRefed<Promise>
   Catch(JSContext* aCx, AnyCallback* aRejectCallback, ErrorResult& aRv);
 
-  static already_AddRefed<Promise>
+  static void
   All(const GlobalObject& aGlobal, JS::Handle<JS::Value> aThisv,
-      const Sequence<JS::Value>& aIterable, ErrorResult& aRv);
+      JS::Handle<JS::Value> aIterable, JS::MutableHandle<JS::Value> aRetval,
+      ErrorResult& aRv);
 
   static already_AddRefed<Promise>
   All(const GlobalObject& aGlobal,
       const nsTArray<RefPtr<Promise>>& aPromiseList, ErrorResult& aRv);
 
   static void
   Race(const GlobalObject& aGlobal, JS::Handle<JS::Value> aThisv,
        JS::Handle<JS::Value> aIterable, JS::MutableHandle<JS::Value> aRetval,
--- a/dom/promise/tests/test_promise_xrays.html
+++ b/dom/promise/tests/test_promise_xrays.html
@@ -95,23 +95,101 @@ function testRace4() {
          "Should get the right value when racing chrome-side promises");
     },
     function(e) {
       ok(false, "testRace4 threw exception: " + e);
     }
   ).then(nextTest);
 }
 
+function testAll1() {
+  var p = win.Promise.all(new win.Array(1, 2));
+  p.then(
+    function(arg) {
+      ok(arg instanceof win.Array, "Should get an Array from Promise.all (1)");
+      is(arg[0], 1, "First entry of Promise.all return value should be correct (1)");
+      is(arg[1], 2, "Second entry of Promise.all return value should be correct (1)");
+    },
+    function(e) {
+      ok(false, "testAll1 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
+function testAll2() {
+  var p = win.Promise.all(
+    new Array(win.Promise.resolve(1), win.Promise.resolve(2)));
+  p.then(
+    function(arg) {
+      ok(arg instanceof win.Array, "Should get an Array from Promise.all (2)");
+      is(arg[0], 1, "First entry of Promise.all return value should be correct (2)");
+      is(arg[1], 2, "Second entry of Promise.all return value should be correct (2)");
+    },
+    function(e) {
+      ok(false, "testAll2 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
+function testAll3() {
+  // This works with a chrome-side array because we do the iteration
+  // while still in the Xray compartment.
+  var p = win.Promise.all([1, 2]);
+  p.then(
+    function(arg) {
+      ok(arg instanceof win.Array, "Should get an Array from Promise.all (3)");
+      is(arg[0], 1, "First entry of Promise.all return value should be correct (3)");
+      is(arg[1], 2, "Second entry of Promise.all return value should be correct (3)");
+    },
+    function(e) {
+      ok(false, "testAll3 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
+function testAll4() {
+  // This works with both content-side and chrome-side Promises because we want
+  // it to and go to some lengths to make it work.
+  var p = win.Promise.all([Promise.resolve(1), win.Promise.resolve(2)]);
+  p.then(
+    function(arg) {
+      ok(arg instanceof win.Array, "Should get an Array from Promise.all (4)");
+      is(arg[0], 1, "First entry of Promise.all return value should be correct (4)");
+      is(arg[1], 2, "Second entry of Promise.all return value should be correct (4)");
+    },
+    function(e) {
+      ok(false, "testAll4 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
+function testAll5() {
+  var p = win.Promise.all(new win.Array());
+  p.then(
+    function(arg) {
+      ok(arg instanceof win.Array, "Should get an Array from Promise.all (5)");
+    },
+    function(e) {
+      ok(false, "testAll5 threw exception: " + e);
+    }
+  ).then(nextTest);
+}
+
 var tests = [
   testLoadComplete,
   testHaveXray,
   testRace1,
   testRace2,
   testRace3,
   testRace4,
+  testAll1,
+  testAll2,
+  testAll3,
+  testAll4,
+  testAll5,
 ];
 
 function nextTest() {
   if (tests.length == 0) {
     SimpleTest.finish();
     return;
   }
   tests.shift()();
--- a/dom/webidl/Promise.webidl
+++ b/dom/webidl/Promise.webidl
@@ -34,18 +34,23 @@ interface _Promise {
   // nothing instead of throwing errors when non-callable arguments are passed.
   [NewObject, MethodIdentityTestable]
   Promise<any> then([TreatNonCallableAsNull] optional AnyCallback? fulfillCallback = null,
                     [TreatNonCallableAsNull] optional AnyCallback? rejectCallback = null);
 
   [NewObject]
   Promise<any> catch([TreatNonCallableAsNull] optional AnyCallback? rejectCallback = null);
 
-  [NewObject]
-  static Promise<any> all(sequence<any> iterable);
+  // Have to use "any" (or "object", but "any" is simpler) as the type to
+  // support the subclassing behavior, since nothing actually requires the
+  // return value of PromiseSubclass.all to be a Promise object.  As a result,
+  // we also have to do our argument conversion manually, because we want to
+  // convert its exceptions into rejections.
+  [NewObject, Throws]
+  static any all(optional any iterable);
 
   // Have to use "any" (or "object", but "any" is simpler) as the type to
   // support the subclassing behavior, since nothing actually requires the
   // return value of PromiseSubclass.race to be a Promise object.  As a result,
   // we also have to do our argument conversion manually, because we want to
   // convert its exceptions into rejections.
   [NewObject, Throws]
   static any race(optional any iterable);
--- a/testing/web-platform/tests/js/builtins/Promise-subclassing.html
+++ b/testing/web-platform/tests/js/builtins/Promise-subclassing.html
@@ -145,9 +145,23 @@ promise_test(function testPromiseRaceNoS
                             "Next 2", "Resolve 2",
                             "Next 3"]);
   assert_true(p instanceof SpeciesLessPromise);
   return p.then(function(arg) {
     assert_true(arg == 1 || arg == 2);
   });
 }, "Promise.race without species behavior");
 
+promise_test(function testPromiseAll() {
+  clearLog();
+  var p = LoggingPromise.all(new LoggingIterable([1, 2]));
+  var log = takeLog();
+  assert_array_equals(log, ["All 1", "Constructor 1",
+                            "Next 1", "Resolve 1",
+                            "Next 2", "Resolve 2",
+                            "Next 3"]);
+  assert_true(p instanceof LoggingPromise);
+  return p.then(function(arg) {
+    assert_array_equals(arg, [1, 2]);
+  });
+}, "Promise.all behavior");
+
 </script>