Issue #2089 - Don't use BlockOnPromise fast path with non-default resolving functions, combine default resolving functions flags

Based-on: m-c 1474348

js/src/builtin/Promise.cpp

@@ -540,14 +540,14 @@ const Class PromiseReactionRecord::class_ = {
 static void
 AddPromiseFlags(PromiseObject& promise, int32_t flag)
 {
-    int32_t flags = promise.getFixedSlot(PromiseSlot_Flags).toInt32();
+    int32_t flags = promise.flags();
     promise.setFixedSlot(PromiseSlot_Flags, Int32Value(flags | flag));
 }
 
 static bool
 PromiseHasAnyFlag(PromiseObject& promise, int32_t flag)
 {
-    return promise.getFixedSlot(PromiseSlot_Flags).toInt32() & flag;
+    return promise.flags() & flag;
 }
 
 static bool ResolvePromiseFunction(JSContext* cx, unsigned argc, Value* vp);
@@ -922,7 +922,7 @@ ResolvePromise(JSContext* cx, Handle<PromiseObject*> promise, HandleValue valueO
     // instead of getting the right list of reactions, we determine the
     // resolution type to retrieve the right information from the
     // reaction records.
-    RootedValue reactionsVal(cx, promise->getFixedSlot(PromiseSlot_ReactionsOrResult));
+    RootedValue reactionsVal(cx, promise->reactions());
 
     // Steps 3-5.
     // The same slot is used for the reactions list and the result, so setting
@@ -930,7 +930,7 @@ ResolvePromise(JSContext* cx, Handle<PromiseObject*> promise, HandleValue valueO
     promise->setFixedSlot(PromiseSlot_ReactionsOrResult, valueOrReason);
 
     // Step 6.
-    int32_t flags = promise->getFixedSlot(PromiseSlot_Flags).toInt32();
+    int32_t flags = promise->flags();
     flags |= PROMISE_FLAG_RESOLVED;
     if (state == JS::PromiseState::Fulfilled)
         flags |= PROMISE_FLAG_FULFILLED;
@@ -994,8 +994,7 @@ CreatePromiseObjectWithoutResolutionFunctions(JSContext* cx)
     if (!promise)
         return nullptr;
 
-    AddPromiseFlags(*promise, PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION |
-                    PROMISE_FLAG_DEFAULT_REJECT_FUNCTION);
+    AddPromiseFlags(*promise, PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS);
     return promise;
 }
 
@@ -1643,9 +1642,9 @@ static MOZ_MUST_USE bool
 AddPromiseReaction(JSContext* cx, Handle<PromiseObject*> promise,
                    Handle<PromiseReactionRecord*> reaction);
 
-static MOZ_MUST_USE bool BlockOnPromise(JSContext* cx, HandleValue promise,
-                                        HandleObject blockedPromise,
-                                        HandleValue onFulfilled, HandleValue onRejected);
+static MOZ_MUST_USE bool
+BlockOnPromise(JSContext* cx, HandleValue promise, HandleObject blockedPromise,
+               HandleValue onFulfilled, HandleValue onRejected, bool onFulfilledReturnsUndefined);
 
 static JSFunction*
 GetResolveFunctionFromReject(JSFunction* reject)
@@ -2210,14 +2209,12 @@ RunResolutionFunction(JSContext *cx, HandleObject resolutionFun, HandleValue res
     if (promise->state() != JS::PromiseState::Pending)
         return true;
 
-    if (mode == ResolveMode) {
-        if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION))
-            return true;
-        return ResolvePromiseInternal(cx, promise, result);
-    }
-
-    if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_REJECT_FUNCTION))
+    if (!PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS))
         return true;
+
+    if (mode == ResolveMode)
+        return ResolvePromiseInternal(cx, promise, result);
+
     return RejectMaybeWrappedPromise(cx, promiseObj, result);
 }
 
@@ -2364,7 +2361,7 @@ PerformPromiseAll(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
 
         // Step q.
         RootedValue resolveFunVal(cx, ObjectValue(*resolveFunc));
-        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal))
+        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal, true))
             return false;
 
         // Step r.
@@ -2479,7 +2476,7 @@ PerformPromiseAllSettled(JSContext *cx, JS::ForOfIterator& iterator, HandleObjec
 
         RootedValue resolveFunVal(cx, ObjectValue(*resolveFunc));
         RootedValue rejectFunVal(cx, ObjectValue(*rejectFunc));
-        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal))
+        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal, true))
             return false;
 
         index++;
@@ -2785,6 +2782,11 @@ PerformPromiseRace(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
     MOZ_ASSERT(C->isConstructor());
     RootedValue CVal(cx, ObjectValue(*C));
 
+    // BlockOnPromise fast path requires the passed onFulfilled function
+    // doesn't return an object value, because otherwise the skipped promise
+    // creation is detectable due to missing property lookups.
+    bool isDefaultResolveFn = IsNativeFunction(resolve, ResolvePromiseFunction);
+
     RootedValue nextValue(cx);
     RootedValue resolveFunVal(cx, ObjectValue(*resolve));
     RootedValue rejectFunVal(cx, ObjectValue(*reject));
@@ -2821,8 +2823,11 @@ PerformPromiseRace(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
             return false;
 
         // Step i.
-        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal))
+        if (!BlockOnPromise(cx, nextPromise, promiseObj, resolveFunVal, rejectFunVal,
+                            isDefaultResolveFn))
+        {
             return false;
+        }
     }
 
     MOZ_ASSERT_UNREACHABLE("Shouldn't reach the end of PerformPromiseRace");
@@ -3826,7 +3831,7 @@ PerformPromiseThenWithReaction(JSContext* cx, Handle<PromiseObject*> promise,
                                Handle<PromiseReactionRecord*> reaction)
 {
     JS::PromiseState state = promise->state();
-    int32_t flags = promise->getFixedSlot(PromiseSlot_Flags).toInt32();
+    int32_t flags = promise->flags();
     if (state == JS::PromiseState::Pending) {
         // Steps 5,6 (reordered).
         // Instead of creating separate reaction records for fulfillment and
@@ -3842,7 +3847,7 @@ PerformPromiseThenWithReaction(JSContext* cx, Handle<PromiseObject*> promise,
         MOZ_ASSERT_IF(state != JS::PromiseState::Fulfilled, state == JS::PromiseState::Rejected);
 
         // Step 8.a. / 9.b.
-        RootedValue valueOrReason(cx, promise->getFixedSlot(PromiseSlot_ReactionsOrResult));
+        RootedValue valueOrReason(cx, promise->valueOrReason());
 
         // We might be operating on a promise from another compartment. In
         // that case, we need to wrap the result/reason value before using it.
@@ -3876,7 +3881,7 @@ PerformPromiseThenWithReaction(JSContext* cx, Handle<PromiseObject*> promise,
  */
 static MOZ_MUST_USE bool
 BlockOnPromise(JSContext* cx, HandleValue promiseVal, HandleObject blockedPromise_,
-               HandleValue onFulfilled, HandleValue onRejected)
+               HandleValue onFulfilled, HandleValue onRejected, bool onFulfilledReturnsUndefined)
 {
     RootedObject promiseObj(cx, ToObject(cx, promiseVal));
     if (!promiseObj)
@@ -3898,7 +3903,7 @@ BlockOnPromise(JSContext* cx, HandleValue promiseVal, HandleObject blockedPromis
         if (!C)
             return false;
 
-        RootedObject resultPromise(cx, blockedPromise_);
+        RootedObject resultPromise(cx);
         RootedObject resolveFun(cx);
         RootedObject rejectFun(cx);
 
@@ -3906,7 +3911,21 @@ BlockOnPromise(JSContext* cx, HandleValue promiseVal, HandleObject blockedPromis
         // rejected promises list.
         bool addToDependent = true;
 
-        if (C == PromiseCtor && resultPromise->is<PromiseObject>()) {
+        // Skip the creation of a built-in Promise object if:
+        // 1. `C` is the built-in Promise constructor.
+        // 2. The `onFulfilled` handler doesn't return an object, which
+        //    ensures no side-effects take place in ResolvePromiseInternal.
+        // 3. The blocked promise is a built-in Promise object.
+        // 4. The blocked promise doesn't use the default resolving functions,
+        //    which in turn means RunResolutionFunction when called from
+        //    PromiseRectionJob won't try to resolve the promise.
+        if (C == PromiseCtor &&
+            onFulfilledReturnsUndefined &&
+            blockedPromise_->is<PromiseObject>() &&
+            !PromiseHasAnyFlag(blockedPromise_->as<PromiseObject>(),
+                               PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS))
+        {
+            resultPromise.set(blockedPromise_);
             addToDependent = false;
         } else {
             // 25.4.5.3., step 4.
@@ -3997,7 +4016,7 @@ AddPromiseReaction(JSContext* cx, Handle<PromiseObject*> promise,
     }
 
     // 25.4.5.3.1 steps 7.a,b.
-    RootedValue reactionsVal(cx, promise->getFixedSlot(PromiseSlot_ReactionsOrResult));
+    RootedValue reactionsVal(cx, promise->reactions());
     RootedNativeObject reactions(cx);
 
     if (reactionsVal.isUndefined()) {
@@ -4093,7 +4112,7 @@ PromiseObject::dependentPromises(JSContext* cx, MutableHandle<GCVector<Value>> v
     if (state() != JS::PromiseState::Pending)
         return true;
 
-    RootedValue reactionsVal(cx, getFixedSlot(PromiseSlot_ReactionsOrResult));
+    RootedValue reactionsVal(cx, reactions());
 
     // If no reactions are pending, we don't have list and are done.
     if (reactionsVal.isNullOrUndefined())
@@ -4143,7 +4162,7 @@ PromiseObject::resolve(JSContext* cx, Handle<PromiseObject*> promise, HandleValu
     if (promise->state() != JS::PromiseState::Pending)
         return true;
 
-    if (PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION))
+    if (PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS))
         return ResolvePromiseInternal(cx, promise, resolutionValue);
 
     RootedObject resolveFun(cx, GetResolveFunctionFromPromise(promise));
@@ -4172,7 +4191,7 @@ PromiseObject::reject(JSContext* cx, Handle<PromiseObject*> promise, HandleValue
     if (promise->state() != JS::PromiseState::Pending)
         return true;
 
-    if (PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_REJECT_FUNCTION))
+    if (PromiseHasAnyFlag(*promise, PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS))
         return RejectMaybeWrappedPromise(cx, promise, rejectionValue);
 
     RootedValue funVal(cx, promise->getFixedSlot(PromiseSlot_RejectFunction));

js/src/builtin/Promise.h

@@ -24,9 +24,8 @@ enum PromiseSlots {
 #define PROMISE_FLAG_FULFILLED 0x2
 #define PROMISE_FLAG_HANDLED   0x4
 #define PROMISE_FLAG_REPORTED  0x8
-#define PROMISE_FLAG_DEFAULT_RESOLVE_FUNCTION 0x10
-#define PROMISE_FLAG_DEFAULT_REJECT_FUNCTION  0x20
-#define PROMISE_FLAG_ASYNC    0x40
+#define PROMISE_FLAG_DEFAULT_RESOLVING_FUNCTIONS 0x10
+#define PROMISE_FLAG_ASYNC    0x20
 
 class AutoSetNewObjectMetadata;
 
@@ -44,8 +43,11 @@ class PromiseObject : public NativeObject
     static JSObject* unforgeableResolve(JSContext* cx, HandleValue value);
     static JSObject* unforgeableReject(JSContext* cx, HandleValue value);
 
+    int32_t flags() {
+        return getFixedSlot(PromiseSlot_Flags).toInt32();
+    }
     JS::PromiseState state() {
-        int32_t flags = getFixedSlot(PromiseSlot_Flags).toInt32();
+        int32_t flags = this->flags();
         if (!(flags & PROMISE_FLAG_RESOLVED)) {
             MOZ_ASSERT(!(flags & PROMISE_FLAG_FULFILLED));
             return JS::PromiseState::Pending;
@@ -54,6 +56,10 @@ class PromiseObject : public NativeObject
             return JS::PromiseState::Fulfilled;
         return JS::PromiseState::Rejected;
     }
+    Value reactions() {
+        MOZ_ASSERT(state() == JS::PromiseState::Pending);
+        return getFixedSlot(PromiseSlot_ReactionsOrResult);
+    }
     Value value()  {
         MOZ_ASSERT(state() == JS::PromiseState::Fulfilled);
         return getFixedSlot(PromiseSlot_ReactionsOrResult);
@@ -62,6 +68,10 @@ class PromiseObject : public NativeObject
         MOZ_ASSERT(state() == JS::PromiseState::Rejected);
         return getFixedSlot(PromiseSlot_ReactionsOrResult);
     }
+    Value valueOrReason()  {
+        MOZ_ASSERT(state() != JS::PromiseState::Pending);
+        return getFixedSlot(PromiseSlot_ReactionsOrResult);
+    }
 
     static MOZ_MUST_USE bool resolve(JSContext* cx, Handle<PromiseObject*> promise,
                                      HandleValue resolutionValue);
@@ -83,7 +93,7 @@ class PromiseObject : public NativeObject
     uint64_t getID();
     bool isUnhandled() {
         MOZ_ASSERT(state() == JS::PromiseState::Rejected);
-        return !(getFixedSlot(PromiseSlot_Flags).toInt32() & PROMISE_FLAG_HANDLED);
+        return !(flags() & PROMISE_FLAG_HANDLED);
     }
     void markAsReported() {
         MOZ_ASSERT(isUnhandled());

js/src/builtin/TestingFunctions.cpp

@@ -1334,8 +1334,8 @@ SettlePromiseNow(JSContext* cx, unsigned argc, Value* vp)
         return false;
     }
 
-    RootedNativeObject promise(cx, &args[0].toObject().as<NativeObject>());
-    int32_t flags = promise->getFixedSlot(PromiseSlot_Flags).toInt32();
+    Rooted<PromiseObject*> promise(cx, &args[0].toObject().as<PromiseObject>());
+    int32_t flags = promise->flags();
     promise->setFixedSlot(PromiseSlot_Flags,
                           Int32Value(flags | PROMISE_FLAG_RESOLVED | PROMISE_FLAG_FULFILLED));
     promise->setFixedSlot(PromiseSlot_ReactionsOrResult, UndefinedValue());

js/src/jit-test/tests/promise/promise-race-with-default-resolving-internal.js

@@ -0,0 +1,54 @@
+function newPromiseCapability() {
+    let resolve, reject, promise = new Promise(function(r1, r2) {
+        resolve = r1;
+        reject = r2;
+    });
+    return {promise, resolve, reject};
+}
+
+function neverCalled() {
+    // Quit with non-zero exit code to ensure a test suite error is shown,
+    // even when this function is called within promise handlers which normally
+    // swallow any exceptions.
+    quit(1);
+}
+
+var c = 0;
+var g_resolve;
+
+class P extends Promise {
+    constructor(executor) {
+        // Only the very first object created through this constructor gets
+        // special treatment, all other invocations create built-in Promise
+        // objects.
+        if (c++ > 1) {
+            return new Promise(executor);
+        }
+
+        // Pass a native ResolvePromiseFunction function as the resolve handler.
+        // (It's okay that the promise of this promise capability is never used.)
+        executor(newPromiseCapability().resolve, neverCalled);
+
+        let {promise, resolve} = newPromiseCapability();
+        g_resolve = resolve;
+
+        // Use an async function to create a Promise without resolving functions.
+        return async function(){ await promise; return 456; }();
+    }
+
+    // Ensure we don't take the (spec) fast path in Promise.resolve and instead
+    // create a new promise object. (We could not provide an override at all
+    // and rely on the default behaviour, but giving an explicit definition
+    // may help to interpret this test case.)
+    static resolve(v) {
+        return super.resolve(v);
+    }
+}
+
+let {promise: alwaysPending} = newPromiseCapability();
+
+P.race([alwaysPending]).then(neverCalled, neverCalled);
+
+g_resolve(123);
+
+drainJobQueue();

js/src/jit-test/tests/promise/promise-race-with-non-default-resolving.js

@@ -0,0 +1,56 @@
+function newPromiseCapability() {
+    var resolve, reject, promise = new Promise(function(r1, r2) {
+        resolve = r1;
+        reject = r2;
+    });
+    return {promise, resolve, reject};
+}
+
+function neverCalled() {
+    // Quit with non-zero exit code to ensure a test suite error is shown,
+    // even when this function is called within promise handlers which normally
+    // swallow any exceptions.
+    quit(1);
+}
+
+var {promise, resolve} = newPromiseCapability();
+
+var getterCount = 0;
+
+class P extends Promise {
+    constructor(executor) {
+        var {promise, resolve, reject} = newPromiseCapability();
+
+        executor(function(v) {
+            // Resolve the promise.
+            resolve(v);
+
+            // But then return an object from the resolve function. This object
+            // must be treated as the resolution value for the otherwise
+            // skipped promise which gets created when Promise.prototype.then is
+            // called in PerformPromiseRace.
+            return {
+                get then() {
+                    getterCount++;
+                }
+            };
+        }, neverCalled);
+
+        return promise;
+    }
+
+    // Default to the standard Promise.resolve function, so we don't create
+    // another instance of this class when resolving the passed promise objects
+    // in Promise.race.
+    static resolve(v) {
+        return Promise.resolve(v);
+    }
+}
+
+P.race([promise]);
+
+resolve(0);
+
+drainJobQueue();
+
+assertEq(getterCount, 1);