From c126d28c2ecfcc3c00e4bf2a65f85195e1782de9 Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 16 Mar 2020 02:13:10 -0400 Subject: [PATCH] report: handle on-fatalerror better --report-on-fatalerror was not honored properly, as there was no way to check the value which was stored in the Environment pointer which can be inaccessible under certain fatal error situations. Move the flag out of Environment pointer so that this is doable. Co-authored-by: Shobhit Chittora schittora@paypal.com PR-URL: https://github.com/nodejs/node/pull/32207 Fixes: https://github.com/nodejs/node/issues/31576 Refs: https://github.com/nodejs/node/pull/29881 Reviewed-By: Gireesh Punathil Reviewed-By: Sam Roberts --- src/node_errors.cc | 2 +- src/node_options.cc | 8 +++--- src/node_options.h | 2 +- src/node_report_module.cc | 4 +-- test/report/test-report-fatal-error.js | 37 ++++++++++++++++---------- 5 files changed, 31 insertions(+), 22 deletions(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index ae1da87ef6b0e0..590562cccffbc3 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -406,7 +406,7 @@ void OnFatalError(const char* location, const char* message) { Isolate* isolate = Isolate::GetCurrent(); Environment* env = Environment::GetCurrent(isolate); - if (env == nullptr || env->isolate_data()->options()->report_on_fatalerror) { + if (per_process::cli_options->report_on_fatalerror) { report::TriggerNodeReport( isolate, env, message, "FatalError", "", Local()); } diff --git a/src/node_options.cc b/src/node_options.cc index 3e78c65e5b9f1f..0b4f69899476ad 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -630,10 +630,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser( "generate diagnostic report upon receiving signals", &PerIsolateOptions::report_on_signal, kAllowedInEnvironment); - AddOption("--report-on-fatalerror", - "generate diagnostic report on fatal (internal) errors", - &PerIsolateOptions::report_on_fatalerror, - kAllowedInEnvironment); AddOption("--report-signal", "causes diagnostic report to be produced on provided signal," " unsupported in Windows. (default: SIGUSR2)", @@ -711,6 +707,10 @@ PerProcessOptionsParser::PerProcessOptionsParser( AddOption("--v8-options", "print V8 command line options", &PerProcessOptions::print_v8_help); + AddOption("--report-on-fatalerror", + "generate diagnostic report on fatal (internal) errors", + &PerProcessOptions::report_on_fatalerror, + kAllowedInEnvironment); #ifdef NODE_HAVE_I18N_SUPPORT AddOption("--icu-data-dir", diff --git a/src/node_options.h b/src/node_options.h index 46e11850e557ba..264098c94cc6df 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -187,7 +187,6 @@ class PerIsolateOptions : public Options { bool no_node_snapshot = false; bool report_uncaught_exception = false; bool report_on_signal = false; - bool report_on_fatalerror = false; bool report_compact = false; std::string report_signal = "SIGUSR2"; std::string report_filename; @@ -238,6 +237,7 @@ class PerProcessOptions : public Options { std::string use_largepages = "off"; bool trace_sigint = false; std::vector cmdline; + bool report_on_fatalerror = false; inline PerIsolateOptions* get_per_isolate_options(); void CheckOptions(std::vector* errors) override; diff --git a/src/node_report_module.cc b/src/node_report_module.cc index e06fe52514ef7f..769511ec468af2 100644 --- a/src/node_report_module.cc +++ b/src/node_report_module.cc @@ -131,13 +131,13 @@ static void SetSignal(const FunctionCallbackInfo& info) { static void ShouldReportOnFatalError(const FunctionCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); info.GetReturnValue().Set( - env->isolate_data()->options()->report_on_fatalerror); + node::per_process::cli_options->report_on_fatalerror); } static void SetReportOnFatalError(const FunctionCallbackInfo& info) { Environment* env = Environment::GetCurrent(info); CHECK(info[0]->IsBoolean()); - env->isolate_data()->options()->report_on_fatalerror = info[0]->IsTrue(); + node::per_process::cli_options->report_on_fatalerror = info[0]->IsTrue(); } static void ShouldReportOnSignal(const FunctionCallbackInfo& info) { diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index 99a2da71c203e1..3c93d9c77a755c 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -1,6 +1,6 @@ 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); // Testcase to produce report on fatal error (javascript heap OOM) if (process.argv[2] === 'child') { @@ -20,17 +20,26 @@ if (process.argv[2] === 'child') { const helper = require('../common/report.js'); const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); - const spawn = require('child_process').spawn; - const args = ['--report-on-fatalerror', - '--max-old-space-size=20', - __filename, - 'child']; - const child = spawn(process.execPath, args, { cwd: tmpdir.path }); - child.on('exit', common.mustCall((code) => { - assert.notStrictEqual(code, 0, 'Process exited unexpectedly'); - const reports = helper.findReports(child.pid, tmpdir.path); - assert.strictEqual(reports.length, 1); - const report = reports[0]; - helper.validate(report); - })); + const spawnSync = require('child_process').spawnSync; + let args = ['--report-on-fatalerror', + '--max-old-space-size=20', + __filename, + 'child']; + + let child = spawnSync(process.execPath, args, { cwd: tmpdir.path }); + + assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly'); + let 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']; + + child = spawnSync(process.execPath, args, { cwd: tmpdir.path }); + assert.notStrictEqual(child.status, 0, 'Process exited unexpectedly'); + reports = helper.findReports(child.pid, tmpdir.path); + assert.strictEqual(reports.length, 0); }