feat: add error event for utility process (33-x-y) (#43997)

* feat: add error event for utility process (#43774)

* feat: add error event for utility process

* chore: use public report api

* chore: fix lint

* doc: mark error event as experimental

---------

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* fixup! feat: add error event for utility process (#43774)

remove #include "electron/mas.h"

this header did not exist before c1c8fbfd9

---------

Co-authored-by: Robo <hop2deep@gmail.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

docs/api/utility-process.md

@@ -116,6 +116,17 @@ When the child process exits, then the value is `null` after the `exit` event is
 
 Emitted once the child process has spawned successfully.
 
+#### Event: 'error' _Experimental_
+
+Returns:
+
+* `type` string - Type of error. One of the following values:
+  * `FatalError`
+* `location` string - Source location from where the error originated.
+* `report` string - [`Node.js diagnostic report`][].
+
+Emitted when the child process needs to terminate due to non continuable error from V8.
+
 #### Event: 'exit'
 
 Returns:
@@ -138,3 +149,4 @@ Emitted when the child process sends a message using [`process.parentPort.postMe
 [stdio]: https://nodejs.org/dist/latest/docs/api/child_process.html#optionsstdio
 [event-emitter]: https://nodejs.org/api/events.html#events_class_eventemitter
 [`MessagePortMain`]: message-port-main.md
+[`Node.js diagnostic report`]: https://nodejs.org/docs/latest/api/report.html#diagnostic-report

shell/browser/api/electron_api_utility_process.cc

@@ -223,7 +223,8 @@ UtilityProcessWrapper::UtilityProcessWrapper(
   params->use_network_observer_from_url_loader_factory =
       create_network_observer;
 
-  node_service_remote_->Initialize(std::move(params));
+  node_service_remote_->Initialize(std::move(params),
+                                   receiver_.BindNewPipeAndPassRemote());
 }
 
 UtilityProcessWrapper::~UtilityProcessWrapper() {
@@ -258,8 +259,9 @@ void UtilityProcessWrapper::HandleTermination(uint64_t exit_code) {
 void UtilityProcessWrapper::OnServiceProcessDisconnected(
     uint32_t exit_code,
     const std::string& description) {
-  if (description == "process_exit_termination")
+  if (description == "process_exit_termination") {
     HandleTermination(exit_code);
+  }
 }
 
 void UtilityProcessWrapper::OnServiceProcessTerminatedNormally(
@@ -372,6 +374,11 @@ bool UtilityProcessWrapper::Accept(mojo::Message* mojo_message) {
   return true;
 }
 
+void UtilityProcessWrapper::OnV8FatalError(const std::string& location,
+                                           const std::string& report) {
+  EmitWithoutEvent("error", "FatalError", location, report);
+}
+
 // static
 raw_ptr<UtilityProcessWrapper> UtilityProcessWrapper::FromProcessId(
     base::ProcessId pid) {

shell/browser/api/electron_api_utility_process.h

@@ -44,6 +44,7 @@ class UtilityProcessWrapper final
       public gin_helper::Pinnable<UtilityProcessWrapper>,
       public gin_helper::EventEmitterMixin<UtilityProcessWrapper>,
       public mojo::MessageReceiver,
+      public node::mojom::NodeServiceClient,
       public content::ServiceProcessHost::Observer {
  public:
   enum class IOHandle : size_t { STDIN = 0, STDOUT = 1, STDERR = 2 };
@@ -81,6 +82,10 @@ class UtilityProcessWrapper final
   // mojo::MessageReceiver
   bool Accept(mojo::Message* mojo_message) override;
 
+  // node::mojom::NodeServiceClient
+  void OnV8FatalError(const std::string& location,
+                      const std::string& report) override;
+
   // content::ServiceProcessHost::Observer
   void OnServiceProcessTerminatedNormally(
       const content::ServiceProcessInfo& info) override;
@@ -102,6 +107,7 @@ class UtilityProcessWrapper final
   bool connector_closed_ = false;
   std::unique_ptr<mojo::Connector> connector_;
   blink::MessagePortDescriptor host_port_;
+  mojo::Receiver<node::mojom::NodeServiceClient> receiver_{this};
   mojo::Remote<node::mojom::NodeService> node_service_remote_;
   std::optional<electron::URLLoaderNetworkObserver>
       url_loader_network_observer_;

shell/common/node_includes.h

@@ -27,6 +27,7 @@
 #include "node_options-inl.h"
 #include "node_options.h"
 #include "node_platform.h"
+#include "node_report.h"
 #include "tracing/agent.h"
 
 #include "electron/pop_node_defines.h"

shell/services/node/node_service.cc

@@ -4,6 +4,7 @@
 
 #include "shell/services/node/node_service.h"
 
+#include <sstream>
 #include <utility>
 
 #include "base/command_line.h"
@@ -20,8 +21,32 @@
 #include "shell/common/node_includes.h"
 #include "shell/services/node/parent_port.h"
 
+#if !IS_MAS_BUILD()
+#include "shell/common/crash_keys.h"
+#endif
+
 namespace electron {
 
+mojo::Remote<node::mojom::NodeServiceClient> g_client_remote;
+
+void V8FatalErrorCallback(const char* location, const char* message) {
+  if (g_client_remote.is_bound() && g_client_remote.is_connected()) {
+    auto* isolate = v8::Isolate::TryGetCurrent();
+    std::ostringstream outstream;
+    node::GetNodeReport(isolate, message, location,
+                        v8::Local<v8::Object>() /* error */, outstream);
+    g_client_remote->OnV8FatalError(location, outstream.str());
+  }
+
+#if !IS_MAS_BUILD()
+  electron::crash_keys::SetCrashKey("electron.v8-fatal.message", message);
+  electron::crash_keys::SetCrashKey("electron.v8-fatal.location", location);
+#endif
+
+  volatile int* zero = nullptr;
+  *zero = 0;
+}
+
 URLLoaderBundle::URLLoaderBundle() = default;
 
 URLLoaderBundle::~URLLoaderBundle() = default;
@@ -73,12 +98,20 @@ NodeService::~NodeService() {
     js_env_->DestroyMicrotasksRunner();
     node::Stop(node_env_.get(), node::StopFlags::kDoNotTerminateIsolate);
   }
+  if (g_client_remote.is_bound()) {
+    g_client_remote.reset();
+  }
 }
 
-void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) {
+void NodeService::Initialize(
+    node::mojom::NodeServiceParamsPtr params,
+    mojo::PendingRemote<node::mojom::NodeServiceClient> client_pending_remote) {
   if (NodeBindings::IsInitialized())
     return;
 
+  g_client_remote.Bind(std::move(client_pending_remote));
+  g_client_remote.reset_on_disconnect();
+
   ParentPort::GetInstance()->Initialize(std::move(params->port));
 
   URLLoaderBundle::GetInstance()->SetURLLoaderFactory(
@@ -105,6 +138,9 @@ void NodeService::Initialize(node::mojom::NodeServiceParamsPtr params) {
       js_env_->isolate()->GetCurrentContext(), js_env_->platform(),
       params->args, params->exec_args);
 
+  // Override the default handler set by NodeBindings.
+  node_env_->isolate()->SetFatalErrorHandler(V8FatalErrorCallback);
+
   node::SetProcessExitHandler(
       node_env_.get(), [this](node::Environment* env, int exit_code) {
         // Destroy node platform.

shell/services/node/node_service.h

@@ -61,7 +61,9 @@ class NodeService : public node::mojom::NodeService {
   NodeService& operator=(const NodeService&) = delete;
 
   // mojom::NodeService implementation:
-  void Initialize(node::mojom::NodeServiceParamsPtr params) override;
+  void Initialize(node::mojom::NodeServiceParamsPtr params,
+                  mojo::PendingRemote<node::mojom::NodeServiceClient>
+                      client_pending_remote) override;
 
  private:
   // This needs to be initialized first so that it can be destroyed last

shell/services/node/public/mojom/node_service.mojom

@@ -20,7 +20,12 @@ struct NodeServiceParams {
   bool use_network_observer_from_url_loader_factory = false;
 };
 
+interface NodeServiceClient {
+  OnV8FatalError(string location, string report);
+};
+
 [ServiceSandbox=sandbox.mojom.Sandbox.kNoSandbox]
 interface NodeService {
-  Initialize(NodeServiceParams params);
+  Initialize(NodeServiceParams params,
+             pending_remote<NodeServiceClient> client_remote);
 };

spec/.gitignore

@@ -1,2 +1,3 @@
 node_modules
 artifacts
+**/native-addon/*/build

spec/api-utility-process-spec.ts

@@ -113,6 +113,21 @@ describe('utilityProcess module', () => {
       const [code] = await once(child, 'exit');
       expect(code).to.equal(exitCode);
     });
+
+    // 32-bit system will not have V8 Sandbox enabled.
+    // WoA testing does not have VS toolchain configured to build native addons.
+    ifit(process.arch !== 'ia32' && process.arch !== 'arm' && !isWindowsOnArm)('emits \'error\' when fatal error is triggered from V8', async () => {
+      const child = utilityProcess.fork(path.join(fixturesPath, 'external-ab-test.js'));
+      const [type, location, report] = await once(child, 'error');
+      const [code] = await once(child, 'exit');
+      expect(type).to.equal('FatalError');
+      expect(location).to.equal('v8_ArrayBuffer_NewBackingStore');
+      const reportJSON = JSON.parse(report);
+      expect(reportJSON.header.trigger).to.equal('v8_ArrayBuffer_NewBackingStore');
+      const addonPath = path.join(require.resolve('@electron-ci/external-ab'), '..', '..', 'build', 'Release', 'external_ab.node');
+      expect(reportJSON.sharedObjects).to.include(path.toNamespacedPath(addonPath));
+      expect(code).to.not.equal(0);
+    });
   });
 
   describe('app \'child-process-gone\' event', () => {

spec/fixtures/api/utility-process/external-ab-test.js

@@ -0,0 +1,3 @@
+'use strict';
+
+require('@electron-ci/external-ab');

spec/fixtures/native-addon/external-ab/binding.cc

@@ -0,0 +1,50 @@
+#include <node_api.h>
+#undef NAPI_VERSION
+#include <node_buffer.h>
+#include <v8.h>
+
+namespace {
+
+napi_value CreateBuffer(napi_env env, napi_callback_info info) {
+  v8::Isolate* isolate = v8::Isolate::TryGetCurrent();
+  if (isolate == nullptr) {
+    return NULL;
+  }
+
+  const size_t length = 4;
+
+  uint8_t* data = new uint8_t[length];
+  for (size_t i = 0; i < 4; i++) {
+    data[i] = static_cast<uint8_t>(length);
+  }
+
+  auto finalizer = [](char* data, void* hint) {
+    delete[] static_cast<uint8_t*>(reinterpret_cast<void*>(data));
+  };
+
+  // NOTE: Buffer API is invoked directly rather than
+  // napi version to trigger the FATAL error from V8.
+  v8::MaybeLocal<v8::Object> maybe = node::Buffer::New(
+      isolate, static_cast<char*>(reinterpret_cast<void*>(data)), length,
+      finalizer, nullptr);
+
+  return reinterpret_cast<napi_value>(*maybe.ToLocalChecked());
+}
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_status status;
+  napi_property_descriptor descriptors[] = {{"createBuffer", NULL, CreateBuffer,
+                                             NULL, NULL, NULL, napi_default,
+                                             NULL}};
+
+  status = napi_define_properties(
+      env, exports, sizeof(descriptors) / sizeof(*descriptors), descriptors);
+  if (status != napi_ok)
+    return NULL;
+
+  return exports;
+}
+
+}  // namespace
+
+NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

spec/fixtures/native-addon/external-ab/binding.gyp

@@ -0,0 +1,10 @@
+{
+  "targets": [
+    {
+      "target_name": "external_ab",
+      "sources": [
+        "binding.cc"
+      ]
+    }
+  ]
+}

spec/fixtures/native-addon/external-ab/lib/test-array-buffer.js

@@ -0,0 +1,4 @@
+'use strict';
+
+const binding = require('../build/Release/external_ab.node');
+binding.createBuffer();

spec/fixtures/native-addon/external-ab/package.json

@@ -0,0 +1,5 @@
+{
+  "main": "./lib/test-array-buffer.js",
+  "name": "@electron-ci/external-ab",
+  "version": "0.0.1"
+}

spec/package.json

@@ -10,6 +10,7 @@
     "@electron-ci/echo": "file:./fixtures/native-addon/echo",
     "@electron-ci/is-valid-window": "file:./is-valid-window",
     "@electron-ci/uv-dlopen": "file:./fixtures/native-addon/uv-dlopen/",
+    "@electron-ci/external-ab": "file:./fixtures/native-addon/external-ab/",
     "@electron/fuses": "^1.8.0",
     "@electron/packager": "^18.3.2",
     "@marshallofsound/mocha-appveyor-reporter": "^0.4.3",

spec/yarn.lock

@@ -5,6 +5,9 @@
 "@electron-ci/echo@file:./fixtures/native-addon/echo":
   version "0.0.1"
 
+"@electron-ci/external-ab@file:./fixtures/native-addon/external-ab":
+  version "0.0.1"
+
 "@electron-ci/is-valid-window@file:./is-valid-window":
   version "0.0.5"
   dependencies: