Bug 1314386 - Properly handle a primitive-returning Promise.resolve in Promise.all. r=arai
authorTill Schneidereit <till@tillschneidereit.net>
Thu, 03 Nov 2016 16:17:47 +0100
changeset 347579 33c17b8aae5aa72927594654865450427052f2fd
parent 347578 92c5b8d103dd531deaf3715864a61239cd5d49df
child 347580 d3041026a9219ae3fdf6b6e081a2a0e679b23af7
push id10298
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:33:03 +0000
treeherdermozilla-aurora@7e29173b1641 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersarai
bugs1314386
milestone52.0a1
Bug 1314386 - Properly handle a primitive-returning Promise.resolve in Promise.all. r=arai MozReview-Commit-ID: GCWH4QPag6E
js/src/builtin/Promise.cpp
js/src/jit-test/tests/promise/primitives-handling-in-promise-all.js
--- a/js/src/builtin/Promise.cpp
+++ b/js/src/builtin/Promise.cpp
@@ -1011,17 +1011,17 @@ static MOZ_MUST_USE bool
 AddPromiseReaction(JSContext* cx, Handle<PromiseObject*> promise, HandleValue onFulfilled,
                    HandleValue onRejected, HandleObject dependentPromise,
                    HandleObject resolve, HandleObject reject, HandleObject incumbentGlobal);
 
 static MOZ_MUST_USE bool
 AddPromiseReaction(JSContext* cx, Handle<PromiseObject*> promise,
                    Handle<PromiseReactionRecord*> reaction);
 
-static MOZ_MUST_USE bool BlockOnPromise(JSContext* cx, HandleObject promise,
+static MOZ_MUST_USE bool BlockOnPromise(JSContext* cx, HandleValue promise,
                                         HandleObject blockedPromise,
                                         HandleValue onFulfilled, HandleValue onRejected);
 
 static JSFunction*
 GetResolveFunctionFromReject(JSFunction* reject)
 {
     MOZ_ASSERT(reject->maybeNative() == RejectPromiseFunction);
     Value resolveFunVal = reject->getExtendedSlot(RejectFunctionSlot_ResolveFunction);
@@ -1651,19 +1651,18 @@ PerformPromiseAll(JSContext *cx, JS::For
         // Step l.
         resolveFunc->setExtendedSlot(PromiseAllResolveElementFunctionSlot_ElementIndex,
                                      Int32Value(index));
 
         // Steps o-p.
         dataHolder->increaseRemainingCount();
 
         // Step q.
-        RootedObject nextPromiseObj(cx, &nextPromise.toObject());
         RootedValue resolveFunVal(cx, ObjectValue(*resolveFunc));
-        if (!BlockOnPromise(cx, nextPromiseObj, promiseObj, resolveFunVal, rejectFunVal))
+        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal))
             return false;
 
         // Step r.
         index++;
         MOZ_ASSERT(index > 0);
     }
 }
 
@@ -1840,18 +1839,17 @@ PerformPromiseRace(JSContext *cx, JS::Fo
             return false;
 
         FixedInvokeArgs<1> resolveArgs(cx);
         resolveArgs[0].set(nextValue);
         if (!Call(cx, staticResolve, CVal, resolveArgs, &nextPromise))
             return false;
 
         // Step i.
-        RootedObject nextPromiseObj(cx, &nextPromise.toObject());
-        if (!BlockOnPromise(cx, nextPromiseObj, promiseObj, resolveFunVal, rejectFunVal))
+        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal))
             return false;
     }
 
     MOZ_ASSERT_UNREACHABLE("Shouldn't reach the end of PerformPromiseRace");
 }
 
 // ES2016, Sub-steps of 25.4.4.4 and 25.4.4.5.
 static MOZ_MUST_USE bool
@@ -2171,24 +2169,28 @@ PerformPromiseThen(JSContext* cx, Handle
  * its list of dependent promises. Used by |Promise.all| and |Promise.race|.
  *
  * If |promise.then| is the original |Promise.prototype.then| function and
  * the call to |promise.then| would use the original |Promise| constructor to
  * create the resulting promise, this function skips the call to |promise.then|
  * and thus creating a new promise that would not be observable by content.
  */
 static MOZ_MUST_USE bool
-BlockOnPromise(JSContext* cx, HandleObject promiseObj, HandleObject blockedPromise_,
+BlockOnPromise(JSContext* cx, HandleValue promiseVal, HandleObject blockedPromise_,
                HandleValue onFulfilled, HandleValue onRejected)
 {
     RootedValue thenVal(cx);
-    if (!GetProperty(cx, promiseObj, promiseObj, cx->names().then, &thenVal))
+    if (!GetProperty(cx, promiseVal, cx->names().then, &thenVal))
         return false;
 
-    if (promiseObj->is<PromiseObject>() && IsNativeFunction(thenVal, Promise_then)) {
+    RootedObject promiseObj(cx);
+    if (promiseVal.isObject())
+        promiseObj = &promiseVal.toObject();
+
+    if (promiseObj && promiseObj->is<PromiseObject>() && IsNativeFunction(thenVal, Promise_then)) {
         // |promise| is an unwrapped Promise, and |then| is the original
         // |Promise.prototype.then|, inline it here.
         // 25.4.5.3., step 3.
         RootedObject PromiseCtor(cx);
         if (!GetBuiltinConstructor(cx, JSProto_Promise, &PromiseCtor))
             return false;
         RootedValue PromiseCtorVal(cx, ObjectValue(*PromiseCtor));
         RootedValue CVal(cx);
@@ -2219,22 +2221,29 @@ BlockOnPromise(JSContext* cx, HandleObje
         {
             return false;
         }
 
         if (!addToDependent)
             return true;
     } else {
         // Optimization failed, do the normal call.
-        RootedValue promiseVal(cx, ObjectValue(*promiseObj));
         RootedValue rval(cx);
         if (!Call(cx, thenVal, promiseVal, onFulfilled, onRejected, &rval))
             return false;
     }
 
+    // In case the value to depend on isn't an object at all, there's nothing
+    // more to do here: we can only add reactions to Promise objects
+    // (potentially after unwrapping them), and non-object values can't be
+    // Promise objects. This can happen if Promise.all is called on an object
+    // with a `resolve` method that returns primitives.
+    if (!promiseObj)
+        return true;
+
     // The object created by the |promise.then| call or the inlined version
     // of it above is visible to content (either because |promise.then| was
     // overridden by content and could leak it, or because a constructor
     // other than the original value of |Promise| was used to create it).
     // To have both that object and |blockedPromise| show up as dependent
     // promises in the debugger, add a dummy reaction to the list of reject
     // reactions that contains |blockedPromise|, but otherwise does nothing.
     RootedObject unwrappedPromiseObj(cx, promiseObj);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/promise/primitives-handling-in-promise-all.js
@@ -0,0 +1,3 @@
+// This just shouldn't crash.
+Promise.resolve = () => 42;
+Promise.all([1]);