From 7f496fefb67c6a9ad22db87eb69104e9a9abb6d2 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 26 Aug 2022 00:20:07 +0800 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/44398 Reviewed-By: Rafael Gonzaga --- src/node_report.cc | 46 +++++++++++-------- ...test-report-fatalerror-oomerror-compact.js | 10 ++-- ...st-report-fatalerror-oomerror-directory.js | 10 ++-- ...est-report-fatalerror-oomerror-filename.js | 10 ++-- ...test-report-fatalerror-oomerror-not-set.js | 2 +- .../test-report-fatalerror-oomerror-set.js | 15 +++--- 6 files changed, 55 insertions(+), 38 deletions(-) diff --git a/src/node_report.cc b/src/node_report.cc index 3970f4ec53127e..38391300cda130 100644 --- 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 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 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 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 index c8ed75e3ede21f..66bc1fa88e624d 100644 --- 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 index 5135760d7db9df..39ba7a1ca4b516 100644 --- 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 index b0995f8d5aeb27..9c3bb7e4d1a3ce 100644 --- 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 index 31bb8f1f3848ba..a54003ac7192ce 100644 --- 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 index ce2a7869f7e6c0..1a05f83d4e3b50 100644 --- 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); }