Bug 1474348 - Part 2: Don't use BlockOnPromise fast path with non-default resolving functions. r=arai
authorAndré Bargull <andre.bargull@gmail.com>
Mon, 09 Jul 2018 09:49:23 -0700
changeset 425729 ff0152ae26bd6b6ea3d57ec80289e6b3fb79c4b0
parent 425728 f80f0ce6287322d4fa4595c60da063cb1c0e22d1
child 425730 1f836d71ddc52a92a705feb9dc791413bf63bf6f
push id34262
push usercsabou@mozilla.com
push dateTue, 10 Jul 2018 21:51:50 +0000
treeherdermozilla-central@70f901964f97 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersarai
bugs1474348
milestone63.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 1474348 - Part 2: Don't use BlockOnPromise fast path with non-default resolving functions. r=arai
js/src/builtin/Promise.cpp
js/src/jit-test/tests/promise/promise-race-with-default-resolving-internal.js
js/src/jit-test/tests/promise/promise-race-with-non-default-resolving.js
--- a/js/src/builtin/Promise.cpp
+++ b/js/src/builtin/Promise.cpp
@@ -1655,19 +1655,19 @@ EnqueuePromiseResolveThenableBuiltinJob(
 static MOZ_MUST_USE bool
 AddDummyPromiseReactionForDebugger(JSContext* cx, Handle<PromiseObject*> promise,
                                    HandleObject dependentPromise);
 
 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)
 {
     MOZ_ASSERT(reject->maybeNative() == RejectPromiseFunction);
     Value resolveFunVal = reject->getExtendedSlot(RejectFunctionSlot_ResolveFunction);
     MOZ_ASSERT(IsNativeFunction(resolveFunVal, ResolvePromiseFunction));
     return &resolveFunVal.toObject().as<JSFunction>();
@@ -2341,17 +2341,17 @@ PerformPromiseAll(JSContext *cx, JS::For
         resolveFunc->setExtendedSlot(PromiseAllResolveElementFunctionSlot_ElementIndex,
                                      Int32Value(index));
 
         // Steps o-p.
         dataHolder->increaseRemainingCount();
 
         // 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.
         index++;
         MOZ_ASSERT(index > 0);
     }
 }
 
@@ -2499,16 +2499,21 @@ static MOZ_MUST_USE bool
 PerformPromiseRace(JSContext *cx, JS::ForOfIterator& iterator, HandleObject C,
                    HandleObject promiseObj, HandleObject resolve, HandleObject reject,
                    bool* done)
 {
     *done = false;
     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));
 
     while (true) {
         // Steps a-c, e-g.
         if (!iterator.next(&nextValue, done)) {
             // Steps b, f.
@@ -2535,18 +2540,21 @@ 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.
-        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");
 }
 
 
 // ES2016, Sub-steps of 25.4.4.4 and 25.4.4.5.
 static MOZ_MUST_USE JSObject*
@@ -3597,17 +3605,17 @@ PerformPromiseThenWithReaction(JSContext
  *
  * 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, HandleValue promiseVal, HandleObject blockedPromise_,
-               HandleValue onFulfilled, HandleValue onRejected)
+               HandleValue onFulfilled, HandleValue onRejected, bool onFulfilledReturnsUndefined)
 {
     RootedObject promiseObj(cx, ToObject(cx, promiseVal));
     if (!promiseObj)
         return false;
 
     RootedValue thenVal(cx);
     if (!GetProperty(cx, promiseObj, promiseVal, cx->names().then, &thenVal))
         return false;
@@ -3620,25 +3628,39 @@ BlockOnPromise(JSContext* cx, HandleValu
                                  GlobalObject::getOrCreatePromiseConstructor(cx, cx->global()));
         if (!PromiseCtor)
             return false;
 
         RootedObject C(cx, SpeciesConstructor(cx, promiseObj, JSProto_Promise, IsPromiseSpecies));
         if (!C)
             return false;
 
-        RootedObject resultPromise(cx, blockedPromise_);
+        RootedObject resultPromise(cx);
         RootedObject resolveFun(cx);
         RootedObject rejectFun(cx);
 
         // By default, the blocked promise is added as an extra entry to the
         // 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.
             if (!NewPromiseCapability(cx, C, &resultPromise, &resolveFun, &rejectFun, true))
                 return false;
         }
 
         // 25.4.5.3., step 5.
new file mode 100644
--- /dev/null
+++ b/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();
new file mode 100644
--- /dev/null
+++ b/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);