report: fix missing section javascriptHeap on OOMError

`Environment::GetCurrent` may not available in the context of OOM. Removes the cyclic `Environment::GetCurrent` and `env->isolate()` calls to ensure both `isolate` and `env` is present if available. However, this behavior is not guaranteed. As `Environment::GetCurrent` didn't allocate new handles in the heap, when a Context is entered it can still get the valid env pointer. Removes the unstable assertion of the absence of env in the test. PR-URL: #44398 Reviewed-By: Rafael Gonzaga <[email protected]>
nodejs · Sep 7, 2022 · b17cc87 · b17cc87
1 parent 54b6ed5
commit b17cc87
Show file tree

Hide file tree

Showing 6 changed files with 55 additions and 38 deletions.
diff --git a/src/node_report.cc b/src/node_report.cc
@@ -784,21 +784,8 @@ static void PrintRelease(JSONWriter* writer) {
 
 }  // namespace report
 
-// External function to trigger a report, writing to file.
 std::string TriggerNodeReport(Isolate* isolate,
-                              const char* message,
-                              const char* trigger,
-                              const std::string& name,
-                              Local<Value> error) {
-  Environment* env = nullptr;
-  if (isolate != nullptr) {
-    env = Environment::GetCurrent(isolate);
-  }
-  return TriggerNodeReport(env, message, trigger, name, error);
-}
-
-// External function to trigger a report, writing to file.
-std::string TriggerNodeReport(Environment* env,
+                              Environment* env,
                               const char* message,
                               const char* trigger,
                               const std::string& name,
@@ -868,10 +855,6 @@ std::string TriggerNodeReport(Environment* env,
     compact = per_process::cli_options->report_compact;
   }
 
-  Isolate* isolate = nullptr;
-  if (env != nullptr) {
-    isolate = env->isolate();
-  }
   report::WriteNodeReport(
       isolate, env, message, trigger, filename, *outstream, error, compact);
 
@@ -887,6 +870,33 @@ std::string TriggerNodeReport(Environment* env,
   return filename;
 }
 
+// External function to trigger a report, writing to file.
+std::string TriggerNodeReport(Isolate* isolate,
+                              const char* message,
+                              const char* trigger,
+                              const std::string& name,
+                              Local<Value> error) {
+  Environment* env = nullptr;
+  if (isolate != nullptr) {
+    env = Environment::GetCurrent(isolate);
+  }
+  return TriggerNodeReport(isolate, env, message, trigger, name, error);
+}
+
+// External function to trigger a report, writing to file.
+std::string TriggerNodeReport(Environment* env,
+                              const char* message,
+                              const char* trigger,
+                              const std::string& name,
+                              Local<Value> error) {
+  return TriggerNodeReport(env != nullptr ? env->isolate() : nullptr,
+                           env,
+                           message,
+                           trigger,
+                           name,
+                           error);
+}
+
 // External function to trigger a report, writing to a supplied stream.
 void GetNodeReport(Isolate* isolate,
                    const char* message,

diff --git a/test/report/test-report-fatalerror-oomerror-compact.js b/test/report/test-report-fatalerror-oomerror-compact.js
@@ -12,9 +12,13 @@ const fixtures = require('../common/fixtures');
 
 // Common args that will cause an out-of-memory error for child process.
 const ARGS = [
-  '--max-old-space-size=20',
+  '--max-heap-size=20',
   fixtures.path('report-oom'),
 ];
+const REPORT_FIELDS = [
+  ['header.trigger', 'OOMError'],
+  ['javascriptHeap.memoryLimit', 20 * 1024 * 1024 /* 20MB */],
+];
 
 {
   // Verify that --report-compact is respected when set.
@@ -27,8 +31,8 @@ const ARGS = [
   assert.strictEqual(reports.length, 1);
 
   const report = reports[0];
-  helper.validate(report);
-  assert.strictEqual(require(report).header.threadId, null);
+  helper.validate(report, REPORT_FIELDS);
+
   // Subtract 1 because "xx\n".split("\n") => [ 'xx', '' ].
   const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
   assert.strictEqual(lines, 1);

diff --git a/test/report/test-report-fatalerror-oomerror-directory.js b/test/report/test-report-fatalerror-oomerror-directory.js
@@ -12,9 +12,13 @@ const fixtures = require('../common/fixtures');
 
 // Common args that will cause an out-of-memory error for child process.
 const ARGS = [
-  '--max-old-space-size=20',
+  '--max-heap-size=20',
   fixtures.path('report-oom'),
 ];
+const REPORT_FIELDS = [
+  ['header.trigger', 'OOMError'],
+  ['javascriptHeap.memoryLimit', 20 * 1024 * 1024 /* 20MB */],
+];
 
 {
   // Verify that --report-directory is respected when set.
@@ -29,8 +33,8 @@ const ARGS = [
   assert.strictEqual(reports.length, 1);
 
   const report = reports[0];
-  helper.validate(report);
-  assert.strictEqual(require(report).header.threadId, null);
+  helper.validate(report, REPORT_FIELDS);
+
   const lines = fs.readFileSync(report, 'utf8').split('\n').length - 1;
   assert(lines > 10);
 }
diff --git a/test/report/test-report-fatalerror-oomerror-filename.js b/test/report/test-report-fatalerror-oomerror-filename.js
@@ -11,9 +11,13 @@ const fixtures = require('../common/fixtures');
 
 // Common args that will cause an out-of-memory error for child process.
 const ARGS = [
-  '--max-old-space-size=20',
+  '--max-heap-size=20',
   fixtures.path('report-oom'),
 ];
+const REPORT_FIELDS = [
+  ['header.trigger', 'OOMError'],
+  ['javascriptHeap.memoryLimit', 20 * 1024 * 1024 /* 20MB */],
+];
 
 {
   // Verify that --report-compact is respected when set.
@@ -34,7 +38,5 @@ const ARGS = [
   const lines = child.stderr.split('\n');
   // Skip over unavoidable free-form output and gc log from V8.
   const report = lines.find((i) => i.startsWith('{'));
-  const json = JSON.parse(report);
-
-  assert.strictEqual(json.header.threadId, null);
+  helper.validateContent(report, REPORT_FIELDS);
 }
diff --git a/test/report/test-report-fatalerror-oomerror-not-set.js b/test/report/test-report-fatalerror-oomerror-not-set.js
@@ -11,7 +11,7 @@ const fixtures = require('../common/fixtures');
 
 // Common args that will cause an out-of-memory error for child process.
 const ARGS = [
-  '--max-old-space-size=20',
+  '--max-heap-size=20',
   fixtures.path('report-oom'),
 ];
 

diff --git a/test/report/test-report-fatalerror-oomerror-set.js b/test/report/test-report-fatalerror-oomerror-set.js
@@ -11,9 +11,13 @@ const fixtures = require('../common/fixtures');
 
 // Common args that will cause an out-of-memory error for child process.
 const ARGS = [
-  '--max-old-space-size=20',
+  '--max-heap-size=20',
   fixtures.path('report-oom'),
 ];
+const REPORT_FIELDS = [
+  ['header.trigger', 'OOMError'],
+  ['javascriptHeap.memoryLimit', 20 * 1024 * 1024 /* 20MB */],
+];
 
 {
   // Verify that --report-on-fatalerror is respected when set.
@@ -26,12 +30,5 @@ const ARGS = [
   assert.strictEqual(reports.length, 1);
 
   const report = reports[0];
-  helper.validate(report);
-
-  const content = require(report);
-  // Errors occur in a context where env is not available, so thread ID is
-  // unknown. Assert this, to verify that the underlying env-less situation is
-  // actually reached.
-  assert.strictEqual(content.header.threadId, null);
-  assert.strictEqual(content.header.trigger, 'OOMError');
+  helper.validate(report, REPORT_FIELDS);
 }