Change which ExceptionEvents are triggered by Promises

src/js/promise.js

Index: src/js/promise.js
diff --git a/src/js/promise.js b/src/js/promise.js
index b50fc80b305bdcfd509e479cfb703171b85eac9b..34cf784581fc325aa55c66e3c5faf77ec35083af 100644
--- a/src/js/promise.js
+++ b/src/js/promise.js
@@ -44,7 +44,7 @@ var lastMicrotaskId = 0;
 
 // ES#sec-createresolvingfunctions
 // CreateResolvingFunctions ( promise )
-function CreateResolvingFunctions(promise) {
+function CreateResolvingFunctions(promise, suppressDebugEvent) {
   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, suppressDebugEvent);
   };
 
   return {
@@ -268,7 +268,7 @@ function ResolvePromise(promise, resolution) {
           // Revoke previously triggered reject event.
           %PromiseRevokeReject(resolution);
         }
-        RejectPromise(promise, thenableValue);
+        RejectPromise(promise, thenableValue, true);
         SET_PRIVATE(resolution, promiseHasHandlerSymbol, true);
         return;
       }
@@ -283,7 +283,7 @@ function ResolvePromise(promise, resolution) {
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
         }
-        var callbacks = CreateResolvingFunctions(promise);
+        var callbacks = CreateResolvingFunctions(promise, true);
         try {
           %_Call(then, resolution, callbacks.resolve, callbacks.reject);
         } catch (e) {
@@ -305,14 +305,14 @@ function ResolvePromise(promise, resolution) {
 
 // ES#sec-rejectpromise
 // RejectPromise ( promise, reason )
-function RejectPromise(promise, reason) {
+function RejectPromise(promise, reason, suppressDebugEvent) {

[adamk, 2016/08/23 22:29:27]

It's confusing to me that %PromiseRejectEvent takes a boolean that's inverted
from this one. Can you change them to match? Either change this one to
"emitDebugEvent" or similar, or change the runtime function to make its argument
mean "suppress". If you do the latter, you could avoid passing the
DEBUG_IS_ACTIVE bit, since I assume it's easy to get ahold of that from C++
anyway.

[Dan Ehrenberg, 2016/08/23 23:28:38]

Good point. Switched it to debugEvent as a positive reason everywhere. That
change motivated an audit of the code base (since I'm not depending on a default
argument anymore) to see whether events should be triggered, and I found that we
probably shouldn't be triggering these events for the output of Promise.all and
Promise.race, so this patch fixes that.

   // Check promise status to confirm that this reject has an effect.
   // Call runtime for callbacks to the debugger or for unhandled reject.
   if (GET_PRIVATE(promise, promiseStateSymbol) === kPending) {
-    var debug_is_active = DEBUG_IS_ACTIVE;
-    if (debug_is_active ||
+    var debugEvent = DEBUG_IS_ACTIVE && !suppressDebugEvent
+    if (debugEvent ||
         !HAS_DEFINED_PRIVATE(promise, promiseHasHandlerSymbol)) {
-      %PromiseRejectEvent(promise, reason, debug_is_active);
+      %PromiseRejectEvent(promise, reason, debugEvent, false);
     }
   }
   FulfillPromise(promise, kRejected, reason, promiseRejectReactionsSymbol)
@@ -320,11 +320,11 @@ function RejectPromise(promise, reason) {
 
 // ES#sec-newpromisecapability
 // NewPromiseCapability ( C )
-function NewPromiseCapability(C) {
+function NewPromiseCapability(C, suppressDebugEvent) {
   if (C === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseInit(new GlobalPromise(promiseRawSymbol));
-    var callbacks = CreateResolvingFunctions(promise);
+    var callbacks = CreateResolvingFunctions(promise, suppressDebugEvent);
     return {
       promise: promise,
       resolve: callbacks.resolve,
@@ -355,9 +355,9 @@ 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.
+    %PromiseRejectEvent(promise, r, DEBUG_IS_ACTIVE, true);
     return promise;
   } else {
     var promiseCapability = NewPromiseCapability(this);
@@ -369,7 +369,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, false);

[adamk, 2016/08/23 22:29:27]

Passing two bools is not ideal, see suggestion in runtime-internal.cc.

[Dan Ehrenberg, 2016/08/23 23:28:38]

Fixed, now separated out a different function.

+  return promise;
 }
 
 function PromiseCreateResolved(value) {
@@ -427,7 +431,7 @@ function PromiseThen(onResolve, onReject) {
   }
 
   var constructor = SpeciesConstructor(this, GlobalPromise);
-  var resultCapability = NewPromiseCapability(constructor);
+  var resultCapability = NewPromiseCapability(constructor, true);
   return PerformPromiseThen(this, onResolve, onReject, resultCapability);
 }