src: handle report options on fatalerror

Follow on to nodejs#32207, 3 other options are also not respected under situations that the isolate is not available. PR-URL: nodejs#32497 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
targos · Apr 25, 2020 · c6da2f4 · c6da2f4
1 parent 4f9ff92
commit c6da2f4
Show file tree

Hide file tree

Showing 5 changed files with 154 additions and 57 deletions.
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -591,10 +591,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
             "generate diagnostic report on uncaught exceptions",
             &PerIsolateOptions::report_uncaught_exception,
             kAllowedInEnvironment);
-  AddOption("--report-compact",
-            "output compact single-line JSON",
-            &PerIsolateOptions::report_compact,
-            kAllowedInEnvironment);
   AddOption("--report-on-signal",
             "generate diagnostic report upon receiving signals",
             &PerIsolateOptions::report_on_signal,
@@ -605,16 +601,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser(
             &PerIsolateOptions::report_signal,
             kAllowedInEnvironment);
   Implies("--report-signal", "--report-on-signal");
-  AddOption("--report-filename",
-            "define custom report file name."
-            " (default: YYYYMMDD.HHMMSS.PID.SEQUENCE#.txt)",
-            &PerIsolateOptions::report_filename,
-            kAllowedInEnvironment);
-  AddOption("--report-directory",
-            "define custom report pathname."
-            " (default: current working directory of Node.js process)",
-            &PerIsolateOptions::report_directory,
-            kAllowedInEnvironment);
 
   Insert(eop, &PerIsolateOptions::get_per_env_options);
 }
@@ -676,6 +662,20 @@ PerProcessOptionsParser::PerProcessOptionsParser(
   AddOption("--v8-options",
             "print V8 command line options",
             &PerProcessOptions::print_v8_help);
+  AddOption("--report-compact",
+            "output compact single-line JSON",
+            &PerProcessOptions::report_compact,
+            kAllowedInEnvironment);
+  AddOption("--report-directory",
+            "define custom report pathname."
+            " (default: current working directory of Node.js process)",
+            &PerProcessOptions::report_directory,
+            kAllowedInEnvironment);
+  AddOption("--report-filename",
+            "define custom report file name."
+            " (default: YYYYMMDD.HHMMSS.PID.SEQUENCE#.txt)",
+            &PerProcessOptions::report_filename,
+            kAllowedInEnvironment);
   AddOption("--report-on-fatalerror",
               "generate diagnostic report on fatal (internal) errors",
               &PerProcessOptions::report_on_fatalerror,

diff --git a/src/node_options.h b/src/node_options.h
@@ -187,10 +187,7 @@ class PerIsolateOptions : public Options {
   bool no_node_snapshot = false;
   bool report_uncaught_exception = false;
   bool report_on_signal = false;
-  bool report_compact = false;
   std::string report_signal = "SIGUSR2";
-  std::string report_filename;
-  std::string report_directory;
   inline EnvironmentOptions* get_per_env_options();
   void CheckOptions(std::vector<std::string>* errors) override;
 };
@@ -238,6 +235,9 @@ class PerProcessOptions : public Options {
   bool trace_sigint = false;
   std::vector<std::string> cmdline;
   bool report_on_fatalerror = false;
+  bool report_compact = false;
+  std::string report_directory;
+  std::string report_filename;
 
   inline PerIsolateOptions* get_per_isolate_options();
   void CheckOptions(std::vector<std::string>* errors) override;

diff --git a/src/node_report.cc b/src/node_report.cc
@@ -32,7 +32,6 @@ using node::DiagnosticFilename;
 using node::Environment;
 using node::Mutex;
 using node::NativeSymbolDebuggingContext;
-using node::PerIsolateOptions;
 using node::TIME_TYPE;
 using node::worker::Worker;
 using v8::HeapSpaceStatistics;
@@ -45,6 +44,8 @@ using v8::String;
 using v8::V8;
 using v8::Value;
 
+namespace per_process = node::per_process;
+
 // Internal/static function declarations
 static void WriteNodeReport(Isolate* isolate,
                             Environment* env,
@@ -70,29 +71,32 @@ static void PrintCpuInfo(JSONWriter* writer);
 static void PrintNetworkInterfaceInfo(JSONWriter* writer);
 
 // External function to trigger a report, writing to file.
-// The 'name' parameter is in/out: an input filename is used
-// if supplied, and the actual filename is returned.
 std::string TriggerNodeReport(Isolate* isolate,
                               Environment* env,
                               const char* message,
                               const char* trigger,
                               const std::string& name,
                               Local<String> stackstr) {
   std::string filename;
-  std::shared_ptr<PerIsolateOptions> options;
-  if (env != nullptr) options = env->isolate_data()->options();
 
   // Determine the required report filename. In order of priority:
   //   1) supplied on API 2) configured on startup 3) default generated
   if (!name.empty()) {
     // Filename was specified as API parameter.
     filename = name;
-  } else if (env != nullptr && options->report_filename.length() > 0) {
-    // File name was supplied via start-up option.
-    filename = options->report_filename;
   } else {
-    filename = *DiagnosticFilename(env != nullptr ? env->thread_id() : 0,
-                                   "report", "json");
+    std::string report_filename;
+    {
+      Mutex::ScopedLock lock(per_process::cli_options_mutex);
+      report_filename = per_process::cli_options->report_filename;
+    }
+    if (report_filename.length() > 0) {
+      // File name was supplied via start-up option.
+      filename = report_filename;
+    } else {
+      filename = *DiagnosticFilename(env != nullptr ? env->thread_id() : 0,
+          "report", "json");
+    }
   }
 
   // Open the report file stream for writing. Supports stdout/err,
@@ -104,9 +108,14 @@ std::string TriggerNodeReport(Isolate* isolate,
   } else if (filename == "stderr") {
     outstream = &std::cerr;
   } else {
+    std::string report_directory;
+    {
+      Mutex::ScopedLock lock(per_process::cli_options_mutex);
+      report_directory = per_process::cli_options->report_directory;
+    }
     // Regular file. Append filename to directory path if one was specified
-    if (env != nullptr && options->report_directory.length() > 0) {
-      std::string pathname = options->report_directory;
+    if (report_directory.length() > 0) {
+      std::string pathname = report_directory;
       pathname += node::kPathSeparator;
       pathname += filename;
       outfile.open(pathname, std::ios::out | std::ios::binary);
@@ -117,8 +126,8 @@ std::string TriggerNodeReport(Isolate* isolate,
     if (!outfile.is_open()) {
       std::cerr << "\nFailed to open Node.js report file: " << filename;
 
-      if (env != nullptr && options->report_directory.length() > 0)
-        std::cerr << " directory: " << options->report_directory;
+      if (report_directory.length() > 0)
+        std::cerr << " directory: " << report_directory;
 
       std::cerr << " (errno: " << errno << ")" << std::endl;
       return "";
@@ -127,7 +136,11 @@ std::string TriggerNodeReport(Isolate* isolate,
     std::cerr << "\nWriting Node.js report to file: " << filename;
   }
 
-  bool compact = env != nullptr ? options->report_compact : true;
+  bool compact;
+  {
+    Mutex::ScopedLock lock(per_process::cli_options_mutex);
+    compact = per_process::cli_options->report_compact;
+  }
   WriteNodeReport(isolate, env, message, trigger, filename, *outstream,
                   stackstr, compact);
 
@@ -136,7 +149,10 @@ std::string TriggerNodeReport(Isolate* isolate,
     outfile.close();
   }
 
-  std::cerr << "\nNode.js report completed" << std::endl;
+  // Do not mix JSON and free-form text on stderr.
+  if (filename != "stderr") {
+    std::cerr << "\nNode.js report completed" << std::endl;
+  }
   return filename;
 }
 

diff --git a/src/node_report_module.cc b/src/node_report_module.cc
@@ -69,47 +69,52 @@ void GetReport(const FunctionCallbackInfo<Value>& info) {
 }
 
 static void GetCompact(const FunctionCallbackInfo<Value>& info) {
-  Environment* env = Environment::GetCurrent(info);
-  info.GetReturnValue().Set(env->isolate_data()->options()->report_compact);
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
+  info.GetReturnValue().Set(node::per_process::cli_options->report_compact);
 }
 
 static void SetCompact(const FunctionCallbackInfo<Value>& info) {
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
   Isolate* isolate = env->isolate();
   bool compact = info[0]->ToBoolean(isolate)->Value();
-  env->isolate_data()->options()->report_compact = compact;
+  node::per_process::cli_options->report_compact = compact;
 }
 
 static void GetDirectory(const FunctionCallbackInfo<Value>& info) {
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
-  std::string directory = env->isolate_data()->options()->report_directory;
+  std::string directory = node::per_process::cli_options->report_directory;
   auto result = String::NewFromUtf8(env->isolate(),
                                     directory.c_str(),
                                     v8::NewStringType::kNormal);
   info.GetReturnValue().Set(result.ToLocalChecked());
 }
 
 static void SetDirectory(const FunctionCallbackInfo<Value>& info) {
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
   CHECK(info[0]->IsString());
   Utf8Value dir(env->isolate(), info[0].As<String>());
-  env->isolate_data()->options()->report_directory = *dir;
+  node::per_process::cli_options->report_directory = *dir;
 }
 
 static void GetFilename(const FunctionCallbackInfo<Value>& info) {
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
-  std::string filename = env->isolate_data()->options()->report_filename;
+  std::string filename = node::per_process::cli_options->report_filename;
   auto result = String::NewFromUtf8(env->isolate(),
                                     filename.c_str(),
                                     v8::NewStringType::kNormal);
   info.GetReturnValue().Set(result.ToLocalChecked());
 }
 
 static void SetFilename(const FunctionCallbackInfo<Value>& info) {
+  node::Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
   Environment* env = Environment::GetCurrent(info);
   CHECK(info[0]->IsString());
   Utf8Value name(env->isolate(), info[0].As<String>());
-  env->isolate_data()->options()->report_filename = *name;
+  node::per_process::cli_options->report_filename = *name;
 }
 
 static void GetSignal(const FunctionCallbackInfo<Value>& info) {

diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js
@@ -1,8 +1,14 @@
 'use strict';
 
+// Testcases for situations involving fatal errors, like Javascript heap OOM
+
 require('../common');
 const assert = require('assert');
-// Testcase to produce report on fatal error (javascript heap OOM)
+const fs = require('fs');
+const helper = require('../common/report.js');
+const spawnSync = require('child_process').spawnSync;
+const tmpdir = require('../common/tmpdir');
+
 if (process.argv[2] === 'child') {
 
   const list = [];
@@ -16,30 +22,100 @@ if (process.argv[2] === 'child') {
     this.id = 128;
     this.account = 98454324;
   }
-} else {
-  const helper = require('../common/report.js');
-  const tmpdir = require('../common/tmpdir');
+}
+
+// Common args that will cause an out-of-memory error for child process.
+const ARGS = [
+  '--max-old-space-size=20',
+  __filename,
+  'child'
+];
+
+{
+  // Verify that --report-on-fatalerror is respected when set.
   tmpdir.refresh();
-  const spawnSync = require('child_process').spawnSync;
-  let args = ['--report-on-fatalerror',
-              '--max-old-space-size=20',
-              __filename,
-              'child'];
+  const args = ['--report-on-fatalerror', ...ARGS];
+  const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
+  assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
 
-  let child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
+  const reports = helper.findReports(child.pid, tmpdir.path);
+  assert.strictEqual(reports.length, 1);
 
+  const report = reports[0];
+  helper.validate(report);
+
+  // Errors occur in a context where env is not available, so thread ID is
+  // unknown. Assert this, to verify that the underlying env-less situation is
+  // actually reached.
+  assert.strictEqual(require(report).header.threadId, null);
+}
+
+{
+  // Verify that --report-on-fatalerror is respected when not set.
+  const args = ARGS;
+  const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
+  assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
+  const reports = helper.findReports(child.pid, tmpdir.path);
+  assert.strictEqual(reports.length, 0);
+}
+
+{
+  // Verify that --report-directory is respected when set.
+  // Verify that --report-compact is respected when not set.
+  tmpdir.refresh();
+  const dir = '--report-directory=' + tmpdir.path;
+  const args = ['--report-on-fatalerror', dir, ...ARGS];
+  const child = spawnSync(process.execPath, args, { });
   assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
-  let reports = helper.findReports(child.pid, tmpdir.path);
+
+  const reports = helper.findReports(child.pid, tmpdir.path);
+  assert.strictEqual(reports.length, 1);
+
+  const report = reports[0];
+  helper.validate(report);
+  assert.strictEqual(require(report).header.threadId, null);
+  const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
+  assert(lines > 10);
+}
+
+{
+  // Verify that --report-compact is respected when set.
+  tmpdir.refresh();
+  const args = ['--report-on-fatalerror', '--report-compact', ...ARGS];
+  const child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
+  assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
+
+  const reports = helper.findReports(child.pid, tmpdir.path);
   assert.strictEqual(reports.length, 1);
+
   const report = reports[0];
   helper.validate(report);
-  // Verify that reports are not created on fatal error by default.
-  args = ['--max-old-space-size=20',
-          __filename,
-          'child'];
+  assert.strictEqual(require(report).header.threadId, null);
+  // Subtract 1 because "xx\n".split("\n") => [ 'xx', '' ].
+  const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
+  assert.strictEqual(lines, 1);
+}
 
-  child = spawnSync(process.execPath, args, { cwd: tmpdir.path });
+{
+  // Verify that --report-compact is respected when set.
+  // Verify that --report-filename is respected when set.
+  tmpdir.refresh();
+  const args = [
+    '--report-on-fatalerror',
+    '--report-compact',
+    '--report-filename=stderr',
+    ...ARGS
+  ];
+  const child = spawnSync(process.execPath, args, { encoding: 'utf8' });
   assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly');
-  reports = helper.findReports(child.pid, tmpdir.path);
+
+  const reports = helper.findReports(child.pid, tmpdir.path);
   assert.strictEqual(reports.length, 0);
+
+  const lines = child.stderr.split('\n');
+  // Skip over unavoidable free-form output from V8.
+  const report = lines[1];
+  const json = JSON.parse(report);
+
+  assert.strictEqual(json.header.threadId, null);
 }