From 175c2e7a3f40c0e97edd46f9322878aaaa43710d Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 16 Mar 2020 02:13:10 -0400 Subject: [PATCH 1/4] 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 Fixes: #31576 Refs: #29881 --- 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 | 35 ++++++++++++++++---------- 5 files changed, 30 insertions(+), 21 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 fe6f0f24d31384..f134d4a735f835 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -590,10 +590,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)", @@ -667,6 +663,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 d372a83d4e4e4d..cffc06beb8c630 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -186,7 +186,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; @@ -236,6 +235,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..36069ea0456d45 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -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); + + 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); } From 7915e7eb4665df8965d4bbb3e0766b1abb8639af Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 16 Mar 2020 02:52:02 -0400 Subject: [PATCH 2/4] fixup: address review comment --- test/report/test-report-fatal-error.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index 36069ea0456d45..a6392a86f552fd 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -33,7 +33,8 @@ if (process.argv[2] === 'child') { 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']; From 3bf8c608974205ade1a89b03ac5a770ccc8868e3 Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 16 Mar 2020 03:02:17 -0400 Subject: [PATCH 3/4] fixup: fix linter error --- test/report/test-report-fatal-error.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index a6392a86f552fd..8d333b7c9dbaf1 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -33,7 +33,6 @@ if (process.argv[2] === 'child') { 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, From 7dbc5dea7cfd8a87f3019b94002923c4d78495ee Mon Sep 17 00:00:00 2001 From: Harshitha KP Date: Mon, 16 Mar 2020 09:21:16 -0400 Subject: [PATCH 4/4] fixup: fix linter error --- test/report/test-report-fatal-error.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index 8d333b7c9dbaf1..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') {