From 3691faafbc7f135ad264e6de75dc3450845f0501 Mon Sep 17 00:00:00 2001
From: Chengzhong Wu <chengzhong.wcz@alibaba-inc.com>
Date: Tue, 17 May 2022 03:40:40 +0000
Subject: [PATCH] worker: fix heap snapshot crash on exit

Worker.parent_port_ can be released before the exit event of Worker. The
parent_port_ is not owned by `node::worker::Worker`, thus the reference
to it is not always valid, and accessing it at exit crashes the process.

As the Worker.parent_port_ is only used in the memory info tracking, it
can be safely removed.

PR-URL: https://github.com/nodejs/node/pull/43123
Fixes: https://github.com/nodejs/node/issues/43122
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 src/node_worker.cc                             | 16 ++++++----------
 src/node_worker.h                              |  6 +-----
 test/parallel/test-worker-exit-heapsnapshot.js | 17 +++++++++++++++++
 test/pummel/test-heapdump-worker.js            |  8 --------
 4 files changed, 24 insertions(+), 23 deletions(-)
 create mode 100644 test/parallel/test-worker-exit-heapsnapshot.js

diff --git a/src/node_worker.cc b/src/node_worker.cc
index 02420cd6327047..86493374af13b4 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -61,18 +61,18 @@ Worker::Worker(Environment* env,
         thread_id_.id);
 
   // Set up everything that needs to be set up in the parent environment.
-  parent_port_ = MessagePort::New(env, env->context());
-  if (parent_port_ == nullptr) {
+  MessagePort* parent_port = MessagePort::New(env, env->context());
+  if (parent_port == nullptr) {
     // This can happen e.g. because execution is terminating.
     return;
   }
 
   child_port_data_ = std::make_unique<MessagePortData>(nullptr);
-  MessagePort::Entangle(parent_port_, child_port_data_.get());
+  MessagePort::Entangle(parent_port, child_port_data_.get());
 
-  object()->Set(env->context(),
-                env->message_port_string(),
-                parent_port_->object()).Check();
+  object()
+      ->Set(env->context(), env->message_port_string(), parent_port->object())
+      .Check();
 
   object()->Set(env->context(),
                 env->thread_id_string(),
@@ -724,10 +724,6 @@ void Worker::Exit(int code, const char* error_code, const char* error_message) {
   }
 }
 
-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.
diff --git a/src/node_worker.h b/src/node_worker.h
index cc3d24188ef735..119b75d137a43f 100644
--- a/src/node_worker.h
+++ b/src/node_worker.h
@@ -47,7 +47,7 @@ class Worker : public AsyncWrap {
   template <typename Fn>
   inline bool RequestInterrupt(Fn&& cb);
 
-  void MemoryInfo(MemoryTracker* tracker) const override;
+  SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(Worker)
   SET_SELF_SIZE(Worker)
   bool IsNotIndicativeOfMemoryLeakAtExit() const override;
@@ -107,10 +107,6 @@ class Worker : public AsyncWrap {
   std::unique_ptr<MessagePortData> child_port_data_;
   std::shared_ptr<KVStore> env_vars_;
 
-  // This is always kept alive because the JS object associated with the Worker
-  // instance refers to it via its [kPort] property.
-  MessagePort* parent_port_ = nullptr;
-
   // A raw flag that is used by creator and worker threads to
   // sync up on pre-mature termination of worker  - while in the
   // warmup phase.  Once the worker is fully warmed up, use the
diff --git a/test/parallel/test-worker-exit-heapsnapshot.js b/test/parallel/test-worker-exit-heapsnapshot.js
new file mode 100644
index 00000000000000..a7b7b26ecaefd7
--- /dev/null
+++ b/test/parallel/test-worker-exit-heapsnapshot.js
@@ -0,0 +1,17 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { getHeapSnapshot } = require('v8');
+const { isMainThread, Worker } = require('worker_threads');
+
+// Checks taking heap snapshot at the exit event listener of Worker doesn't
+// crash the process.
+// Regression for https://github.com/nodejs/node/issues/43122.
+if (isMainThread) {
+  const worker = new Worker(__filename);
+
+  worker.once('exit', common.mustCall((code) => {
+    assert.strictEqual(code, 0);
+    getHeapSnapshot().pipe(process.stdout);
+  }));
+}
diff --git a/test/pummel/test-heapdump-worker.js b/test/pummel/test-heapdump-worker.js
index 0e8322affb5d64..d494de50922155 100644
--- a/test/pummel/test-heapdump-worker.js
+++ b/test/pummel/test-heapdump-worker.js
@@ -6,14 +6,6 @@ const { Worker } = require('worker_threads');
 
 validateSnapshotNodes('Node / Worker', []);
 const worker = new Worker('setInterval(() => {}, 100);', { eval: true });
-validateSnapshotNodes('Node / Worker', [
-  {
-    children: [
-      { node_name: 'Node / MessagePort', edge_name: 'parent_port' },
-      { node_name: 'Worker', edge_name: 'wrapped' },
-    ]
-  },
-]);
 validateSnapshotNodes('Node / MessagePort', [
   {
     children: [