fixup! report: expose report public native apis

src/node.h

@@ -75,8 +75,9 @@
 #include "v8-platform.h"  // NOLINT(build/include_order)
 #include "node_version.h"  // NODE_MODULE_VERSION
 
-#include <memory>
 #include <functional>
+#include <memory>
+#include <ostream>
 
 // We cannot use __POSIX__ in this header because that's only defined when
 // building Node.js.
@@ -620,16 +621,25 @@ NODE_EXTERN v8::MaybeLocal<v8::Value> PrepareStackTraceCallback(
 // Writes a diagnostic report to a file. If filename is not provided, the
 // default filename includes the date, time, PID, and a sequence number.
 // The report's JavaScript stack trace is taken from err, if present.
-// If isolate or env is nullptr, no information about the isolate and env
+// If isolate is nullptr, no information about the JavaScript environment
 // is included in the report.
+// Returns the filename of the written report.
 NODE_EXTERN std::string TriggerNodeReport(v8::Isolate* isolate,
-                                          Environment* env,
+                                          const char* message,
+                                          const char* trigger,
+                                          const std::string& filename,
+                                          v8::Local<v8::Value> error);
+NODE_EXTERN std::string TriggerNodeReport(Environment* env,
                                           const char* message,
                                           const char* trigger,
                                           const std::string& filename,
                                           v8::Local<v8::Value> error);
 NODE_EXTERN void GetNodeReport(v8::Isolate* isolate,
-                               Environment* env,
+                               const char* message,
+                               const char* trigger,
+                               v8::Local<v8::Value> error,
+                               std::ostream& out);
+NODE_EXTERN void GetNodeReport(Environment* env,
                                const char* message,
                                const char* trigger,
                                v8::Local<v8::Value> error,

src/node_errors.cc

@@ -474,18 +474,14 @@ void OnFatalError(const char* location, const char* message) {
   }
 
   Isolate* isolate = Isolate::TryGetCurrent();
-  Environment* env = nullptr;
-  if (isolate != nullptr) {
-    env = Environment::GetCurrent(isolate);
-  }
   bool report_on_fatalerror;
   {
     Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
     report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
   }
 
   if (report_on_fatalerror) {
-    TriggerNodeReport(isolate, env, message, "FatalError", "", Local<Object>());
+    TriggerNodeReport(isolate, message, "FatalError", "", Local<Object>());
   }
 
   fflush(stderr);
@@ -503,18 +499,14 @@ void OOMErrorHandler(const char* location, bool is_heap_oom) {
   }
 
   Isolate* isolate = Isolate::TryGetCurrent();
-  Environment* env = nullptr;
-  if (isolate != nullptr) {
-    env = Environment::GetCurrent(isolate);
-  }
   bool report_on_fatalerror;
   {
     Mutex::ScopedLock lock(node::per_process::cli_options_mutex);
     report_on_fatalerror = per_process::cli_options->report_on_fatalerror;
   }
 
   if (report_on_fatalerror) {
-    TriggerNodeReport(isolate, env, message, "OOMError", "", Local<Object>());
+    TriggerNodeReport(isolate, message, "OOMError", "", Local<Object>());
   }
 
   fflush(stderr);

src/node_report.cc

@@ -222,12 +222,8 @@ static void WriteNodeReport(Isolate* isolate,
       expected_results += w->RequestInterrupt([&](Environment* env) {
         std::ostringstream os;
 
-        GetNodeReport(env->isolate(),
-                      env,
-                      "Worker thread subreport",
-                      trigger,
-                      Local<Value>(),
-                      os);
+        GetNodeReport(
+            env, "Worker thread subreport", trigger, Local<Value>(), os);
 
         Mutex::ScopedLock lock(workers_mutex);
         worker_infos.emplace_back(os.str());
@@ -790,7 +786,19 @@ static void PrintRelease(JSONWriter* writer) {
 
 // External function to trigger a report, writing to file.
 std::string TriggerNodeReport(Isolate* isolate,
-                              Environment* env,
+                              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,
                               const char* message,
                               const char* trigger,
                               const std::string& name,
@@ -859,6 +867,11 @@ std::string TriggerNodeReport(Isolate* isolate,
     Mutex::ScopedLock lock(per_process::cli_options_mutex);
     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);
 
@@ -876,11 +889,28 @@ std::string TriggerNodeReport(Isolate* isolate,
 
 // External function to trigger a report, writing to a supplied stream.
 void GetNodeReport(Isolate* isolate,
-                   Environment* env,
                    const char* message,
                    const char* trigger,
                    Local<Value> error,
                    std::ostream& out) {
+  Environment* env = nullptr;
+  if (isolate != nullptr) {
+    env = Environment::GetCurrent(isolate);
+  }
+  report::WriteNodeReport(
+      isolate, env, message, trigger, "", out, error, false);
+}
+
+// External function to trigger a report, writing to a supplied stream.
+void GetNodeReport(Environment* env,
+                   const char* message,
+                   const char* trigger,
+                   Local<Value> error,
+                   std::ostream& out) {
+  Isolate* isolate = nullptr;
+  if (env != nullptr) {
+    isolate = env->isolate();
+  }
   report::WriteNodeReport(
       isolate, env, message, trigger, "", out, error, false);
 }

src/node_report.h

@@ -35,21 +35,6 @@ void WriteReport(const v8::FunctionCallbackInfo<v8::Value>& info);
 void GetReport(const v8::FunctionCallbackInfo<v8::Value>& info);
 
 }  // namespace report
-
-// Function declarations - functions in src/node_report.cc
-std::string TriggerNodeReport(v8::Isolate* isolate,
-                              Environment* env,
-                              const char* message,
-                              const char* trigger,
-                              const std::string& name,
-                              v8::Local<v8::Value> error);
-void GetNodeReport(v8::Isolate* isolate,
-                   Environment* env,
-                   const char* message,
-                   const char* trigger,
-                   v8::Local<v8::Value> error,
-                   std::ostream& out);
-
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/node_report_module.cc

@@ -44,8 +44,7 @@ void WriteReport(const FunctionCallbackInfo<Value>& info) {
   else
     error = Local<Value>();
 
-  filename = TriggerNodeReport(
-      isolate, env, *message, *trigger, filename, error);
+  filename = TriggerNodeReport(env, *message, *trigger, filename, error);
   // Return value is the report filename
   info.GetReturnValue().Set(
       String::NewFromUtf8(isolate, filename.c_str()).ToLocalChecked());
@@ -65,8 +64,7 @@ void GetReport(const FunctionCallbackInfo<Value>& info) {
   else
     error = Local<Object>();
 
-  GetNodeReport(
-      isolate, env, "JavaScript API", __func__, error, out);
+  GetNodeReport(env, "JavaScript API", __func__, error, out);
 
   // Return value is the contents of a report as a string.
   info.GetReturnValue().Set(

test/addons/report-api/binding.cc

@@ -11,16 +11,43 @@ void TriggerReport(const FunctionCallbackInfo<Value>& args) {
   Isolate* isolate = args.GetIsolate();
 
   node::TriggerNodeReport(
-      isolate,
+      isolate, "FooMessage", "BarTrigger", std::string(), Local<Value>());
+}
+
+void TriggerReportNoIsolate(const FunctionCallbackInfo<Value>& args) {
+  node::TriggerNodeReport(static_cast<Isolate*>(nullptr),
+                          "FooMessage",
+                          "BarTrigger",
+                          std::string(),
+                          Local<Value>());
+}
+
+void TriggerReportEnv(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+
+  node::TriggerNodeReport(
       node::GetCurrentEnvironment(isolate->GetCurrentContext()),
       "FooMessage",
       "BarTrigger",
       std::string(),
       Local<Value>());
 }
 
+void TriggerReportNoEnv(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+
+  node::TriggerNodeReport(static_cast<node::Environment*>(nullptr),
+                          "FooMessage",
+                          "BarTrigger",
+                          std::string(),
+                          Local<Value>());
+}
+
 void init(Local<Object> exports) {
   NODE_SET_METHOD(exports, "triggerReport", TriggerReport);
+  NODE_SET_METHOD(exports, "triggerReportNoIsolate", TriggerReportNoIsolate);
+  NODE_SET_METHOD(exports, "triggerReportEnv", TriggerReportEnv);
+  NODE_SET_METHOD(exports, "triggerReportNoEnv", TriggerReportNoEnv);
 }
 
 NODE_MODULE(NODE_GYP_MODULE_NAME, init)

test/addons/report-api/test.js

@@ -9,11 +9,11 @@ const tmpdir = require('../../common/tmpdir');
 const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
 const addon = require(binding);
 
-function myAddonMain() {
+function myAddonMain(method, hasJavaScriptFrames) {
   tmpdir.refresh();
   process.report.directory = tmpdir.path;
 
-  addon.triggerReport();
+  addon[method]();
 
   const reports = helper.findReports(process.pid, tmpdir.path);
   assert.strictEqual(reports.length, 1);
@@ -26,7 +26,19 @@ function myAddonMain() {
   assert.strictEqual(content.header.trigger, 'BarTrigger');
 
   // Check that the javascript stack is present.
-  assert.strictEqual(content.javascriptStack.stack.findIndex((frame) => frame.match('myAddonMain')), 0);
+  if (hasJavaScriptFrames) {
+    assert.strictEqual(content.javascriptStack.stack.findIndex((frame) => frame.match('myAddonMain')), 0);
+  } else {
+    assert.strictEqual(content.javascriptStack, undefined);
+  }
 }
 
-myAddonMain();
+const methods = [
+  ['triggerReport', true],
+  ['triggerReportNoIsolate', false],
+  ['triggerReportEnv', true],
+  ['triggerReportNoEnv', false],
+];
+for (const [method, hasJavaScriptFrames] of methods) {
+  myAddonMain(method, hasJavaScriptFrames);
+}

test/cctest/test_report.cc

@@ -25,20 +25,70 @@ class ReportTest : public EnvironmentTestFixture {
   }
 };
 
-TEST_F(ReportTest, ReportWithNoIsolateAndEnv) {
+TEST_F(ReportTest, ReportWithNoIsolate) {
   SealHandleScope handle_scope(isolate_);
 
   std::ostringstream oss;
-  node::GetNodeReport(
-      nullptr, nullptr, "FooMessage", "BarTrigger", Local<Value>(), oss);
+  node::GetNodeReport(static_cast<Isolate*>(nullptr),
+                      "FooMessage",
+                      "BarTrigger",
+                      Local<Value>(),
+                      oss);
 
   // Simple checks on the output string contains the message and trigger.
   std::string actual = oss.str();
   EXPECT_NE(actual.find("FooMessage"), std::string::npos);
   EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
 }
 
-TEST_F(ReportTest, ReportWithIsolateAndEnv) {
+TEST_F(ReportTest, ReportWithNoEnv) {
+  SealHandleScope handle_scope(isolate_);
+
+  std::ostringstream oss;
+  node::GetNodeReport(static_cast<Environment*>(nullptr),
+                      "FooMessage",
+                      "BarTrigger",
+                      Local<Value>(),
+                      oss);
+
+  // Simple checks on the output string contains the message and trigger.
+  std::string actual = oss.str();
+  EXPECT_NE(actual.find("FooMessage"), std::string::npos);
+  EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
+}
+
+TEST_F(ReportTest, ReportWithIsolate) {
+  const HandleScope handle_scope(isolate_);
+  const Argv argv;
+  Env env{handle_scope, argv};
+
+  Local<Context> context = isolate_->GetCurrentContext();
+  Local<Function> fn =
+      Function::New(context, [](const FunctionCallbackInfo<Value>& args) {
+        Isolate* isolate = args.GetIsolate();
+        HandleScope scope(isolate);
+
+        std::ostringstream oss;
+        node::GetNodeReport(isolate, "FooMessage", "BarTrigger", args[0], oss);
+
+        // Simple checks on the output string contains the message and trigger.
+        std::string actual = oss.str();
+        EXPECT_NE(actual.find("FooMessage"), std::string::npos);
+        EXPECT_NE(actual.find("BarTrigger"), std::string::npos);
+
+        report_callback_called = true;
+      }).ToLocalChecked();
+
+  context->Global()
+      ->Set(context, String::NewFromUtf8(isolate_, "foo").ToLocalChecked(), fn)
+      .FromJust();
+
+  node::LoadEnvironment(*env, "foo()").ToLocalChecked();
+
+  EXPECT_TRUE(report_callback_called);
+}
+
+TEST_F(ReportTest, ReportWithEnv) {
   const HandleScope handle_scope(isolate_);
   const Argv argv;
   Env env{handle_scope, argv};
@@ -48,12 +98,14 @@ TEST_F(ReportTest, ReportWithIsolateAndEnv) {
       Function::New(context, [](const FunctionCallbackInfo<Value>& args) {
         Isolate* isolate = args.GetIsolate();
         HandleScope scope(isolate);
-        Environment* env =
-            node::GetCurrentEnvironment(isolate->GetCurrentContext());
 
         std::ostringstream oss;
         node::GetNodeReport(
-            isolate, env, "FooMessage", "BarTrigger", args[0], oss);
+            node::GetCurrentEnvironment(isolate->GetCurrentContext()),
+            "FooMessage",
+            "BarTrigger",
+            args[0],
+            oss);
 
         // Simple checks on the output string contains the message and trigger.
         std::string actual = oss.str();