From c842ab36b6066b2e921e87d27c66a6f1322176b3 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Tue, 16 Aug 2022 21:04:48 +0800 Subject: [PATCH] report: skip report if uncaught exception is handled If the exception is handled by the userland process#uncaughtException handler, reports should not be generated repetitively as the process may continue to run. PR-URL: https://github.com/nodejs/node/pull/44208 Reviewed-By: Rafael Gonzaga Reviewed-By: Gireesh Punathil Reviewed-By: Stephen Belanger --- doc/api/cli.md | 9 +++-- lib/internal/process/execution.js | 21 ----------- src/node_errors.cc | 8 +++++ .../test-report-uncaught-exception-compat.js | 35 ++++++++++++++++--- .../test-report-uncaught-exception-handled.js | 23 ++++++++++++ ...test-report-uncaught-exception-override.js | 4 +-- ...st-report-uncaught-exception-primitives.js | 27 ++++++++------ .../test-report-uncaught-exception-symbols.js | 24 ++++++++----- test/report/test-report-uncaught-exception.js | 31 ++++++++++------ 9 files changed, 122 insertions(+), 60 deletions(-) create mode 100644 test/report/test-report-uncaught-exception-handled.js diff --git a/doc/api/cli.md b/doc/api/cli.md index 4eb4a669f1200f..3e7c0d41066f43 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1048,6 +1048,9 @@ Default signal is `SIGUSR2`. -Enables report to be generated on uncaught exceptions. Useful when inspecting -the JavaScript stack in conjunction with native stack and other runtime -environment data. +Enables report to be generated when the process exits due to an uncaught +exception. Useful when inspecting the JavaScript stack in conjunction with +native stack and other runtime environment data. ### `--secure-heap=n` diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 4a8e51f694f7e6..20b66d57ca51c9 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -139,27 +139,6 @@ function createOnGlobalUncaughtException() { // call that threw and was never cleared. So clear it now. clearDefaultTriggerAsyncId(); - // If diagnostic reporting is enabled, call into its handler to see - // whether it is interested in handling the situation. - // Ignore if the error is scoped inside a domain. - // use == in the checks as we want to allow for null and undefined - if (er == null || er.domain == null) { - try { - const report = internalBinding('report'); - if (report != null && report.shouldReportOnUncaughtException()) { - report.writeReport( - typeof er?.message === 'string' ? - er.message : - 'Exception', - 'Exception', - null, - er ?? {}); - } - } catch { - // Ignore the exception. Diagnostic reporting is unavailable. - } - } - const type = fromPromise ? 'unhandledRejection' : 'uncaughtException'; process.emit('uncaughtExceptionMonitor', er, type); if (exceptionHandlerState.captureFn !== null) { diff --git a/src/node_errors.cc b/src/node_errors.cc index 51eb20e6c40418..633e5a7244c77f 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -392,6 +392,7 @@ static void ReportFatalException(Environment* env, } node::Utf8Value trace(env->isolate(), stack_trace); + std::string report_message = "Exception"; // range errors have a trace member set to undefined if (trace.length() > 0 && !stack_trace->IsUndefined()) { @@ -424,6 +425,8 @@ static void ReportFatalException(Environment* env, } else { node::Utf8Value name_string(env->isolate(), name.ToLocalChecked()); node::Utf8Value message_string(env->isolate(), message.ToLocalChecked()); + // Update the report message if it is an object has message property. + report_message = message_string.ToString(); if (arrow.IsEmpty() || !arrow->IsString() || decorated) { FPrintF(stderr, "%s: %s\n", name_string, message_string); @@ -445,6 +448,11 @@ static void ReportFatalException(Environment* env, } } + if (env->isolate_data()->options()->report_uncaught_exception) { + report::TriggerNodeReport( + isolate, env, report_message.c_str(), "Exception", "", error); + } + if (env->options()->trace_uncaught) { Local trace = message->GetStackTrace(); if (!trace.IsEmpty()) { diff --git a/test/report/test-report-uncaught-exception-compat.js b/test/report/test-report-uncaught-exception-compat.js index 9fe690595e89a0..91989f6ecda994 100644 --- a/test/report/test-report-uncaught-exception-compat.js +++ b/test/report/test-report-uncaught-exception-compat.js @@ -1,5 +1,32 @@ -// Flags: --experimental-report --report-uncaught-exception --report-compact 'use strict'; -// Test producing a compact report on uncaught exception. -require('../common'); -require('./test-report-uncaught-exception.js'); +// Test producing a report on uncaught exception. +const common = require('../common'); +const assert = require('assert'); +const childProcess = require('child_process'); +const helper = require('../common/report'); +const tmpdir = require('../common/tmpdir'); + +if (process.argv[2] === 'child') { + throw new Error('test error'); +} + +tmpdir.refresh(); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + '--report-compact', + __filename, + 'child', +], { + cwd: tmpdir.path +}); +child.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 1); + const reports = helper.findReports(child.pid, tmpdir.path); + assert.strictEqual(reports.length, 1); + + helper.validate(reports[0], [ + ['header.event', 'Exception'], + ['header.trigger', 'Exception'], + ['javascriptStack.message', 'Error: test error'], + ]); +})); diff --git a/test/report/test-report-uncaught-exception-handled.js b/test/report/test-report-uncaught-exception-handled.js new file mode 100644 index 00000000000000..fffe5ef333f9c3 --- /dev/null +++ b/test/report/test-report-uncaught-exception-handled.js @@ -0,0 +1,23 @@ +// Flags: --report-uncaught-exception +'use strict'; +// Test report is suppressed on uncaught exception hook. +const common = require('../common'); +const assert = require('assert'); +const helper = require('../common/report'); +const tmpdir = require('../common/tmpdir'); +const error = new Error('test error'); + +tmpdir.refresh(); +process.report.directory = tmpdir.path; + +// Make sure the uncaughtException listener is called. +process.on('uncaughtException', common.mustCall()); + +process.on('exit', (code) => { + assert.strictEqual(code, 0); + // Make sure no reports are generated. + const reports = helper.findReports(process.pid, tmpdir.path); + assert.strictEqual(reports.length, 0); +}); + +throw error; diff --git a/test/report/test-report-uncaught-exception-override.js b/test/report/test-report-uncaught-exception-override.js index df4f8a1958114b..007e8a234cfcb4 100644 --- a/test/report/test-report-uncaught-exception-override.js +++ b/test/report/test-report-uncaught-exception-override.js @@ -12,9 +12,7 @@ process.report.directory = tmpdir.path; // First, install an uncaught exception hook. process.setUncaughtExceptionCaptureCallback(common.mustCall()); - -// Make sure this is ignored due to the above override. -process.on('uncaughtException', common.mustNotCall()); +// Do not install process uncaughtException handler. process.on('exit', (code) => { assert.strictEqual(code, 0); diff --git a/test/report/test-report-uncaught-exception-primitives.js b/test/report/test-report-uncaught-exception-primitives.js index 8de67eeb6a2747..8e78ad3317d6f2 100644 --- a/test/report/test-report-uncaught-exception-primitives.js +++ b/test/report/test-report-uncaught-exception-primitives.js @@ -1,25 +1,32 @@ -// Flags: --report-uncaught-exception 'use strict'; // Test producing a report on uncaught exception. const common = require('../common'); const assert = require('assert'); +const childProcess = require('child_process'); const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); -const exception = 1; +if (process.argv[2] === 'child') { + // eslint-disable-next-line no-throw-literal + throw 1; +} tmpdir.refresh(); -process.report.directory = tmpdir.path; - -process.on('uncaughtException', common.mustCall((err) => { - assert.strictEqual(err, exception); - const reports = helper.findReports(process.pid, tmpdir.path); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + __filename, + 'child', +], { + cwd: tmpdir.path, +}); +child.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 1); + const reports = helper.findReports(child.pid, tmpdir.path); assert.strictEqual(reports.length, 1); helper.validate(reports[0], [ ['header.event', 'Exception'], - ['javascriptStack.message', `${exception}`], + ['header.trigger', 'Exception'], + ['javascriptStack.message', '1'], ]); })); - -throw exception; diff --git a/test/report/test-report-uncaught-exception-symbols.js b/test/report/test-report-uncaught-exception-symbols.js index b1656172851f66..09dd46536095da 100644 --- a/test/report/test-report-uncaught-exception-symbols.js +++ b/test/report/test-report-uncaught-exception-symbols.js @@ -1,25 +1,31 @@ -// Flags: --report-uncaught-exception 'use strict'; // Test producing a report on uncaught exception. const common = require('../common'); const assert = require('assert'); +const childProcess = require('child_process'); const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); -const exception = Symbol('foobar'); +if (process.argv[2] === 'child') { + throw Symbol('foobar'); +} tmpdir.refresh(); -process.report.directory = tmpdir.path; - -process.on('uncaughtException', common.mustCall((err) => { - assert.strictEqual(err, exception); - const reports = helper.findReports(process.pid, tmpdir.path); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + __filename, + 'child', +], { + cwd: tmpdir.path, +}); +child.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 1); + const reports = helper.findReports(child.pid, tmpdir.path); assert.strictEqual(reports.length, 1); helper.validate(reports[0], [ ['header.event', 'Exception'], + ['header.trigger', 'Exception'], ['javascriptStack.message', 'Symbol(foobar)'], ]); })); - -throw exception; diff --git a/test/report/test-report-uncaught-exception.js b/test/report/test-report-uncaught-exception.js index 10dcccb090019c..5809104165be9e 100644 --- a/test/report/test-report-uncaught-exception.js +++ b/test/report/test-report-uncaught-exception.js @@ -1,20 +1,31 @@ -// Flags: --report-uncaught-exception 'use strict'; // Test producing a report on uncaught exception. const common = require('../common'); const assert = require('assert'); +const childProcess = require('child_process'); const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); -const error = new Error('test error'); -tmpdir.refresh(); -process.report.directory = tmpdir.path; +if (process.argv[2] === 'child') { + throw new Error('test error'); +} -process.on('uncaughtException', common.mustCall((err) => { - assert.strictEqual(err, error); - const reports = helper.findReports(process.pid, tmpdir.path); +tmpdir.refresh(); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + __filename, + 'child', +], { + cwd: tmpdir.path, +}); +child.on('exit', common.mustCall((code) => { + assert.strictEqual(code, 1); + const reports = helper.findReports(child.pid, tmpdir.path); assert.strictEqual(reports.length, 1); - helper.validate(reports[0]); -})); -throw error; + helper.validate(reports[0], [ + ['header.event', 'Exception'], + ['header.trigger', 'Exception'], + ['javascriptStack.message', 'Error: test error'], + ]); +}));