Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report: handle on-fatalerror better #32207

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
37 changes: 23 additions & 14 deletions test/report/test-report-fatal-error.js
Original file line number Diff line number Diff line change
@@ -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') {
Expand All @@ -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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment here. Something along the lines of // Verify that reports are not created on fatal error by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added suggested comment in test case. PTAL.

__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);
}