Skip to content

Commit

Permalink
report: handle on-fatalerror better
Browse files Browse the repository at this point in the history
--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: nodejs#31576
Refs: nodejs#29881
  • Loading branch information
HarshithaKP committed Mar 24, 2020
1 parent 96f06e6 commit 175c2e7
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 21 deletions.
2 changes: 1 addition & 1 deletion src/node_errors.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>());
}
Expand Down
8 changes: 4 additions & 4 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -236,6 +235,7 @@ class PerProcessOptions : public Options {
std::string use_largepages = "off";
bool trace_sigint = false;
std::vector<std::string> cmdline;
bool report_on_fatalerror = false;

inline PerIsolateOptions* get_per_isolate_options();
void CheckOptions(std::vector<std::string>* errors) override;
Expand Down
4 changes: 2 additions & 2 deletions src/node_report_module.cc
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,13 @@ static void SetSignal(const FunctionCallbackInfo<Value>& info) {
static void ShouldReportOnFatalError(const FunctionCallbackInfo<Value>& 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<Value>& 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<Value>& info) {
Expand Down
35 changes: 22 additions & 13 deletions test/report/test-report-fatal-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit 175c2e7

Please sign in to comment.