From d93774cfeced393fcaa02d9963e03e9e0abaee58 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Juan=20Jos=C3=A9=20Arboleda?= <soyjuanarbol@gmail.com>
Date: Sat, 5 Jul 2025 01:05:58 -0500
Subject: [PATCH] src,lib: support path and encoding in `v8.writeHeapSnapshot`
 output
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes: https://github.com/nodejs/node/issues/58857
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
---
 doc/api/v8.md                                 |  6 ++
 lib/v8.js                                     | 11 +++-
 src/heap_utils.cc                             | 61 ++++++++++++++-----
 src/node_internals.h                          | 12 ++--
 test/common/heap.js                           | 12 ++++
 .../test-write-heapsnapshot-options.js        | 24 +++++++-
 6 files changed, 99 insertions(+), 27 deletions(-)

diff --git a/doc/api/v8.md b/doc/api/v8.md
index db92ad062b7e63..ec6c23b625ae89 100644
--- a/doc/api/v8.md
+++ b/doc/api/v8.md
@@ -511,6 +511,9 @@ disk unless [`v8.stopCoverage()`][] is invoked before the process exits.
 <!-- YAML
 added: v11.13.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/58959
+    description: Support path and encoding options.
   - version: v19.1.0
     pr-url: https://github.com/nodejs/node/pull/44989
     description: Support options to configure the heap snapshot.
@@ -533,6 +536,9 @@ changes:
     **Default:** `false`.
   * `exposeNumericValues` {boolean} If true, expose numeric values in
     artificial fields. **Default:** `false`.
+  * `path` {string|Buffer|URL} If provided, specifies the file system location
+    where the snapshot will be written. **Default:** `undefined`
+  * `encoding` {string|null} **Default:** `'utf8'`
 * Returns: {string} The filename where the snapshot was saved.
 
 Generates a snapshot of the current V8 heap and writes it to a JSON
diff --git a/lib/v8.js b/lib/v8.js
index 54395945aa1787..213ebe70dd0c5c 100644
--- a/lib/v8.js
+++ b/lib/v8.js
@@ -78,7 +78,9 @@ const { getOptionValue } = require('internal/options');
  * @param {string} [filename]
  * @param {{
  *   exposeInternals?: boolean,
- *   exposeNumericValues?: boolean
+ *   exposeNumericValues?: boolean,
+ *   path?: string,
+ *   encoding?: string
  * }} [options]
  * @returns {string}
  */
@@ -86,8 +88,13 @@ function writeHeapSnapshot(filename, options) {
   if (filename !== undefined) {
     filename = getValidatedPath(filename);
   }
+
+  if (options?.path !== undefined) {
+    options.path = getValidatedPath(options.path);
+  }
+
   const optionArray = getHeapSnapshotOptions(options);
-  return triggerHeapSnapshot(filename, optionArray);
+  return triggerHeapSnapshot(filename, optionArray, options?.path, options?.encoding);
 }
 
 /**
diff --git a/src/heap_utils.cc b/src/heap_utils.cc
index 7b93698c7fe125..32666fc92c6d33 100644
--- a/src/heap_utils.cc
+++ b/src/heap_utils.cc
@@ -5,6 +5,7 @@
 #include "path.h"
 #include "permission/permission.h"
 #include "stream_base-inl.h"
+#include "string_bytes.h"
 #include "util-inl.h"
 
 // Copied from https://github.com/nodejs/node/blob/b07dc4d19fdbc15b4f76557dc45b3ce3a43ad0c3/src/util.cc#L36-L41.
@@ -448,30 +449,60 @@ void CreateHeapSnapshotStream(const FunctionCallbackInfo<Value>& args) {
 void TriggerHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = args.GetIsolate();
-  CHECK_EQ(args.Length(), 2);
+  CHECK_EQ(args.Length(), 4);
   Local<Value> filename_v = args[0];
   auto options = GetHeapSnapshotOptions(args[1]);
+  bool has_path = !args[2]->IsUndefined() && !args[2]->IsNull();
+  std::string final_filename;
+  BufferValue path_v(isolate, args[2]);
+  ToNamespacedPath(env, &path_v);
+
+  const enum encoding encoding = ParseEncoding(isolate, args[3], UTF8);
 
   if (filename_v->IsUndefined()) {
     DiagnosticFilename name(env, "Heap", "heapsnapshot");
-    THROW_IF_INSUFFICIENT_PERMISSIONS(
-        env,
-        permission::PermissionScope::kFileSystemWrite,
-        Environment::GetCwd(env->exec_path()));
-    if (WriteSnapshot(env, *name, options).IsNothing()) return;
-    if (String::NewFromUtf8(isolate, *name).ToLocal(&filename_v)) {
-      args.GetReturnValue().Set(filename_v);
+    if (!has_path) {
+      THROW_IF_INSUFFICIENT_PERMISSIONS(
+          env,
+          permission::PermissionScope::kFileSystemWrite,
+          Environment::GetCwd(env->exec_path()));
+      final_filename = *name;
+    } else {
+      THROW_IF_INSUFFICIENT_PERMISSIONS(
+          env,
+          permission::PermissionScope::kFileSystemWrite,
+          path_v.ToStringView());
+      final_filename = path_v.ToString() + "/" +
+                       *name;  // NOLINT(readability/pointer_notation)
+    }
+  } else {
+    BufferValue filename(isolate, filename_v);
+    CHECK_NOT_NULL(*filename);
+
+    if (has_path) {
+      final_filename = path_v.ToString() + "/" + filename.ToString();
+      THROW_IF_INSUFFICIENT_PERMISSIONS(
+          env, permission::PermissionScope::kFileSystemWrite, final_filename);
+    } else {
+      ToNamespacedPath(env, &filename);
+      THROW_IF_INSUFFICIENT_PERMISSIONS(
+          env,
+          permission::PermissionScope::kFileSystemWrite,
+          filename.ToStringView());
+      final_filename = filename.ToString();
     }
+  }
+
+  // If encoding fails, snapshot is not triggered.
+  Local<Value> ret;
+  if (!StringBytes::Encode(isolate, final_filename.c_str(), encoding)
+           .ToLocal(&ret)) {
     return;
   }
 
-  BufferValue path(isolate, filename_v);
-  CHECK_NOT_NULL(*path);
-  ToNamespacedPath(env, &path);
-  THROW_IF_INSUFFICIENT_PERMISSIONS(
-      env, permission::PermissionScope::kFileSystemWrite, path.ToStringView());
-  if (WriteSnapshot(env, *path, options).IsNothing()) return;
-  return args.GetReturnValue().Set(filename_v);
+  if (WriteSnapshot(env, final_filename.c_str(), options).IsNothing()) return;
+
+  args.GetReturnValue().Set(ret);
 }
 
 void Initialize(Local<Object> target,
diff --git a/src/node_internals.h b/src/node_internals.h
index 6ec17b35cae6c6..9a3036e31ab821 100644
--- a/src/node_internals.h
+++ b/src/node_internals.h
@@ -447,9 +447,6 @@ namespace heap {
 v8::Maybe<void> WriteSnapshot(Environment* env,
                               const char* filename,
                               v8::HeapProfiler::HeapSnapshotOptions options);
-}
-
-namespace heap {
 
 void DeleteHeapSnapshot(const v8::HeapSnapshot* snapshot);
 using HeapSnapshotPointer =
@@ -457,6 +454,10 @@ using HeapSnapshotPointer =
 
 BaseObjectPtr<AsyncWrap> CreateHeapSnapshotStream(
     Environment* env, HeapSnapshotPointer&& snapshot);
+
+v8::HeapProfiler::HeapSnapshotOptions GetHeapSnapshotOptions(
+    v8::Local<v8::Value> options);
+
 }  // namespace heap
 
 node_module napi_module_to_node_module(const napi_module* mod);
@@ -485,11 +486,6 @@ std::ostream& operator<<(std::ostream& output,
 
 bool linux_at_secure();
 
-namespace heap {
-v8::HeapProfiler::HeapSnapshotOptions GetHeapSnapshotOptions(
-    v8::Local<v8::Value> options);
-}  // namespace heap
-
 enum encoding ParseEncoding(v8::Isolate* isolate,
                             v8::Local<v8::Value> encoding_v,
                             v8::Local<v8::Value> encoding_id,
diff --git a/test/common/heap.js b/test/common/heap.js
index df7e2447b87c93..777c5c61732a0b 100644
--- a/test/common/heap.js
+++ b/test/common/heap.js
@@ -316,6 +316,18 @@ function getHeapSnapshotOptionTests() {
         ],
       }],
     },
+    // This is the same as the previous case, but with a path option.
+    // The path option will be mutated by the test
+    // test-write-heapsnapshot-options.js
+    // Refs: https://github.com/nodejs/node/issues/58857
+    {
+      options: { exposeNumericValues: true, path: 'TBD' },
+      expected: [{
+        children: [
+          { edge_name: 'numeric', node_name: 'smi number' },
+        ],
+      }],
+    },
   ];
   return {
     fixtures: fixtures.path('klass-with-fields.js'),
diff --git a/test/sequential/test-write-heapsnapshot-options.js b/test/sequential/test-write-heapsnapshot-options.js
index dca3cfebfc8df5..348c32480bb831 100644
--- a/test/sequential/test-write-heapsnapshot-options.js
+++ b/test/sequential/test-write-heapsnapshot-options.js
@@ -4,7 +4,9 @@
 
 require('../common');
 
+const assert = require('assert');
 const fs = require('fs');
+const tmpdir = require('../common/tmpdir');
 const { getHeapSnapshotOptionTests, recordState } = require('../common/heap');
 
 class ReadStream {
@@ -20,7 +22,12 @@ if (process.argv[2] === 'child') {
   const { writeHeapSnapshot } = require('v8');
   require(tests.fixtures);
   const { options, expected } = tests.cases[parseInt(process.argv[3])];
+  const { path } = options;
+  // If path is set, writeHeapSnapshot will write to the tmpdir.
+  if (path) options.path = tmpdir.path;
   const filename = writeHeapSnapshot(undefined, options);
+  // If path is set, the filename will be the provided path.
+  if (path) assert(filename.includes(tmpdir.path));
   const snapshot = recordState(new ReadStream(filename));
   console.log('Snapshot nodes', snapshot.snapshot.length);
   console.log('Searching for', expected[0].children);
@@ -30,8 +37,6 @@ if (process.argv[2] === 'child') {
 }
 
 const { spawnSync } = require('child_process');
-const assert = require('assert');
-const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
 // Start child processes to prevent the heap from growing too big.
@@ -48,3 +53,18 @@ for (let i = 0; i < tests.cases.length; ++i) {
   console.log('[STDOUT]', stdout);
   assert.strictEqual(child.status, 0);
 }
+
+{
+  // Don't need to spawn child process for this test, it will fail inmediately.
+  // Refs: https://github.com/nodejs/node/issues/58857
+  for (const pathName of [null, true, false, {}, [], () => {}, 1, 0, NaN]) {
+    assert.throws(() => {
+      require('v8').writeHeapSnapshot(undefined, {
+        path: pathName,
+      });
+    }, {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError',
+    });
+  }
+}