[v14.x] backport process.exit changes

doc/api/embedding.md

@@ -181,18 +181,19 @@ int RunNodeInstance(MultiIsolatePlatform* platform,
         more = uv_loop_alive(&loop);
         if (more) continue;
 
-        // node::EmitBeforeExit() is used to emit the 'beforeExit' event on
-        // the `process` object.
-        node::EmitBeforeExit(env.get());
+        // node::EmitProcessBeforeExit() is used to emit the 'beforeExit' event
+        // on the `process` object.
+        if (node::EmitProcessBeforeExit(env.get()).IsNothing())
+          break;
 
         // 'beforeExit' can also schedule new work that keeps the event loop
         // running.
         more = uv_loop_alive(&loop);
       } while (more == true);
     }
 
-    // node::EmitExit() returns the current exit code.
-    exit_code = node::EmitExit(env.get());
+    // node::EmitProcessExit() returns the current exit code.
+    exit_code = node::EmitProcessExit(env.get()).FromMaybe(1);
 
     // node::Stop() can be used to explicitly stop the event loop and keep
     // further JavaScript from running. It can be called from any thread,

src/api/hooks.cc

@@ -9,8 +9,11 @@ using v8::Context;
 using v8::HandleScope;
 using v8::Integer;
 using v8::Isolate;
+using v8::Just;
 using v8::Local;
+using v8::Maybe;
 using v8::NewStringType;
+using v8::Nothing;
 using v8::Object;
 using v8::String;
 using v8::Value;
@@ -30,6 +33,10 @@ void AtExit(Environment* env, void (*cb)(void* arg), void* arg) {
 }
 
 void EmitBeforeExit(Environment* env) {
+  USE(EmitProcessBeforeExit(env));
+}
+
+Maybe<bool> EmitProcessBeforeExit(Environment* env) {
   TraceEventScope trace_scope(TRACING_CATEGORY_NODE1(environment),
                               "BeforeExit", env);
   if (!env->destroy_async_id_list()->empty())
@@ -40,39 +47,49 @@ void EmitBeforeExit(Environment* env) {
 
   Local<Value> exit_code_v;
   if (!env->process_object()->Get(env->context(), env->exit_code_string())
-      .ToLocal(&exit_code_v)) return;
+      .ToLocal(&exit_code_v)) return Nothing<bool>();
 
   Local<Integer> exit_code;
-  if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) return;
+  if (!exit_code_v->ToInteger(env->context()).ToLocal(&exit_code)) {
+    return Nothing<bool>();
+  }
 
-  // TODO(addaleax): Provide variants of EmitExit() and EmitBeforeExit() that
-  // actually forward empty MaybeLocal<>s (and check env->can_call_into_js()).
-  USE(ProcessEmit(env, "beforeExit", exit_code));
+  return ProcessEmit(env, "beforeExit", exit_code).IsEmpty() ?
+      Nothing<bool>() : Just(true);
 }
 
 int EmitExit(Environment* env) {
+  return EmitProcessExit(env).FromMaybe(1);
+}
+
+Maybe<int> EmitProcessExit(Environment* env) {
   // process.emit('exit')
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
   Local<Object> process_object = env->process_object();
-  process_object
+
+  // TODO(addaleax): It might be nice to share process._exiting and
+  // process.exitCode via getter/setter pairs that pass data directly to the
+  // native side, so that we don't manually have to read and write JS properties
+  // here. These getters could use e.g. a typed array for performance.
+  if (process_object
       ->Set(env->context(),
             FIXED_ONE_BYTE_STRING(env->isolate(), "_exiting"),
-            True(env->isolate()))
-      .Check();
+            True(env->isolate())).IsNothing()) return Nothing<int>();
 
   Local<String> exit_code = env->exit_code_string();
-  int code = process_object->Get(env->context(), exit_code)
-                 .ToLocalChecked()
-                 ->Int32Value(env->context())
-                 .ToChecked();
-  ProcessEmit(env, "exit", Integer::New(env->isolate(), code));
-
-  // Reload exit code, it may be changed by `emit('exit')`
-  return process_object->Get(env->context(), exit_code)
-      .ToLocalChecked()
-      ->Int32Value(env->context())
-      .ToChecked();
+  Local<Value> code_v;
+  int code;
+  if (!process_object->Get(env->context(), exit_code).ToLocal(&code_v) ||
+      !code_v->Int32Value(env->context()).To(&code) ||
+      ProcessEmit(env, "exit", Integer::New(env->isolate(), code)).IsEmpty() ||
+      // Reload exit code, it may be changed by `emit('exit')`
+      !process_object->Get(env->context(), exit_code).ToLocal(&code_v) ||
+      !code_v->Int32Value(env->context()).To(&code)) {
+    return Nothing<int>();
+  }
+
+  return Just(code);
 }
 
 typedef void (*CleanupHook)(void* arg);

src/async_wrap.cc

@@ -98,6 +98,12 @@ struct AsyncWrapObject : public AsyncWrap {
     return tmpl;
   }
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // We can't really know what the underlying operation does. One of the
+    // signs that it's time to remove this class. :)
+    return true;
+  }
+
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(AsyncWrapObject)
   SET_SELF_SIZE(AsyncWrapObject)

src/base_object-inl.h

@@ -146,6 +146,13 @@ void BaseObject::ClearWeak() {
   persistent_handle_.ClearWeak();
 }
 
+bool BaseObject::IsWeakOrDetached() const {
+  if (persistent_handle_.IsWeak()) return true;
+
+  if (!has_pointer_data()) return false;
+  const PointerData* pd = const_cast<BaseObject*>(this)->pointer_data();
+  return pd->wants_weak_jsobj || pd->is_detached;
+}
 
 v8::Local<v8::FunctionTemplate>
 BaseObject::MakeLazilyInitializedJSTemplate(Environment* env) {

src/base_object.h

@@ -78,6 +78,11 @@ class BaseObject : public MemoryRetainer {
   // root and will not be touched by the garbage collector.
   inline void ClearWeak();
 
+  // Reports whether this BaseObject is using a weak reference or detached,
+  // i.e. whether is can be deleted by GC once no strong BaseObjectPtrs refer
+  // to it anymore.
+  inline bool IsWeakOrDetached() const;
+
   // Utility to create a FunctionTemplate with one internal field (used for
   // the `BaseObject*` pointer) and a constructor that initializes that field
   // to `nullptr`.
@@ -147,6 +152,10 @@ class BaseObject : public MemoryRetainer {
   virtual v8::Maybe<bool> FinalizeTransferRead(
       v8::Local<v8::Context> context, v8::ValueDeserializer* deserializer);
 
+  // Indicates whether this object is expected to use a strong reference during
+  // a clean process exit (due to an empty event loop).
+  virtual bool IsNotIndicativeOfMemoryLeakAtExit() const;
+
   virtual inline void OnGCCollect();
 
  private:

src/env.cc

@@ -1043,6 +1043,36 @@ void Environment::RemoveUnmanagedFd(int fd) {
   }
 }
 
+void Environment::VerifyNoStrongBaseObjects() {
+  // When a process exits cleanly, i.e. because the event loop ends up without
+  // things to wait for, the Node.js objects that are left on the heap should
+  // be:
+  //
+  //   1. weak, i.e. ready for garbage collection once no longer referenced, or
+  //   2. detached, i.e. scheduled for destruction once no longer referenced, or
+  //   3. an unrefed libuv handle, i.e. does not keep the event loop alive, or
+  //   4. an inactive libuv handle (essentially the same here)
+  //
+  // There are a few exceptions to this rule, but generally, if there are
+  // C++-backed Node.js objects on the heap that do not fall into the above
+  // categories, we may be looking at a potential memory leak. Most likely,
+  // the cause is a missing MakeWeak() call on the corresponding object.
+  //
+  // In order to avoid this kind of problem, we check the list of BaseObjects
+  // for these criteria. Currently, we only do so when explicitly instructed to
+  // or when in debug mode (where --verify-base-objects is always-on).
+
+  if (!options()->verify_base_objects) return;
+
+  ForEachBaseObject([](BaseObject* obj) {
+    if (obj->IsNotIndicativeOfMemoryLeakAtExit()) return;
+    fprintf(stderr, "Found bad BaseObject during clean exit: %s\n",
+            obj->MemoryInfoName().c_str());
+    fflush(stderr);
+    ABORT();
+  });
+}
+
 void Environment::BuildEmbedderGraph(Isolate* isolate,
                                      EmbedderGraph* graph,
                                      void* data) {
@@ -1135,4 +1165,8 @@ Local<FunctionTemplate> BaseObject::GetConstructorTemplate(Environment* env) {
   return tmpl;
 }
 
+bool BaseObject::IsNotIndicativeOfMemoryLeakAtExit() const {
+  return IsWeakOrDetached();
+}
+
 }  // namespace node

src/env.h

@@ -833,6 +833,7 @@ class Environment : public MemoryRetainer {
   void MemoryInfo(MemoryTracker* tracker) const override;
 
   void CreateProperties();
+  void VerifyNoStrongBaseObjects();
   // Should be called before InitializeInspector()
   void InitializeDiagnostics();
 #if HAVE_INSPECTOR

src/handle_wrap.cc

@@ -88,6 +88,13 @@ void HandleWrap::OnGCCollect() {
 }
 
 
+bool HandleWrap::IsNotIndicativeOfMemoryLeakAtExit() const {
+  return IsWeakOrDetached() ||
+         !HandleWrap::HasRef(this) ||
+         !uv_is_active(GetHandle());
+}
+
+
 void HandleWrap::MarkAsInitialized() {
   env()->handle_wrap_queue()->PushBack(this);
   state_ = kInitialized;

src/handle_wrap.h

@@ -85,6 +85,7 @@ class HandleWrap : public AsyncWrap {
              AsyncWrap::ProviderType provider);
   virtual void OnClose() {}
   void OnGCCollect() final;
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override;
 
   void MarkAsInitialized();
   void MarkAsUninitialized();

src/inspector_js_api.cc

@@ -155,6 +155,10 @@ class JSBindingsConnection : public AsyncWrap {
   SET_MEMORY_INFO_NAME(JSBindingsConnection)
   SET_SELF_SIZE(JSBindingsConnection)
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    return true;  // Binding connections emit events on their own.
+  }
+
  private:
   std::unique_ptr<InspectorSession> session_;
   Global<Function> callback_;

src/module_wrap.h

@@ -60,6 +60,12 @@ class ModuleWrap : public BaseObject {
   SET_MEMORY_INFO_NAME(ModuleWrap)
   SET_SELF_SIZE(ModuleWrap)
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // XXX: The garbage collection rules for ModuleWrap are *super* unclear.
+    // Do these objects ever get GC'd? Are we just okay with leaking them?
+    return true;
+  }
+
  private:
   ModuleWrap(Environment* env,
              v8::Local<v8::Object> object,

src/node.h

@@ -539,8 +539,19 @@ NODE_EXTERN void FreePlatform(MultiIsolatePlatform* platform);
 NODE_EXTERN v8::TracingController* GetTracingController();
 NODE_EXTERN void SetTracingController(v8::TracingController* controller);
 
-NODE_EXTERN void EmitBeforeExit(Environment* env);
-NODE_EXTERN int EmitExit(Environment* env);
+// Run `process.emit('beforeExit')` as it would usually happen when Node.js is
+// run in standalone mode.
+NODE_EXTERN v8::Maybe<bool> EmitProcessBeforeExit(Environment* env);
+NODE_DEPRECATED("Use Maybe version (EmitProcessBeforeExit) instead",
+    NODE_EXTERN void EmitBeforeExit(Environment* env));
+// Run `process.emit('exit')` as it would usually happen when Node.js is run
+// in standalone mode. The return value corresponds to the exit code.
+NODE_EXTERN v8::Maybe<int> EmitProcessExit(Environment* env);
+NODE_DEPRECATED("Use Maybe version (EmitProcessExit) instead",
+    NODE_EXTERN int EmitExit(Environment* env));
+
+// Runs hooks added through `AtExit()`. This is part of `FreeEnvironment()`,
+// so calling it manually is typically not necessary.
 NODE_EXTERN void RunAtExit(Environment* env);
 
 // This may return nullptr if the current v8::Context is not associated

src/node_contextify.h

@@ -174,6 +174,8 @@ class CompiledFnEntry final : public BaseObject {
                   v8::Local<v8::ScriptOrModule> script);
   ~CompiledFnEntry();
 
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override { return true; }
+
  private:
   uint32_t id_;
   v8::Global<v8::ScriptOrModule> script_;

src/node_http_parser.cc

@@ -880,6 +880,15 @@ class Parser : public AsyncWrap, public StreamListener {
     return HPE_PAUSED;
   }
 
+
+  bool IsNotIndicativeOfMemoryLeakAtExit() const override {
+    // HTTP parsers are able to emit events without any GC root referring
+    // to them, because they receive events directly from the underlying
+    // libuv resource.
+    return true;
+  }
+
+
   llhttp_t parser_;
   StringPtr fields_[kMaxHeaderFieldsCount];  // header fields
   StringPtr values_[kMaxHeaderFieldsCount];  // header values

src/node_main_instance.cc

@@ -135,7 +135,8 @@ int NodeMainInstance::Run() {
         if (more && !env->is_stopping()) continue;
 
         if (!uv_loop_alive(env->event_loop())) {
-          EmitBeforeExit(env.get());
+          if (EmitProcessBeforeExit(env.get()).IsNothing())
+            break;
         }
 
         // Emit `beforeExit` if the loop became alive either after emitting
@@ -147,7 +148,8 @@ int NodeMainInstance::Run() {
     }
 
     env->set_trace_sync_io(false);
-    exit_code = EmitExit(env.get());
+    if (!env->is_stopping()) env->VerifyNoStrongBaseObjects();
+    exit_code = EmitProcessExit(env.get()).FromMaybe(1);
   }
 
   ResetStdio();

src/node_options.cc

@@ -483,6 +483,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             "exit code 1 unless 'unhandledRejection' hook is set).",
             &EnvironmentOptions::unhandled_rejections,
             kAllowedInEnvironment);
+  AddOption("--verify-base-objects",
+            "", /* undocumented, only for debugging */
+            &EnvironmentOptions::verify_base_objects,
+            kAllowedInEnvironment);
 
   AddOption("--check",
             "syntax check script without executing",

src/node_options.h

@@ -151,6 +151,12 @@ class EnvironmentOptions : public Options {
   bool trace_warnings = false;
   std::string unhandled_rejections;
   std::string userland_loader;
+  bool verify_base_objects =
+#ifdef DEBUG
+      true;
+#else
+      false;
+#endif  // DEBUG
 
   bool syntax_check_only = false;
   bool has_eval_string = false;

src/node_worker.cc

@@ -352,7 +352,8 @@ void Worker::Run() {
           more = uv_loop_alive(&data.loop_);
           if (more && !is_stopped()) continue;
 
-          EmitBeforeExit(env_.get());
+          if (EmitProcessBeforeExit(env_.get()).IsNothing())
+            break;
 
           // Emit `beforeExit` if the loop became alive either after emitting
           // event, or after running some callbacks.
@@ -366,8 +367,10 @@ void Worker::Run() {
     {
       int exit_code;
       bool stopped = is_stopped();
-      if (!stopped)
-        exit_code = EmitExit(env_.get());
+      if (!stopped) {
+        env_->VerifyNoStrongBaseObjects();
+        exit_code = EmitProcessExit(env_.get()).FromMaybe(1);
+      }
       Mutex::ScopedLock lock(mutex_);
       if (exit_code_ == 0 && !stopped)
         exit_code_ = exit_code;
@@ -718,6 +721,12 @@ void Worker::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("parent_port", parent_port_);
 }
 
+bool Worker::IsNotIndicativeOfMemoryLeakAtExit() const {
+  // Worker objects always stay alive as long as the child thread, regardless
+  // of whether they are being referenced in the parent thread.
+  return true;
+}
+
 class WorkerHeapSnapshotTaker : public AsyncWrap {
  public:
   WorkerHeapSnapshotTaker(Environment* env, Local<Object> obj)