From 5836ea1a78e97b6f95ba859dba3402991c9b14f3 Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Sun, 29 Sep 2024 17:12:15 -0500 Subject: [PATCH] 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 * 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 Co-authored-by: Shelley Vohr --- docs/api/utility-process.md | 12 +++++ .../api/electron_api_utility_process.cc | 11 +++- .../api/electron_api_utility_process.h | 6 +++ shell/common/node_includes.h | 1 + shell/services/node/node_service.cc | 38 +++++++++++++- shell/services/node/node_service.h | 4 +- .../node/public/mojom/node_service.mojom | 7 ++- spec/.gitignore | 1 + spec/api-utility-process-spec.ts | 15 ++++++ .../api/utility-process/external-ab-test.js | 3 ++ .../native-addon/external-ab/binding.cc | 50 +++++++++++++++++++ .../native-addon/external-ab/binding.gyp | 10 ++++ .../external-ab/lib/test-array-buffer.js | 4 ++ .../native-addon/external-ab/package.json | 5 ++ spec/package.json | 1 + spec/yarn.lock | 3 ++ 16 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 spec/fixtures/api/utility-process/external-ab-test.js create mode 100644 spec/fixtures/native-addon/external-ab/binding.cc create mode 100644 spec/fixtures/native-addon/external-ab/binding.gyp create mode 100644 spec/fixtures/native-addon/external-ab/lib/test-array-buffer.js create mode 100644 spec/fixtures/native-addon/external-ab/package.json diff --git a/docs/api/utility-process.md b/docs/api/utility-process.md index 100c3ead6d19..af6c74967eb5 100644 --- a/docs/api/utility-process.md +++ b/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 diff --git a/shell/browser/api/electron_api_utility_process.cc b/shell/browser/api/electron_api_utility_process.cc index 333e4b66c514..ab813d7aba80 100644 --- a/shell/browser/api/electron_api_utility_process.cc +++ b/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::FromProcessId( base::ProcessId pid) { diff --git a/shell/browser/api/electron_api_utility_process.h b/shell/browser/api/electron_api_utility_process.h index 2e37157c85ae..c0a992ba7fe4 100644 --- a/shell/browser/api/electron_api_utility_process.h +++ b/shell/browser/api/electron_api_utility_process.h @@ -44,6 +44,7 @@ class UtilityProcessWrapper final public gin_helper::Pinnable, public gin_helper::EventEmitterMixin, 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 connector_; blink::MessagePortDescriptor host_port_; + mojo::Receiver receiver_{this}; mojo::Remote node_service_remote_; std::optional url_loader_network_observer_; diff --git a/shell/common/node_includes.h b/shell/common/node_includes.h index 26d3bb24d695..512a8d636a4c 100644 --- a/shell/common/node_includes.h +++ b/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" diff --git a/shell/services/node/node_service.cc b/shell/services/node/node_service.cc index 6d273119d02f..d1a11db429cf 100644 --- a/shell/services/node/node_service.cc +++ b/shell/services/node/node_service.cc @@ -4,6 +4,7 @@ #include "shell/services/node/node_service.h" +#include #include #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 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() /* 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 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. diff --git a/shell/services/node/node_service.h b/shell/services/node/node_service.h index b025ad233a26..d6e3edd092cd 100644 --- a/shell/services/node/node_service.h +++ b/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 + client_pending_remote) override; private: // This needs to be initialized first so that it can be destroyed last diff --git a/shell/services/node/public/mojom/node_service.mojom b/shell/services/node/public/mojom/node_service.mojom index be9a0cb0aba5..c2fce2660da0 100644 --- a/shell/services/node/public/mojom/node_service.mojom +++ b/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 client_remote); }; diff --git a/spec/.gitignore b/spec/.gitignore index 1e645859177f..a8e40c44c6c7 100644 --- a/spec/.gitignore +++ b/spec/.gitignore @@ -1,2 +1,3 @@ node_modules artifacts +**/native-addon/*/build diff --git a/spec/api-utility-process-spec.ts b/spec/api-utility-process-spec.ts index 76d0e2d6e30f..d283b41f4c5f 100644 --- a/spec/api-utility-process-spec.ts +++ b/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', () => { diff --git a/spec/fixtures/api/utility-process/external-ab-test.js b/spec/fixtures/api/utility-process/external-ab-test.js new file mode 100644 index 000000000000..16afd2d614cb --- /dev/null +++ b/spec/fixtures/api/utility-process/external-ab-test.js @@ -0,0 +1,3 @@ +'use strict'; + +require('@electron-ci/external-ab'); diff --git a/spec/fixtures/native-addon/external-ab/binding.cc b/spec/fixtures/native-addon/external-ab/binding.cc new file mode 100644 index 000000000000..277e13f97e35 --- /dev/null +++ b/spec/fixtures/native-addon/external-ab/binding.cc @@ -0,0 +1,50 @@ +#include +#undef NAPI_VERSION +#include +#include + +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(length); + } + + auto finalizer = [](char* data, void* hint) { + delete[] static_cast(reinterpret_cast(data)); + }; + + // NOTE: Buffer API is invoked directly rather than + // napi version to trigger the FATAL error from V8. + v8::MaybeLocal maybe = node::Buffer::New( + isolate, static_cast(reinterpret_cast(data)), length, + finalizer, nullptr); + + return reinterpret_cast(*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) diff --git a/spec/fixtures/native-addon/external-ab/binding.gyp b/spec/fixtures/native-addon/external-ab/binding.gyp new file mode 100644 index 000000000000..d8c4884bb9fe --- /dev/null +++ b/spec/fixtures/native-addon/external-ab/binding.gyp @@ -0,0 +1,10 @@ +{ + "targets": [ + { + "target_name": "external_ab", + "sources": [ + "binding.cc" + ] + } + ] +} diff --git a/spec/fixtures/native-addon/external-ab/lib/test-array-buffer.js b/spec/fixtures/native-addon/external-ab/lib/test-array-buffer.js new file mode 100644 index 000000000000..97b49e769337 --- /dev/null +++ b/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(); diff --git a/spec/fixtures/native-addon/external-ab/package.json b/spec/fixtures/native-addon/external-ab/package.json new file mode 100644 index 000000000000..1ab30a4dc32d --- /dev/null +++ b/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" +} diff --git a/spec/package.json b/spec/package.json index 8455bdb996e1..6277153850ab 100644 --- a/spec/package.json +++ b/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", diff --git a/spec/yarn.lock b/spec/yarn.lock index d9bf04094c9c..dacf56c6cbf5 100644 --- a/spec/yarn.lock +++ b/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: