Skip to content

Commit

Permalink
report: fix missing section javascriptHeap on OOMError
Browse files Browse the repository at this point in the history 
`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 <rafael.nunu@hotmail.com>
  • Loading branch information
@legendecas @juanarbol
legendecas authored and juanarbol committed Oct 11, 2022
1 parent 2ba547a commit 5355a46
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 38 deletions.
46 changes: 28 additions & 18 deletions src/node_report.cc
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -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);

Expand All @@ -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,
Expand Down
10 changes: 7 additions & 3 deletions test/report/test-report-fatalerror-oomerror-compact.js
View file
Expand Up @@ -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.
Expand All @@ -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);
Expand Down
10 changes: 7 additions & 3 deletions test/report/test-report-fatalerror-oomerror-directory.js
View file
Expand Up @@ -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.
Expand All @@ -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);
}
10 changes: 6 additions & 4 deletions test/report/test-report-fatalerror-oomerror-filename.js
View file
Expand Up @@ -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.
Expand All @@ -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);
}
2 changes: 1 addition & 1 deletion test/report/test-report-fatalerror-oomerror-not-set.js
View file
Expand Up @@ -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'),
];

Expand Down
15 changes: 6 additions & 9 deletions test/report/test-report-fatalerror-oomerror-set.js
View file
Expand Up @@ -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.
Expand All @@ -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);
}

0 comments on commit 5355a46

Please sign in to comment.