Change which ExceptionEvents are triggered by Promises

To make async/await catch prediction work well, this patch regularizes
the exception events sent to DevTools from various places in the Promise
lifecycle. The core is that there should be an exception event when the
rejection first starts, rather than when it is propagated.

- Several cases within Promise code which propagate errors are
  modified to not trigger a new ExceptionEvent in that case, such
  as .then on a rejected Promise and returning a rejected Promise
  from .then, as well as Promise.race and Promise.all.
- Make Promise.reject() create an ExceptionEvent, subject to catch
  prediction based on the Promise stack. This is important
  so that, e.g., if "await Promise.reject()" will trigger a new
  throw (rather than a silent rethrow of something that never
  triggered an event in the first place).

BUG=v8:5167

Review-Url: https://codereview.chromium.org/2244003003
Cr-Commit-Position: refs/heads/master@{#38847}

src/js/harmony-async-await.js

@@ -41,7 +41,8 @@ function AsyncFunctionAwait(generator, value) {
   var onRejected =
       (sentError) => %_Call(AsyncFunctionThrow, generator, sentError);
 
-  var throwawayCapability = NewPromiseCapability(GlobalPromise);
+  // false debugEvent to avoid redundant ExceptionEvents
+  var throwawayCapability = NewPromiseCapability(GlobalPromise, false);
   return PerformPromiseThen(promise, onFulfilled, onRejected,
                             throwawayCapability);
 }

src/js/promise.js

@@ -44,7 +44,7 @@ var lastMicrotaskId = 0;
 
 // ES#sec-createresolvingfunctions
 // CreateResolvingFunctions ( promise )
-function CreateResolvingFunctions(promise) {
+function CreateResolvingFunctions(promise, debugEvent) {
   var alreadyResolved = false;
 
   // ES#sec-promise-resolve-functions
@@ -60,7 +60,7 @@ function CreateResolvingFunctions(promise) {
   var reject = reason => {
     if (alreadyResolved === true) return;
     alreadyResolved = true;
-    RejectPromise(promise, reason);
+    RejectPromise(promise, reason, debugEvent);
   };
 
   return {
@@ -83,7 +83,8 @@ var GlobalPromise = function Promise(executor) {
   }
 
   var promise = PromiseInit(%_NewObject(GlobalPromise, new.target));
-  var callbacks = CreateResolvingFunctions(promise);
+  // Calling the reject function would be a new exception, so debugEvent = true
+  var callbacks = CreateResolvingFunctions(promise, true);
   var debug_is_active = DEBUG_IS_ACTIVE;
   try {
     if (debug_is_active) %DebugPushPromise(promise);
@@ -238,14 +239,16 @@ function PromiseCreate() {
 // Promise Resolve Functions, steps 6-13
 function ResolvePromise(promise, resolution) {
   if (resolution === promise) {
-    return RejectPromise(promise, %make_type_error(kPromiseCyclic, resolution));
+    return RejectPromise(promise,
+                         %make_type_error(kPromiseCyclic, resolution),
+                         true);
   }
   if (IS_RECEIVER(resolution)) {
     // 25.4.1.3.2 steps 8-12
     try {
       var then = resolution.then;
     } catch (e) {
-      return RejectPromise(promise, e);
+      return RejectPromise(promise, e, true);
     }
 
     // Resolution is a native promise and if it's already resolved or
@@ -268,7 +271,8 @@ function ResolvePromise(promise, resolution) {
           // Revoke previously triggered reject event.
           %PromiseRevokeReject(resolution);
         }
-        RejectPromise(promise, thenableValue);
+        // Don't cause a debug event as this case is forwarding a rejection
+        RejectPromise(promise, thenableValue, false);
         SET_PRIVATE(resolution, promiseHasHandlerSymbol, true);
         return;
       }
@@ -283,7 +287,9 @@ function ResolvePromise(promise, resolution) {
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
         }
-        var callbacks = CreateResolvingFunctions(promise);
+        // These resolving functions simply forward the exception, so
+        // don't create a new debugEvent.
+        var callbacks = CreateResolvingFunctions(promise, false);
         try {
           %_Call(then, resolution, callbacks.resolve, callbacks.reject);
         } catch (e) {
@@ -305,26 +311,35 @@ function ResolvePromise(promise, resolution) {
 
 // ES#sec-rejectpromise
 // RejectPromise ( promise, reason )
-function RejectPromise(promise, reason) {
+function RejectPromise(promise, reason, debugEvent) {
   // Check promise status to confirm that this reject has an effect.
   // Call runtime for callbacks to the debugger or for unhandled reject.
+  // The debugEvent parameter sets whether a debug ExceptionEvent should
+  // be triggered. It should be set to false when forwarding a rejection
+  // rather than creating a new one.
   if (GET_PRIVATE(promise, promiseStateSymbol) === kPending) {
-    var debug_is_active = DEBUG_IS_ACTIVE;
-    if (debug_is_active ||
+    // This check is redundant with checks in the runtime, but it may help
+    // avoid unnecessary runtime calls.
+    if ((debugEvent && DEBUG_IS_ACTIVE) ||
         !HAS_DEFINED_PRIVATE(promise, promiseHasHandlerSymbol)) {
-      %PromiseRejectEvent(promise, reason, debug_is_active);
+      %PromiseRejectEvent(promise, reason, debugEvent);
     }
   }
   FulfillPromise(promise, kRejected, reason, promiseRejectReactionsSymbol)
 }
 
+// Export to bindings
+function DoRejectPromise(promise, reason) {
+  return RejectPromise(promise, reason, true);
+}
+
 // ES#sec-newpromisecapability
 // NewPromiseCapability ( C )
-function NewPromiseCapability(C) {
+function NewPromiseCapability(C, debugEvent) {
   if (C === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseInit(new GlobalPromise(promiseRawSymbol));
-    var callbacks = CreateResolvingFunctions(promise);
+    var callbacks = CreateResolvingFunctions(promise, debugEvent);
     return {
       promise: promise,
       resolve: callbacks.resolve,
@@ -355,12 +370,12 @@ function PromiseReject(r) {
   if (this === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseCreateAndSet(kRejected, r);
-    // The debug event for this would always be an uncaught promise reject,
-    // which is usually simply noise. Do not trigger that debug event.
-    %PromiseRejectEvent(promise, r, false);
+    // Trigger debug events if the debugger is on, as Promise.reject is
+    // equivalent to throwing an exception directly.
+    %PromiseRejectEventFromStack(promise, r);
     return promise;
   } else {
-    var promiseCapability = NewPromiseCapability(this);
+    var promiseCapability = NewPromiseCapability(this, true);
     %_Call(promiseCapability.reject, UNDEFINED, r);
     return promiseCapability.promise;
   }
@@ -369,7 +384,11 @@ function PromiseReject(r) {
 // Shortcut Promise.reject and Promise.resolve() implementations, used by
 // Async Functions implementation.
 function PromiseCreateRejected(r) {
-  return %_Call(PromiseReject, GlobalPromise, r);
+  var promise = PromiseCreateAndSet(kRejected, r);
+  // This is called from the desugaring of async/await; no reason to
+  // create a redundant reject event.
+  %PromiseRejectEvent(promise, r, false);
+  return promise;
 }
 
 function PromiseCreateResolved(value) {
@@ -427,7 +446,9 @@ function PromiseThen(onResolve, onReject) {
   }
 
   var constructor = SpeciesConstructor(this, GlobalPromise);
-  var resultCapability = NewPromiseCapability(constructor);
+  // Pass false for debugEvent so .then chaining does not trigger
+  // redundant ExceptionEvents.
+  var resultCapability = NewPromiseCapability(constructor, false);
   return PerformPromiseThen(this, onResolve, onReject, resultCapability);
 }
 
@@ -454,7 +475,8 @@ function PromiseResolve(x) {
     return promise;
   }
 
-  var promiseCapability = NewPromiseCapability(this);
+  // debugEvent is not so meaningful here as it will be resolved
+  var promiseCapability = NewPromiseCapability(this, true);
   var resolveResult = %_Call(promiseCapability.resolve, UNDEFINED, x);
   return promiseCapability.promise;
 }
@@ -466,7 +488,9 @@ function PromiseAll(iterable) {
     throw %make_type_error(kCalledOnNonObject, "Promise.all");
   }
 
-  var deferred = NewPromiseCapability(this);
+  // false debugEvent so that forwarding the rejection through all does not
+  // trigger redundant ExceptionEvents
+  var deferred = NewPromiseCapability(this, false);
   var resolutions = new InternalArray();
   var count;
 
@@ -517,7 +541,9 @@ function PromiseRace(iterable) {
     throw %make_type_error(kCalledOnNonObject, PromiseRace);
   }
 
-  var deferred = NewPromiseCapability(this);
+  // false debugEvent so that forwarding the rejection through race does not
+  // trigger redundant ExceptionEvents
+  var deferred = NewPromiseCapability(this, false);
   try {
     for (var value of iterable) {
       this.resolve(value).then(deferred.resolve, deferred.reject);
@@ -598,7 +624,7 @@ utils.InstallFunctions(GlobalPromise.prototype, DONT_ENUM, [
   "promise_catch", PromiseCatch,
   "promise_create", PromiseCreate,
   "promise_has_user_defined_reject_handler", PromiseHasUserDefinedRejectHandler,
-  "promise_reject", RejectPromise,
+  "promise_reject", DoRejectPromise,
   "promise_resolve", ResolvePromise,
   "promise_then", PromiseThen,
   "promise_create_rejected", PromiseCreateRejected,
@@ -611,7 +637,7 @@ utils.InstallFunctions(GlobalPromise.prototype, DONT_ENUM, [
 utils.InstallFunctions(extrasUtils, 0, [
   "createPromise", PromiseCreate,
   "resolvePromise", ResolvePromise,
-  "rejectPromise", RejectPromise
+  "rejectPromise", DoRejectPromise
 ]);
 
 utils.Export(function(to) {

src/runtime/runtime-internal.cc

@@ -272,6 +272,23 @@ RUNTIME_FUNCTION(Runtime_ThrowApplyNonFunction) {
       isolate, NewTypeError(MessageTemplate::kApplyNonFunction, object, type));
 }
 
+namespace {
+
+void PromiseRejectEvent(Isolate* isolate, Handle<JSObject> promise,
+                        Handle<JSObject> rejected_promise, Handle<Object> value,
+                        bool debug_event) {
+  if (isolate->debug()->is_active() && debug_event) {
+    isolate->debug()->OnPromiseReject(rejected_promise, value);
+  }
+  Handle<Symbol> key = isolate->factory()->promise_has_handler_symbol();
+  // Do not report if we actually have a handler.
+  if (JSReceiver::GetDataProperty(promise, key)->IsUndefined(isolate)) {
+    isolate->ReportPromiseReject(promise, value,
+                                 v8::kPromiseRejectWithNoHandler);
+  }
+}
+
+}  // namespace
 
 RUNTIME_FUNCTION(Runtime_PromiseRejectEvent) {
   DCHECK(args.length() == 3);
@@ -279,16 +296,27 @@ RUNTIME_FUNCTION(Runtime_PromiseRejectEvent) {
   CONVERT_ARG_HANDLE_CHECKED(JSObject, promise, 0);
   CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
   CONVERT_BOOLEAN_ARG_CHECKED(debug_event, 2);
-  if (debug_event) isolate->debug()->OnPromiseReject(promise, value);
-  Handle<Symbol> key = isolate->factory()->promise_has_handler_symbol();
-  // Do not report if we actually have a handler.
-  if (JSReceiver::GetDataProperty(promise, key)->IsUndefined(isolate)) {
-    isolate->ReportPromiseReject(promise, value,
-                                 v8::kPromiseRejectWithNoHandler);
-  }
+
+  PromiseRejectEvent(isolate, promise, promise, value, debug_event);
   return isolate->heap()->undefined_value();
 }
 
+RUNTIME_FUNCTION(Runtime_PromiseRejectEventFromStack) {
+  DCHECK(args.length() == 2);
+  HandleScope scope(isolate);
+  CONVERT_ARG_HANDLE_CHECKED(JSObject, promise, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, value, 1);
+
+  Handle<JSObject> rejected_promise = promise;
+  if (isolate->debug()->is_active()) {
+    Handle<Object> promise_on_stack = isolate->GetPromiseOnStackOnThrow();
+    if (promise_on_stack->IsJSObject()) {
+      rejected_promise = Handle<JSObject>::cast(promise_on_stack);
+    }
+  }
+  PromiseRejectEvent(isolate, promise, rejected_promise, value, true);
+  return isolate->heap()->undefined_value();
+}
 
 RUNTIME_FUNCTION(Runtime_PromiseRevokeReject) {
   DCHECK(args.length() == 1);

src/runtime/runtime.h

@@ -304,6 +304,7 @@ namespace internal {
   F(NewTypeError, 2, 1)                             \
   F(OrdinaryHasInstance, 2, 1)                      \
   F(PromiseRejectEvent, 3, 1)                       \
+  F(PromiseRejectEventFromStack, 2, 1)              \
   F(PromiseRevokeReject, 1, 1)                      \
   F(PromoteScheduledException, 0, 1)                \
   F(ReThrow, 1, 1)                                  \

test/mjsunit/es6/debug-promises/promise-all-uncaught.js

@@ -6,13 +6,14 @@
 
 // Test debug events when we only listen to uncaught exceptions and a
 // Promise p3 created by Promise.all has no catch handler, and is rejected
-// because one of the Promises p2 passed to Promise.all is rejected. We
-// expect two Exception debug events to be triggered, for p2 and p3 each,
-// because neither has an user-defined catch handler.
+// because one of the Promises p2 passed to Promise.all is rejected.
+// We expect one event for p2; the system recognizes the rejection of p3
+// to be redundant and based on the rejection of p2 and does not trigger
+// an additional rejection.
 
 var Debug = debug.Debug;
 
-var expected_events = 2;
+var expected_events = 1;
 var log = [];
 
 var p1 = Promise.resolve();
@@ -35,13 +36,9 @@ function listener(event, exec_state, event_data, data) {
     assertTrue(expected_events >= 0);
     assertEquals("uncaught", event_data.exception().message);
     assertTrue(event_data.promise() instanceof Promise);
-    if (expected_events === 1) {
-      // Assert that the debug event is triggered at the throw site.
-      assertTrue(exec_state.frame(0).sourceLineText().indexOf("// event") > 0);
-      assertEquals("p2", event_data.promise().name);
-    } else {
-      assertEquals("p3", event_data.promise().name);
-    }
+    // Assert that the debug event is triggered at the throw site.
+    assertTrue(exec_state.frame(0).sourceLineText().indexOf("// event") > 0);
+    assertEquals("p2", event_data.promise().name);
     assertTrue(event_data.uncaught());
   } catch (e) {
     %AbortJS(e + "\n" + e.stack);

test/mjsunit/es6/debug-promises/promise-race-uncaught.js

@@ -6,13 +6,14 @@
 
 // Test debug events when we only listen to uncaught exceptions and a
 // Promise p3 created by Promise.race has no catch handler, and is rejected
-// because one of the Promises p2 passed to Promise.all is rejected. We
-// expect two Exception debug events to be triggered, for p2 and p3 each,
-// because neither has an user-defined catch handler.
+// because one of the Promises p2 passed to Promise.race is rejected.
+// We expect one event for p2; the system recognizes the rejection of p3
+// to be redundant and based on the rejection of p2 and does not trigger
+// an additional rejection.
 
 var Debug = debug.Debug;
 
-var expected_events = 2;
+var expected_events = 1;
 var log = [];
 
 var p1 = Promise.resolve();
@@ -35,13 +36,9 @@ function listener(event, exec_state, event_data, data) {
     assertTrue(expected_events >= 0);
     assertEquals("uncaught", event_data.exception().message);
     assertTrue(event_data.promise() instanceof Promise);
-    if (expected_events === 1) {
-      // Assert that the debug event is triggered at the throw site.
-      assertTrue(exec_state.frame(0).sourceLineText().indexOf("// event") > 0);
-      assertEquals("p2", event_data.promise().name);
-    } else {
-      assertEquals("p3", event_data.promise().name);
-    }
+    // Assert that the debug event is triggered at the throw site.
+    assertTrue(exec_state.frame(0).sourceLineText().indexOf("// event") > 0);
+    assertEquals("p2", event_data.promise().name);
     assertTrue(event_data.uncaught());
   } catch (e) {
     %AbortJS(e + "\n" + e.stack);

test/mjsunit/es6/debug-promises/reject-caught-by-default-reject-handler.js

@@ -44,8 +44,8 @@ function listener(event, exec_state, event_data, data) {
       assertTrue(event_data.uncaught());
       assertTrue(event_data.promise() instanceof Promise);
       if (expected_events == 1) {
-        // p1 is rejected, uncaught except for its default reject handler.
-        assertEquals(0, exec_state.frameCount());
+        // p1 is rejected, uncaught, with the error from the Promise.reject line
+        assertNotNull(event_data.sourceLineText().match("Promise.reject"));
         assertSame(p1, event_data.promise());
       } else {
         // p2 is rejected by p1's default reject handler.

test/mjsunit/es6/debug-promises/reject-uncaught-all.js

@@ -33,8 +33,8 @@ function listener(event, exec_state, event_data, data) {
       assertTrue(event_data.promise() instanceof Promise);
       assertSame(q, event_data.promise());
       assertTrue(event_data.uncaught());
-      // All of the frames on the stack are from native Javascript.
-      assertEquals(0, exec_state.frameCount());
+      // The frame comes from the Promise.reject call
+      assertNotNull(/Promise\.reject/.exec(event_data.sourceLineText()));
     }
   } catch (e) {
     %AbortJS(e + "\n" + e.stack);

test/mjsunit/es6/debug-promises/reject-uncaught-uncaught.js

@@ -33,8 +33,8 @@ function listener(event, exec_state, event_data, data) {
       assertTrue(event_data.promise() instanceof Promise);
       assertSame(q, event_data.promise());
       assertTrue(event_data.uncaught());
-      // All of the frames on the stack are from native Javascript.
-      assertEquals(0, exec_state.frameCount());
+      // The JavaScript frame is from the Promise rejection
+      assertTrue(/Promise\.reject/.test(event_data.sourceLineText()));
     }
   } catch (e) {
     %AbortJS(e + "\n" + e.stack);

test/mjsunit/harmony/async-debug-caught-exception.js

@@ -87,3 +87,37 @@ Debug.setListener(null);
 Debug.clearBreakOnUncaughtException();
 assertEquals([], log);
 assertNull(exception);
+
+log = [];
+Debug.setListener(listener);
+Debug.setBreakOnException();
+
+// "rethrown" uncaught exceptions in return don't cause another event
+async function propagate_inner() { return thrower(); }
+async function propagate_outer() { return propagate_inner(); }
+
+propagate_outer();
+%RunMicrotasks();
+assertEquals(["a"], log);
+assertNull(exception);
+
+// Also don't propagate if an await interceded
+log = [];
+async function propagate_await() { await 1; return thrower(); }
+async function propagate_await_outer() { return propagate_await(); }
+propagate_await_outer();
+%RunMicrotasks();
+assertEquals(["a"], log);
+assertNull(exception);
+
+Debug.clearBreakOnException();
+Debug.setBreakOnUncaughtException();
+
+log = [];
+Promise.resolve().then(() => Promise.reject()).catch(() => log.push("d")); // Exception c
+%RunMicrotasks();
+assertEquals(["d"], log);
+assertNull(exception);
+
+Debug.clearBreakOnUncaughtException();
+Debug.setListener(null);