report: skip report if uncaught exception is handled

legendecas · legendecas · commit 678551c294e0 · 2022-08-16T21:04:48.000+08:00
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: #44208 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -1139,6 +1139,9 @@ Default signal is `SIGUSR2`.
 <!-- YAML
 added: v11.8.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/44208
+    description: Report is not generated if the uncaught exception is handled.
   - version:
      - v13.12.0
      - v12.17.0
@@ -1150,9 +1153,9 @@ changes:
                  `--report-uncaught-exception`.
 -->
 
-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
@@ -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
@@ -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<StackTrace> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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'],
+  ]);
+}));
