Skip to content

Commit 1316825

Browse files
legendecaslegendecas
committed
fixup! report: expose report public native apis
1 parent 8c148bf commit 1316825

File tree

8 files changed

+159
-53
lines changed

8 files changed

+159
-53
lines changed

‎src/node.h

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,9 @@
7575
#include "v8-platform.h" // NOLINT(build/include_order)
7676
#include "node_version.h" // NODE_MODULE_VERSION
7777

78-
#include <memory>
7978
#include <functional>
79+
#include <memory>
80+
#include <ostream>
8081

8182
// We cannot use __POSIX__ in this header because that's only defined when
8283
// building Node.js.
@@ -620,16 +621,25 @@ NODE_EXTERN v8::MaybeLocal<v8::Value> PrepareStackTraceCallback(
620621
// Writes a diagnostic report to a file. If filename is not provided, the
621622
// default filename includes the date, time, PID, and a sequence number.
622623
// The report's JavaScript stack trace is taken from err, if present.
623-
// If isolate or env is nullptr, no information about the isolate and env
624+
// If isolate is nullptr, no information about the JavaScript environment
624625
// is included in the report.
626+
// Returns the filename of the written report.
625627
NODE_EXTERN std::string TriggerNodeReport(v8::Isolate* isolate,
626-
Environment* env,
628+
const char* message,
629+
const char* trigger,
630+
const std::string& filename,
631+
v8::Local<v8::Value> error);
632+
NODE_EXTERN std::string TriggerNodeReport(Environment* env,
627633
const char* message,
628634
const char* trigger,
629635
const std::string& filename,
630636
v8::Local<v8::Value> error);
631637
NODE_EXTERN void GetNodeReport(v8::Isolate* isolate,
632-
Environment* env,
638+
const char* message,
639+
const char* trigger,
640+
v8::Local<v8::Value> error,
641+
std::ostream& out);
642+
NODE_EXTERN void GetNodeReport(Environment* env,
633643
const char* message,
634644
const char* trigger,
635645
v8::Local<v8::Value> error,

‎src/node_errors.cc

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -474,18 +474,14 @@ void OnFatalError(const char* location, const char* message) {
474474
}
475475

476476
Isolate* isolate = Isolate::TryGetCurrent();
477-
Environment* env = nullptr;
478-
if (isolate != nullptr) {
479-
env = Environment::GetCurrent(isolate);
480-
}
481477
bool report_on_fatalerror;
482478
{
483479
Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
484480
report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
485481
}
486482

487483
if (report_on_fatalerror) {
488-
TriggerNodeReport(isolate, env, message, "FatalError", "", Local<Object>());
484+
TriggerNodeReport(isolate, message, "FatalError", "", Local<Object>());
489485
}
490486

491487
fflush(stderr);
@@ -503,18 +499,14 @@ void OOMErrorHandler(const char* location, bool is_heap_oom) {
503499
}
504500

505501
Isolate* isolate = Isolate::TryGetCurrent();
506-
Environment* env = nullptr;
507-
if (isolate != nullptr) {
508-
env = Environment::GetCurrent(isolate);
509-
}
510502
bool report_on_fatalerror;
511503
{
512504
Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
513505
report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
514506
}
515507

516508
if (report_on_fatalerror) {
517-
TriggerNodeReport(isolate, env, message, "OOMError", "", Local<Object>());
509+
TriggerNodeReport(isolate, message, "OOMError", "", Local<Object>());
518510
}
519511

520512
fflush(stderr);

‎src/node_report.cc

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -222,12 +222,8 @@ static void WriteNodeReport(Isolate* isolate,
222222
expected_results += w->RequestInterrupt([&](Environment* env) {
223223
std::ostringstream os;
224224

225-
GetNodeReport(env->isolate(),
226-
env,
227-
"Worker thread subreport",
228-
trigger,
229-
Local<Value>(),
230-
os);
225+
GetNodeReport(
226+
env, "Worker thread subreport", trigger, Local<Value>(), os);
231227

232228
Mutex::ScopedLock lock(workers_mutex);
233229
worker_infos.emplace_back(os.str());
@@ -790,7 +786,19 @@ static void PrintRelease(JSONWriter* writer) {
790786

791787
// External function to trigger a report, writing to file.
792788
std::string TriggerNodeReport(Isolate* isolate,
793-
Environment* env,
789+
const char* message,
790+
const char* trigger,
791+
const std::string& name,
792+
Local<Value> error) {
793+
Environment* env = nullptr;
794+
if (isolate != nullptr) {
795+
env = Environment::GetCurrent(isolate);
796+
}
797+
return TriggerNodeReport(env, message, trigger, name, error);
798+
}
799+
800+
// External function to trigger a report, writing to file.
801+
std::string TriggerNodeReport(Environment* env,
794802
const char* message,
795803
const char* trigger,
796804
const std::string& name,
@@ -859,6 +867,11 @@ std::string TriggerNodeReport(Isolate* isolate,
859867
Mutex::ScopedLock lock(per_process::cli_options_mutex);
860868
compact = per_process::cli_options->report_compact;
861869
}
870+
871+
Isolate* isolate = nullptr;
872+
if (env != nullptr) {
873+
isolate = env->isolate();
874+
}
862875
report::WriteNodeReport(
863876
isolate, env, message, trigger, filename, *outstream, error, compact);
864877

@@ -876,11 +889,28 @@ std::string TriggerNodeReport(Isolate* isolate,
876889

877890
// External function to trigger a report, writing to a supplied stream.
878891
void GetNodeReport(Isolate* isolate,
879-
Environment* env,
880892
const char* message,
881893
const char* trigger,
882894
Local<Value> error,
883895
std::ostream& out) {
896+
Environment* env = nullptr;
897+
if (isolate != nullptr) {
898+
env = Environment::GetCurrent(isolate);
899+
}
900+
report::WriteNodeReport(
901+
isolate, env, message, trigger, "", out, error, false);
902+
}
903+
904+
// External function to trigger a report, writing to a supplied stream.
905+
void GetNodeReport(Environment* env,
906+
const char* message,
907+
const char* trigger,
908+
Local<Value> error,
909+
std::ostream& out) {
910+
Isolate* isolate = nullptr;
911+
if (env != nullptr) {
912+
isolate = env->isolate();
913+
}
884914
report::WriteNodeReport(
885915
isolate, env, message, trigger, "", out, error, false);
886916
}

‎src/node_report.h

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,21 +35,6 @@ void WriteReport(const v8::FunctionCallbackInfo<v8::Value>& info);
3535
void GetReport(const v8::FunctionCallbackInfo<v8::Value>& info);
3636

3737
} // namespace report
38-
39-
// Function declarations - functions in src/node_report.cc
40-
std::string TriggerNodeReport(v8::Isolate* isolate,
41-
Environment* env,
42-
const char* message,
43-
const char* trigger,
44-
const std::string& name,
45-
v8::Local<v8::Value> error);
46-
void GetNodeReport(v8::Isolate* isolate,
47-
Environment* env,
48-
const char* message,
49-
const char* trigger,
50-
v8::Local<v8::Value> error,
51-
std::ostream& out);
52-
5338
} // namespace node
5439

5540
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

‎src/node_report_module.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,7 @@ void WriteReport(const FunctionCallbackInfo<Value>& info) {
4444
else
4545
error = Local<Value>();
4646

47-
filename = TriggerNodeReport(
48-
isolate, env, *message, *trigger, filename, error);
47+
filename = TriggerNodeReport(env, *message, *trigger, filename, error);
4948
// Return value is the report filename
5049
info.GetReturnValue().Set(
5150
String::NewFromUtf8(isolate, filename.c_str()).ToLocalChecked());
@@ -65,8 +64,7 @@ void GetReport(const FunctionCallbackInfo<Value>& info) {
6564
else
6665
error = Local<Object>();
6766

68-
GetNodeReport(
69-
isolate, env, "JavaScript API", __func__, error, out);
67+
GetNodeReport(env, "JavaScript API", __func__, error, out);
7068

7169
// Return value is the contents of a report as a string.
7270
info.GetReturnValue().Set(

‎test/addons/report-api/binding.cc

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,43 @@ void TriggerReport(const FunctionCallbackInfo<Value>& args) {
1111
Isolate* isolate = args.GetIsolate();
1212

1313
node::TriggerNodeReport(
14-
isolate,
14+
isolate, "FooMessage", "BarTrigger", std::string(), Local<Value>());
15+
}
16+
17+
void TriggerReportNoIsolate(const FunctionCallbackInfo<Value>& args) {
18+
node::TriggerNodeReport(static_cast<Isolate*>(nullptr),
19+
"FooMessage",
20+
"BarTrigger",
21+
std::string(),
22+
Local<Value>());
23+
}
24+
25+
void TriggerReportEnv(const FunctionCallbackInfo<Value>& args) {
26+
Isolate* isolate = args.GetIsolate();
27+
28+
node::TriggerNodeReport(
1529
node::GetCurrentEnvironment(isolate->GetCurrentContext()),
1630
"FooMessage",
1731
"BarTrigger",
1832
std::string(),
1933
Local<Value>());
2034
}
2135

36+
void TriggerReportNoEnv(const FunctionCallbackInfo<Value>& args) {
37+
Isolate* isolate = args.GetIsolate();
38+
39+
node::TriggerNodeReport(static_cast<node::Environment*>(nullptr),
40+
"FooMessage",
41+
"BarTrigger",
42+
std::string(),
43+
Local<Value>());
44+
}
45+
2246
void init(Local<Object> exports) {
2347
NODE_SET_METHOD(exports, "triggerReport", TriggerReport);
48+
NODE_SET_METHOD(exports, "triggerReportNoIsolate", TriggerReportNoIsolate);
49+
NODE_SET_METHOD(exports, "triggerReportEnv", TriggerReportEnv);
50+
NODE_SET_METHOD(exports, "triggerReportNoEnv", TriggerReportNoEnv);
2451
}
2552

2653
NODE_MODULE(NODE_GYP_MODULE_NAME, init)

‎test/addons/report-api/test.js

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ const tmpdir = require('../../common/tmpdir');
99
const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
1010
const addon = require(binding);
1111

12-
function myAddonMain() {
12+
function myAddonMain(method, hasJavaScriptFrames) {
1313
tmpdir.refresh();
1414
process.report.directory = tmpdir.path;
1515

16-
addon.triggerReport();
16+
addon[method]();
1717

1818
const reports = helper.findReports(process.pid, tmpdir.path);
1919
assert.strictEqual(reports.length, 1);
@@ -26,7 +26,19 @@ function myAddonMain() {
2626
assert.strictEqual(content.header.trigger, 'BarTrigger');
2727

2828
// Check that the javascript stack is present.
29-
assert.strictEqual(content.javascriptStack.stack.findIndex((frame) => frame.match('myAddonMain')), 0);
29+
if (hasJavaScriptFrames) {
30+
assert.strictEqual(content.javascriptStack.stack.findIndex((frame) => frame.match('myAddonMain')), 0);
31+
} else {
32+
assert.strictEqual(content.javascriptStack, undefined);
33+
}
3034
}
3135

32-
myAddonMain();
36+
const methods = [
37+
['triggerReport', true],
38+
['triggerReportNoIsolate', false],
39+
['triggerReportEnv', true],
40+
['triggerReportNoEnv', false],
41+
];
42+
for (const [method, hasJavaScriptFrames] of methods) {
43+
myAddonMain(method, hasJavaScriptFrames);
44+
}

‎test/cctest/test_report.cc

Lines changed: 59 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,20 +25,70 @@ class ReportTest : public EnvironmentTestFixture {
2525
}
2626
};
2727

28-
TEST_F(ReportTest, ReportWithNoIsolateAndEnv) {
28+
TEST_F(ReportTest, ReportWithNoIsolate) {
2929
SealHandleScope handle_scope(isolate_);
3030

3131
std::ostringstream oss;
32-
node::GetNodeReport(
33-
nullptr, nullptr, "FooMessage", "BarTrigger", Local<Value>(), oss);
32+
node::GetNodeReport(static_cast<Isolate*>(nullptr),
33+
"FooMessage",
34+
"BarTrigger",
35+
Local<Value>(),
36+
oss);
3437

3538
// Simple checks on the output string contains the message and trigger.
3639
std::string actual = oss.str();
3740
EXPECT_NE(actual.find("FooMessage"), std::string::npos);
3841
EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
3942
}
4043

41-
TEST_F(ReportTest, ReportWithIsolateAndEnv) {
44+
TEST_F(ReportTest, ReportWithNoEnv) {
45+
SealHandleScope handle_scope(isolate_);
46+
47+
std::ostringstream oss;
48+
node::GetNodeReport(static_cast<Environment*>(nullptr),
49+
"FooMessage",
50+
"BarTrigger",
51+
Local<Value>(),
52+
oss);
53+
54+
// Simple checks on the output string contains the message and trigger.
55+
std::string actual = oss.str();
56+
EXPECT_NE(actual.find("FooMessage"), std::string::npos);
57+
EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
58+
}
59+
60+
TEST_F(ReportTest, ReportWithIsolate) {
61+
const HandleScope handle_scope(isolate_);
62+
const Argv argv;
63+
Env env{handle_scope, argv};
64+
65+
Local<Context> context = isolate_->GetCurrentContext();
66+
Local<Function> fn =
67+
Function::New(context, [](const FunctionCallbackInfo<Value>& args) {
68+
Isolate* isolate = args.GetIsolate();
69+
HandleScope scope(isolate);
70+
71+
std::ostringstream oss;
72+
node::GetNodeReport(isolate, "FooMessage", "BarTrigger", args[0], oss);
73+
74+
// Simple checks on the output string contains the message and trigger.
75+
std::string actual = oss.str();
76+
EXPECT_NE(actual.find("FooMessage"), std::string::npos);
77+
EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
78+
79+
report_callback_called = true;
80+
}).ToLocalChecked();
81+
82+
context->Global()
83+
->Set(context, String::NewFromUtf8(isolate_, "foo").ToLocalChecked(), fn)
84+
.FromJust();
85+
86+
node::LoadEnvironment(*env, "foo()").ToLocalChecked();
87+
88+
EXPECT_TRUE(report_callback_called);
89+
}
90+
91+
TEST_F(ReportTest, ReportWithEnv) {
4292
const HandleScope handle_scope(isolate_);
4393
const Argv argv;
4494
Env env{handle_scope, argv};
@@ -48,12 +98,14 @@ TEST_F(ReportTest, ReportWithIsolateAndEnv) {
4898
Function::New(context, [](const FunctionCallbackInfo<Value>& args) {
4999
Isolate* isolate = args.GetIsolate();
50100
HandleScope scope(isolate);
51-
Environment* env =
52-
node::GetCurrentEnvironment(isolate->GetCurrentContext());
53101

54102
std::ostringstream oss;
55103
node::GetNodeReport(
56-
isolate, env, "FooMessage", "BarTrigger", args[0], oss);
104+
node::GetCurrentEnvironment(isolate->GetCurrentContext()),
105+
"FooMessage",
106+
"BarTrigger",
107+
args[0],
108+
oss);
57109

58110
// Simple checks on the output string contains the message and trigger.
59111
std::string actual = oss.str();

0 commit comments

Comments
 (0)