promises: refactor rejection handling

Remove the unnecessary microTasksTickObject for scheduling microtasks
and instead use TickInfo to keep track of whether promise rejections
exist that need to be emitted. Consequently allow the microtasks to
execute on average fewer times, in more predictable manner than
previously.

Simplify unhandled & handled rejection tracking to do more in C++ to
avoid needing to expose additional info in JS.

When new unhandledRejections are emitted within an unhandledRejection
handler, allow the event loop to proceed first instead. This means
that if the end-user code handles all promise rejections on nextTick,
rejections within unhandledRejection now won't spiral into an infinite
loop.

PR-URL: https://github.com/nodejs/node/pull/18207
Fixes: https://github.com/nodejs/node/issues/17913
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

3 files changed, 39 insertions, 24 deletions

diff --git a/src/env-inl.h b/src/env-inl.h
index 4fec606219..910bba69f7 100644
--- a/src/env-inl.h
+++ b/src/env-inl.h
@@ -264,6 +264,14 @@ inline bool Environment::TickInfo::has_scheduled() const {
   return fields_[kHasScheduled] == 1;
 }
 
+inline bool Environment::TickInfo::has_promise_rejections() const {
+  return fields_[kHasPromiseRejections] == 1;
+}
+
+inline void Environment::TickInfo::promise_rejections_toggle_on() {
+  fields_[kHasPromiseRejections] = 1;
+}
+
 inline void Environment::AssignToContext(v8::Local<v8::Context> context,
                                          const ContextInfo& info) {
   context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);
diff --git a/src/env.h b/src/env.h
index a9f2af0a94..813b2f5a62 100644
--- a/src/env.h
+++ b/src/env.h
@@ -293,7 +293,8 @@ class ModuleWrap;
   V(performance_entry_callback, v8::Function)                                 \
   V(performance_entry_template, v8::Function)                                 \
   V(process_object, v8::Object)                                               \
-  V(promise_reject_function, v8::Function)                                    \
+  V(promise_reject_handled_function, v8::Function)                            \
+  V(promise_reject_unhandled_function, v8::Function)                          \
   V(promise_wrap_template, v8::ObjectTemplate)                                \
   V(push_values_to_array_function, v8::Function)                              \
   V(randombytes_constructor_template, v8::ObjectTemplate)                     \
@@ -482,6 +483,9 @@ class Environment {
    public:
     inline AliasedBuffer<uint8_t, v8::Uint8Array>& fields();
     inline bool has_scheduled() const;
+    inline bool has_promise_rejections() const;
+
+    inline void promise_rejections_toggle_on();
 
    private:
     friend class Environment;  // So we can call the constructor.
@@ -489,6 +493,7 @@ class Environment {
 
     enum Fields {
       kHasScheduled,
+      kHasPromiseRejections,
       kFieldsCount
     };
 
diff --git a/src/node.cc b/src/node.cc
index d01276800a..0ceddaa64e 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -891,19 +891,33 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
 void PromiseRejectCallback(PromiseRejectMessage message) {
   Local<Promise> promise = message.GetPromise();
   Isolate* isolate = promise->GetIsolate();
-  Local<Value> value = message.GetValue();
-  Local<Integer> event = Integer::New(isolate, message.GetEvent());
+  v8::PromiseRejectEvent event = message.GetEvent();
 
   Environment* env = Environment::GetCurrent(isolate);
-  Local<Function> callback = env->promise_reject_function();
+  Local<Function> callback;
+  Local<Value> value;
 
-  if (value.IsEmpty())
+  if (event == v8::kPromiseRejectWithNoHandler) {
+    callback = env->promise_reject_unhandled_function();
+    value = message.GetValue();
+
+    if (value.IsEmpty())
+      value = Undefined(isolate);
+  } else if (event == v8::kPromiseHandlerAddedAfterReject) {
+    callback = env->promise_reject_handled_function();
     value = Undefined(isolate);
+  } else {
+    UNREACHABLE();
+  }
 
-  Local<Value> args[] = { event, promise, value };
-  Local<Object> process = env->process_object();
+  Local<Value> args[] = { promise, value };
+  MaybeLocal<Value> ret = callback->Call(env->context(),
+                                         Undefined(isolate),
+                                         arraysize(args),
+                                         args);
 
-  callback->Call(process, arraysize(args), args);
+  if (!ret.IsEmpty() && ret.ToLocalChecked()->IsTrue())
+    env->tick_info()->promise_rejections_toggle_on();
 }
 
 void SetupPromises(const FunctionCallbackInfo<Value>& args) {
@@ -911,9 +925,11 @@ void SetupPromises(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = env->isolate();
 
   CHECK(args[0]->IsFunction());
+  CHECK(args[1]->IsFunction());
 
   isolate->SetPromiseRejectCallback(PromiseRejectCallback);
-  env->set_promise_reject_function(args[0].As<Function>());
+  env->set_promise_reject_unhandled_function(args[0].As<Function>());
+  env->set_promise_reject_handled_function(args[1].As<Function>());
 
   env->process_object()->Delete(
       env->context(),
@@ -1022,7 +1038,7 @@ void InternalCallbackScope::Close() {
     CHECK_EQ(env_->trigger_async_id(), 0);
   }
 
-  if (!tick_info->has_scheduled()) {
+  if (!tick_info->has_scheduled() && !tick_info->has_promise_rejections()) {
     return;
   }
 
@@ -3038,20 +3054,6 @@ void SetupProcessObject(Environment* env,
       "napi",
       FIXED_ONE_BYTE_STRING(env->isolate(), node_napi_version));
 
-  // process._promiseRejectEvent
-  Local<Object> promiseRejectEvent = Object::New(env->isolate());
-  READONLY_DONT_ENUM_PROPERTY(process,
-                              "_promiseRejectEvent",
-                              promiseRejectEvent);
-  READONLY_PROPERTY(promiseRejectEvent,
-                    "unhandled",
-                    Integer::New(env->isolate(),
-                                 v8::kPromiseRejectWithNoHandler));
-  READONLY_PROPERTY(promiseRejectEvent,
-                    "handled",
-                    Integer::New(env->isolate(),
-                                 v8::kPromiseHandlerAddedAfterReject));
-
 #if HAVE_OPENSSL
   // Stupid code to slice out the version string.
   {  // NOLINT(whitespace/braces)