From 4cfbc7b82905a0b95d255af2954b38ad60863104 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Tue, 8 Oct 2019 04:18:47 +0530 Subject: [PATCH 1/2] src: adds report_on_fatalerror option to per_process moving the flag to per_process allows to read it's value on fatal error where env is null Fixes: https://github.com/nodejs/node/issues/29601 --- src/node_errors.cc | 2 +- src/node_options.cc | 9 +++++++++ src/node_options.h | 1 + 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/node_errors.cc b/src/node_errors.cc index 7e23fca28dfc0e..49e783d86243ca 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -456,7 +456,7 @@ void OnFatalError(const char* location, const char* message) { #ifdef NODE_REPORT 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 8c4ecc4ec44d66..4912ed9f810f5d 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -698,6 +698,15 @@ PerProcessOptionsParser::PerProcessOptionsParser( "print V8 command line options", &PerProcessOptions::print_v8_help); +#ifdef NODE_REPORT + +AddOption("--report-on-fatalerror", + "generate diagnostic report on fatal (internal) errors", + &PerProcessOptions::report_on_fatalerror, + kAllowedInEnvironment); + +#endif // NODE_REPORT + #ifdef NODE_HAVE_I18N_SUPPORT AddOption("--icu-data-dir", "set ICU data load path to dir (overrides NODE_ICU_DATA)" diff --git a/src/node_options.h b/src/node_options.h index 1e156a7edc56d1..ad3eb5475fd94c 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -240,6 +240,7 @@ class PerProcessOptions : public Options { #ifdef NODE_REPORT std::vector cmdline; + bool report_on_fatalerror = false; #endif // NODE_REPORT inline PerIsolateOptions* get_per_isolate_options(); From decb8483c20b714ff1822ab785a5721a490a1778 Mon Sep 17 00:00:00 2001 From: Shobhit Chittora Date: Tue, 8 Oct 2019 15:14:49 +0530 Subject: [PATCH 2/2] src: removes fatal_error check from per-isolate + adds tests --- src/node_options.cc | 23 +++++++--------- src/node_options.h | 1 - src/node_report_module.cc | 4 +-- test/report/test-report-fatal-error.js | 37 ++++++++++++++++---------- 4 files changed, 35 insertions(+), 30 deletions(-) diff --git a/src/node_options.cc b/src/node_options.cc index 4912ed9f810f5d..57b3c7fe9fc5d2 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -70,6 +70,15 @@ void PerProcessOptions::CheckOptions(std::vector* errors) { use_largepages != "silent") { errors->push_back("invalid value for --use-largepages"); } + +// #ifdef NODE_REPORT +// if (report_on_fatalerror) { +// errors->push_back( +// "--report-on-fatalerror option is valid only when " +// "--experimental-report is set"); +// } +// #endif // NODE_REPORT + per_isolate->CheckOptions(errors); } @@ -100,12 +109,6 @@ void PerIsolateOptions::CheckOptions(std::vector* errors) { "--experimental-report is set"); } - if (report_on_fatalerror) { - errors->push_back( - "--report-on-fatalerror option is valid only when " - "--experimental-report is set"); - } - if (report_on_signal) { errors->push_back("--report-on-signal option is valid only when " "--experimental-report is set"); @@ -622,10 +625,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)", @@ -699,12 +698,10 @@ PerProcessOptionsParser::PerProcessOptionsParser( &PerProcessOptions::print_v8_help); #ifdef NODE_REPORT - AddOption("--report-on-fatalerror", "generate diagnostic report on fatal (internal) errors", &PerProcessOptions::report_on_fatalerror, - kAllowedInEnvironment); - + kAllowedInEnvironment); #endif // NODE_REPORT #ifdef NODE_HAVE_I18N_SUPPORT diff --git a/src/node_options.h b/src/node_options.h index ad3eb5475fd94c..949f55a3ff5137 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -190,7 +190,6 @@ class PerIsolateOptions : public Options { #ifdef NODE_REPORT bool report_uncaught_exception = false; bool report_on_signal = false; - bool report_on_fatalerror = false; std::string report_signal; std::string report_filename; std::string report_directory; diff --git a/src/node_report_module.cc b/src/node_report_module.cc index f35d23a06e3232..bd8424ccc7c0d3 100644 --- a/src/node_report_module.cc +++ b/src/node_report_module.cc @@ -119,13 +119,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 8ecd058cf8a4de..b0390dec926337 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -21,18 +21,27 @@ 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 = ['--experimental-report', - '--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 = ['--experimental-report', + '--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); }