From 360e9f76f0d69f68a817bd1b438f7c68b0383def Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Thu, 11 Aug 2022 03:24:01 +0000 Subject: [PATCH 1/4] 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. --- lib/internal/process/execution.js | 21 ----------- src/node_errors.cc | 8 +++++ .../test-report-uncaught-exception-compat.js | 36 ++++++++++++++++--- .../test-report-uncaught-exception-handled.js | 23 ++++++++++++ ...test-report-uncaught-exception-override.js | 4 +-- ...st-report-uncaught-exception-primitives.js | 28 +++++++++------ .../test-report-uncaught-exception-symbols.js | 25 ++++++++----- test/report/test-report-uncaught-exception.js | 32 +++++++++++------ 8 files changed, 120 insertions(+), 57 deletions(-) create mode 100644 test/report/test-report-uncaught-exception-handled.js diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 72e54030b26184..889f94e434bf74 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -151,27 +151,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..c481fe5a163d5d 100644 --- a/test/report/test-report-uncaught-exception-compat.js +++ b/test/report/test-report-uncaught-exception-compat.js @@ -1,5 +1,33 @@ -// 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') { + process.report.directory = process.argv[3]; + + throw new Error('test error'); +} + +tmpdir.refresh(); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + '--report-compact', + __filename, + 'child', + 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..f46060c1e2e5f5 100644 --- a/test/report/test-report-uncaught-exception-primitives.js +++ b/test/report/test-report-uncaught-exception-primitives.js @@ -1,25 +1,33 @@ -// 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') { + process.report.directory = process.argv[3]; -tmpdir.refresh(); -process.report.directory = tmpdir.path; + // eslint-disable-next-line no-throw-literal + throw 1; +} -process.on('uncaughtException', common.mustCall((err) => { - assert.strictEqual(err, exception); - const reports = helper.findReports(process.pid, tmpdir.path); +tmpdir.refresh(); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + __filename, + 'child', + 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..837e3f074cccb5 100644 --- a/test/report/test-report-uncaught-exception-symbols.js +++ b/test/report/test-report-uncaught-exception-symbols.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 = Symbol('foobar'); +if (process.argv[2] === 'child') { + process.report.directory = process.argv[3]; -tmpdir.refresh(); -process.report.directory = tmpdir.path; + throw Symbol('foobar'); +} -process.on('uncaughtException', common.mustCall((err) => { - assert.strictEqual(err, exception); - const reports = helper.findReports(process.pid, tmpdir.path); +tmpdir.refresh(); +const child = childProcess.spawn(process.execPath, [ + '--report-uncaught-exception', + __filename, + 'child', + 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..7d760bee58487d 100644 --- a/test/report/test-report-uncaught-exception.js +++ b/test/report/test-report-uncaught-exception.js @@ -1,20 +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 error = new Error('test error'); -tmpdir.refresh(); -process.report.directory = tmpdir.path; +if (process.argv[2] === 'child') { + process.report.directory = process.argv[3]; + + 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', + 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'], + ]); +})); From 6ec4fb3b9091eb1607ee429ca7dae2fc76a57359 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 12 Aug 2022 09:41:08 +0000 Subject: [PATCH 2/4] fixup! --- test/report/test-report-uncaught-exception-compat.js | 7 +++---- test/report/test-report-uncaught-exception-primitives.js | 7 +++---- test/report/test-report-uncaught-exception-symbols.js | 7 +++---- test/report/test-report-uncaught-exception.js | 7 +++---- 4 files changed, 12 insertions(+), 16 deletions(-) diff --git a/test/report/test-report-uncaught-exception-compat.js b/test/report/test-report-uncaught-exception-compat.js index c481fe5a163d5d..91989f6ecda994 100644 --- a/test/report/test-report-uncaught-exception-compat.js +++ b/test/report/test-report-uncaught-exception-compat.js @@ -7,8 +7,6 @@ const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); if (process.argv[2] === 'child') { - process.report.directory = process.argv[3]; - throw new Error('test error'); } @@ -18,8 +16,9 @@ const child = childProcess.spawn(process.execPath, [ '--report-compact', __filename, 'child', - tmpdir.path, -]); +], { + cwd: tmpdir.path +}); child.on('exit', common.mustCall((code) => { assert.strictEqual(code, 1); const reports = helper.findReports(child.pid, tmpdir.path); diff --git a/test/report/test-report-uncaught-exception-primitives.js b/test/report/test-report-uncaught-exception-primitives.js index f46060c1e2e5f5..8e78ad3317d6f2 100644 --- a/test/report/test-report-uncaught-exception-primitives.js +++ b/test/report/test-report-uncaught-exception-primitives.js @@ -7,8 +7,6 @@ const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); if (process.argv[2] === 'child') { - process.report.directory = process.argv[3]; - // eslint-disable-next-line no-throw-literal throw 1; } @@ -18,8 +16,9 @@ const child = childProcess.spawn(process.execPath, [ '--report-uncaught-exception', __filename, 'child', - tmpdir.path, -]); +], { + cwd: tmpdir.path, +}); child.on('exit', common.mustCall((code) => { assert.strictEqual(code, 1); const reports = helper.findReports(child.pid, tmpdir.path); diff --git a/test/report/test-report-uncaught-exception-symbols.js b/test/report/test-report-uncaught-exception-symbols.js index 837e3f074cccb5..09dd46536095da 100644 --- a/test/report/test-report-uncaught-exception-symbols.js +++ b/test/report/test-report-uncaught-exception-symbols.js @@ -7,8 +7,6 @@ const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); if (process.argv[2] === 'child') { - process.report.directory = process.argv[3]; - throw Symbol('foobar'); } @@ -17,8 +15,9 @@ const child = childProcess.spawn(process.execPath, [ '--report-uncaught-exception', __filename, 'child', - tmpdir.path, -]); +], { + cwd: tmpdir.path, +}); child.on('exit', common.mustCall((code) => { assert.strictEqual(code, 1); const reports = helper.findReports(child.pid, tmpdir.path); diff --git a/test/report/test-report-uncaught-exception.js b/test/report/test-report-uncaught-exception.js index 7d760bee58487d..5809104165be9e 100644 --- a/test/report/test-report-uncaught-exception.js +++ b/test/report/test-report-uncaught-exception.js @@ -7,8 +7,6 @@ const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); if (process.argv[2] === 'child') { - process.report.directory = process.argv[3]; - throw new Error('test error'); } @@ -17,8 +15,9 @@ const child = childProcess.spawn(process.execPath, [ '--report-uncaught-exception', __filename, 'child', - tmpdir.path, -]); +], { + cwd: tmpdir.path, +}); child.on('exit', common.mustCall((code) => { assert.strictEqual(code, 1); const reports = helper.findReports(child.pid, tmpdir.path); From 1bad4171ecb99ee5e490da6483d284d00ba4d929 Mon Sep 17 00:00:00 2001 From: legendecas Date: Sun, 14 Aug 2022 20:10:36 +0800 Subject: [PATCH 3/4] fixup! report: skip report if uncaught exception is handled --- doc/api/cli.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index dd7cbbd12fc1b2..558ed07cd852a6 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1131,6 +1131,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 is exiting for uncaught +exceptions. Useful when inspecting the JavaScript stack in conjunction with +native stack and other runtime environment data. ### `--secure-heap=n` From 64eaa0d5b353f6d279b840731051b0127675f377 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 15 Aug 2022 07:18:45 +0000 Subject: [PATCH 4/4] fixup! --- doc/api/cli.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 558ed07cd852a6..678e2dee850e98 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1145,8 +1145,8 @@ changes: `--report-uncaught-exception`. --> -Enables report to be generated when the process is exiting for uncaught -exceptions. Useful when inspecting the JavaScript stack in conjunction with +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`