report: generates report on threads with no isolates

PR-URL: #38994 Backport-PR-URL: #39515 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Sep 3, 2021 · f69c934 · f69c934
1 parent 99493b0
commit f69c934
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 49 deletions.
diff --git a/src/node_errors.cc b/src/node_errors.cc
@@ -424,13 +424,10 @@ void OnFatalError(const char* location, const char* message) {
   }
 
   Isolate* isolate = Isolate::GetCurrent();
-  // TODO(legendecas): investigate failures on triggering node-report with
-  // nullptr isolates.
-  if (isolate == nullptr) {
-    fflush(stderr);
-    ABORT();
+  Environment* env = nullptr;
+  if (isolate != nullptr) {
+    env = Environment::GetCurrent(isolate);
   }
-  Environment* env = Environment::GetCurrent(isolate);
   bool report_on_fatalerror;
   {
     Mutex::ScopedLock lock(node::per_process::cli_options_mutex);

diff --git a/src/node_report.cc b/src/node_report.cc
@@ -267,20 +267,22 @@ static void WriteNodeReport(Isolate* isolate,
   PrintVersionInformation(&writer);
   writer.json_objectend();
 
-  writer.json_objectstart("javascriptStack");
-  // Report summary JavaScript error stack backtrace
-  PrintJavaScriptErrorStack(&writer, isolate, error, trigger);
+  if (isolate != nullptr) {
+    writer.json_objectstart("javascriptStack");
+    // Report summary JavaScript error stack backtrace
+    PrintJavaScriptErrorStack(&writer, isolate, error, trigger);
 
-  // Report summary JavaScript error properties backtrace
-  PrintJavaScriptErrorProperties(&writer, isolate, error);
-  writer.json_objectend();  // the end of 'javascriptStack'
+    // Report summary JavaScript error properties backtrace
+    PrintJavaScriptErrorProperties(&writer, isolate, error);
+    writer.json_objectend();  // the end of 'javascriptStack'
+
+    // Report V8 Heap and Garbage Collector information
+    PrintGCStatistics(&writer, isolate);
+  }
 
   // Report native stack backtrace
   PrintNativeStack(&writer);
 
-  // Report V8 Heap and Garbage Collector information
-  PrintGCStatistics(&writer, isolate);
-
   // Report OS and current thread resource usage
   PrintResourceUsage(&writer);
 

diff --git a/test/common/report.js b/test/common/report.js
@@ -54,17 +54,20 @@ function validateContent(report, fields = []) {
 
 function _validateContent(report, fields = []) {
   const isWindows = process.platform === 'win32';
+  const isJavaScriptThreadReport = report.javascriptStack != null;
 
   // Verify that all sections are present as own properties of the report.
-  const sections = ['header', 'javascriptStack', 'nativeStack',
-                    'javascriptHeap', 'libuv', 'environmentVariables',
+  const sections = ['header', 'nativeStack', 'libuv', 'environmentVariables',
                     'sharedObjects', 'resourceUsage', 'workers'];
   if (!isWindows)
     sections.push('userLimits');
 
   if (report.uvthreadResourceUsage)
     sections.push('uvthreadResourceUsage');
 
+  if (isJavaScriptThreadReport)
+    sections.push('javascriptStack', 'javascriptHeap');
+
   checkForUnknownFields(report, sections);
   sections.forEach((section) => {
     assert(report.hasOwnProperty(section));
@@ -163,19 +166,6 @@ function _validateContent(report, fields = []) {
   });
   assert.strictEqual(header.host, os.hostname());
 
-  // Verify the format of the javascriptStack section.
-  checkForUnknownFields(report.javascriptStack,
-                        ['message', 'stack', 'errorProperties']);
-  assert.strictEqual(typeof report.javascriptStack.errorProperties,
-                     'object');
-  assert.strictEqual(typeof report.javascriptStack.message, 'string');
-  if (report.javascriptStack.stack !== undefined) {
-    assert(Array.isArray(report.javascriptStack.stack));
-    report.javascriptStack.stack.forEach((frame) => {
-      assert.strictEqual(typeof frame, 'string');
-    });
-  }
-
   // Verify the format of the nativeStack section.
   assert(Array.isArray(report.nativeStack));
   report.nativeStack.forEach((frame) => {
@@ -186,26 +176,41 @@ function _validateContent(report, fields = []) {
     assert.strictEqual(typeof frame.symbol, 'string');
   });
 
-  // Verify the format of the javascriptHeap section.
-  const heap = report.javascriptHeap;
-  const jsHeapFields = ['totalMemory', 'totalCommittedMemory', 'usedMemory',
-                        'availableMemory', 'memoryLimit', 'heapSpaces'];
-  checkForUnknownFields(heap, jsHeapFields);
-  assert(Number.isSafeInteger(heap.totalMemory));
-  assert(Number.isSafeInteger(heap.totalCommittedMemory));
-  assert(Number.isSafeInteger(heap.usedMemory));
-  assert(Number.isSafeInteger(heap.availableMemory));
-  assert(Number.isSafeInteger(heap.memoryLimit));
-  assert(typeof heap.heapSpaces === 'object' && heap.heapSpaces !== null);
-  const heapSpaceFields = ['memorySize', 'committedMemory', 'capacity', 'used',
-                           'available'];
-  Object.keys(heap.heapSpaces).forEach((spaceName) => {
-    const space = heap.heapSpaces[spaceName];
-    checkForUnknownFields(space, heapSpaceFields);
-    heapSpaceFields.forEach((field) => {
-      assert(Number.isSafeInteger(space[field]));
+  if (isJavaScriptThreadReport) {
+    // Verify the format of the javascriptStack section.
+    checkForUnknownFields(report.javascriptStack,
+                          ['message', 'stack', 'errorProperties']);
+    assert.strictEqual(typeof report.javascriptStack.errorProperties,
+                       'object');
+    assert.strictEqual(typeof report.javascriptStack.message, 'string');
+    if (report.javascriptStack.stack !== undefined) {
+      assert(Array.isArray(report.javascriptStack.stack));
+      report.javascriptStack.stack.forEach((frame) => {
+        assert.strictEqual(typeof frame, 'string');
+      });
+    }
+
+    // Verify the format of the javascriptHeap section.
+    const heap = report.javascriptHeap;
+    const jsHeapFields = ['totalMemory', 'totalCommittedMemory', 'usedMemory',
+                          'availableMemory', 'memoryLimit', 'heapSpaces'];
+    checkForUnknownFields(heap, jsHeapFields);
+    assert(Number.isSafeInteger(heap.totalMemory));
+    assert(Number.isSafeInteger(heap.totalCommittedMemory));
+    assert(Number.isSafeInteger(heap.usedMemory));
+    assert(Number.isSafeInteger(heap.availableMemory));
+    assert(Number.isSafeInteger(heap.memoryLimit));
+    assert(typeof heap.heapSpaces === 'object' && heap.heapSpaces !== null);
+    const heapSpaceFields = ['memorySize', 'committedMemory', 'capacity',
+                             'used', 'available'];
+    Object.keys(heap.heapSpaces).forEach((spaceName) => {
+      const space = heap.heapSpaces[spaceName];
+      checkForUnknownFields(space, heapSpaceFields);
+      heapSpaceFields.forEach((field) => {
+        assert(Number.isSafeInteger(space[field]));
+      });
     });
-  });
+  }
 
   // Verify the format of the resourceUsage section.
   const usage = report.resourceUsage;

diff --git a/test/node-api/test_fatal/test_threads_report.js b/test/node-api/test_fatal/test_threads_report.js
@@ -0,0 +1,36 @@
+'use strict';
+const common = require('../../common');
+const helper = require('../../common/report.js');
+const tmpdir = require('../../common/tmpdir');
+
+const assert = require('assert');
+const child_process = require('child_process');
+const test_fatal = require(`./build/${common.buildType}/test_fatal`);
+
+if (common.buildType === 'Debug')
+  common.skip('as this will currently fail with a Debug check ' +
+              'in v8::Isolate::GetCurrent()');
+
+// Test in a child process because the test code will trigger a fatal error
+// that crashes the process.
+if (process.argv[2] === 'child') {
+  test_fatal.TestThread();
+  // Busy loop to allow the work thread to abort.
+  while (true) {}
+}
+
+tmpdir.refresh();
+const p = child_process.spawnSync(
+  process.execPath,
+  [ '--report-on-fatalerror', __filename, 'child' ],
+  { cwd: tmpdir.path });
+assert.ifError(p.error);
+assert.ok(p.stderr.toString().includes(
+  'FATAL ERROR: work_thread foobar'));
+assert.ok(p.status === 134 || p.signal === 'SIGABRT');
+
+const reports = helper.findReports(p.pid, tmpdir.path);
+assert.strictEqual(reports.length, 1);
+
+const report = reports[0];
+helper.validate(report);