From 948cc08265a39a30ea903bdb51eadab9a23f7cef Mon Sep 17 00:00:00 2001 From: Shelley Vohr Date: Wed, 29 Jul 2020 19:04:24 -0700 Subject: [PATCH] chore: add V8 crash information to crashReporter (#24771) * feat: add V8 crash information to crashReporter * Address review feedback --- shell/common/api/electron_api_v8_util.cc | 11 ++++++++++ shell/common/node_bindings.cc | 21 ++++++++++++++++++ spec-main/api-crash-reporter-spec.ts | 28 ++++++++++++++++++++++++ typings/internal-ambient.d.ts | 1 + 4 files changed, 61 insertions(+) diff --git a/shell/common/api/electron_api_v8_util.cc b/shell/common/api/electron_api_v8_util.cc index b54ac924e08e..23dbada47338 100644 --- a/shell/common/api/electron_api_v8_util.cc +++ b/shell/common/api/electron_api_v8_util.cc @@ -128,6 +128,16 @@ std::vector> GetWeaklyTrackedValues(v8::Isolate* isolate) { } return locals; } + +// This causes a fatal error by creating a circular extension dependency. +void TriggerFatalErrorForTesting(v8::Isolate* isolate) { + static const char* aDeps[] = {"B"}; + v8::RegisterExtension(std::make_unique("A", "", 1, aDeps)); + static const char* bDeps[] = {"A"}; + v8::RegisterExtension(std::make_unique("B", "", 1, bDeps)); + v8::ExtensionConfiguration config(1, bDeps); + v8::Context::New(isolate, &config); +} #endif void Initialize(v8::Local exports, @@ -144,6 +154,7 @@ void Initialize(v8::Local exports, &RequestGarbageCollectionForTesting); dict.SetMethod("isSameOrigin", &IsSameOrigin); #ifdef DCHECK_IS_ON + dict.SetMethod("triggerFatalErrorForTesting", &TriggerFatalErrorForTesting); dict.SetMethod("getWeaklyTrackedValues", &GetWeaklyTrackedValues); dict.SetMethod("clearWeaklyTrackedValues", &ClearWeaklyTrackedValues); dict.SetMethod("weaklyTrackValue", &WeaklyTrackValue); diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 714da636b58a..ac77f9e11bde 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -24,6 +24,7 @@ #include "content/public/browser/browser_thread.h" #include "content/public/common/content_paths.h" #include "electron/buildflags/buildflags.h" +#include "shell/common/api/electron_bindings.h" #include "shell/common/electron_command_line.h" #include "shell/common/gin_converters/file_path_converter.h" #include "shell/common/gin_helper/dictionary.h" @@ -33,6 +34,10 @@ #include "shell/common/mac/main_application_bundle.h" #include "shell/common/node_includes.h" +#if !defined(MAS_BUILD) +#include "shell/common/crash_keys.h" +#endif + #define ELECTRON_BUILTIN_MODULES(V) \ V(electron_browser_app) \ V(electron_browser_auto_updater) \ @@ -132,6 +137,18 @@ bool IsPackagedApp() { #endif } +void V8FatalErrorCallback(const char* location, const char* message) { + LOG(ERROR) << "Fatal error in V8: " << location << " " << message; + +#if !defined(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; +} + // Initialize Node.js cli options to pass to Node.js // See https://nodejs.org/api/cli.html#cli_options void SetNodeCliFlags() { @@ -397,6 +414,10 @@ node::Environment* NodeBindings::CreateEnvironment( node::IsolateSettings is; + // Use a custom fatal error callback to allow us to add + // crash message and location to CrashReports. + is.fatal_error_callback = V8FatalErrorCallback; + if (browser_env_ == BrowserEnvironment::BROWSER) { // Node.js requires that microtask checkpoints be explicitly invoked. is.policy = v8::MicrotasksPolicy::kExplicit; diff --git a/spec-main/api-crash-reporter-spec.ts b/spec-main/api-crash-reporter-spec.ts index 7f56d53ed083..a890e336e730 100644 --- a/spec-main/api-crash-reporter-spec.ts +++ b/spec-main/api-crash-reporter-spec.ts @@ -28,6 +28,8 @@ type CrashInfo = { globalParam: 'globalValue' | undefined addedThenRemoved: 'to-be-removed' | undefined longParam: string | undefined + 'electron.v8-fatal.location': string | undefined + 'electron.v8-fatal.message': string | undefined } function checkCrash (expectedProcessType: string, fields: CrashInfo) { @@ -192,6 +194,32 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ expect(crash.rendererSpecific).to.equal('rs'); expect(crash.addedThenRemoved).to.be.undefined(); }); + + it('contains v8 crash keys when a v8 crash occurs', async () => { + const { remotely } = await startRemoteControlApp(); + const { port, waitForCrash } = await startServer(); + + await remotely((port: number) => { + require('electron').crashReporter.start({ + submitURL: `http://127.0.0.1:${port}`, + ignoreSystemCrashHandler: true + }); + }, [port]); + + remotely(() => { + const { BrowserWindow } = require('electron'); + const bw = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } }); + bw.loadURL('about:blank'); + bw.webContents.executeJavaScript('process._linkedBinding(\'electron_common_v8_util\').triggerFatalErrorForTesting()'); + }); + + const crash = await waitForCrash(); + expect(crash.prod).to.equal('Electron'); + expect(crash._productName).to.equal('remote-control'); + expect(crash.process_type).to.equal('renderer'); + expect(crash['electron.v8-fatal.location']).to.equal('v8::Context::New()'); + expect(crash['electron.v8-fatal.message']).to.equal('Circular extension dependency'); + }); }); }); diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index ed10f93bb216..58b76dbcc3d6 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -44,6 +44,7 @@ declare namespace NodeJS { clearWeaklyTrackedValues(): void; getWeaklyTrackedValues(): any[]; addRemoteObjectRef(contextId: string, id: number): void; + triggerFatalErrorForTesting(): void; } type AsarFileInfo = {