From 40e76dca0719ee7d725d29898ff995b7cb2781d1 Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Tue, 3 Aug 2021 14:01:12 -0700 Subject: [PATCH] feat: switch to crashpad on linux (#30278) --- docs/api/crash-reporter.md | 82 ++++++++----------- docs/breaking-changes.md | 15 ++++ patches/config.json | 2 + patches/webrtc/.patches | 1 + .../add_thread_local_to_x_error_trap_cc.patch | 24 ++++++ shell/app/electron_main.cc | 9 +- spec-main/api-crash-reporter-spec.ts | 7 +- 7 files changed, 89 insertions(+), 51 deletions(-) create mode 100644 patches/webrtc/.patches create mode 100644 patches/webrtc/add_thread_local_to_x_error_trap_cc.patch diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 8331bc8d43f..bf318eae954 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -19,6 +19,9 @@ following projects: * [socorro](https://github.com/mozilla/socorro) * [mini-breakpad-server](https://github.com/electron/mini-breakpad-server) +> **Note:** Electron uses Crashpad, not Breakpad, to collect and upload +> crashes, but for the time being, the [upload protocol is the same](https://chromium.googlesource.com/crashpad/crashpad/+/HEAD/doc/overview_design.md#Upload-to-collection-server). + Or use a 3rd party hosted solution: * [Backtrace](https://backtrace.io/electron/) @@ -26,49 +29,12 @@ Or use a 3rd party hosted solution: * [BugSplat](https://www.bugsplat.com/docs/platforms/electron) Crash reports are stored temporarily before being uploaded in a directory -underneath the app's user data directory (called 'Crashpad' on Windows and Mac, -or 'Crash Reports' on Linux). You can override this directory by calling -`app.setPath('crashDumps', '/path/to/crashes')` before starting the crash -reporter. +underneath the app's user data directory, called 'Crashpad'. You can override +this directory by calling `app.setPath('crashDumps', '/path/to/crashes')` +before starting the crash reporter. -On Windows and macOS, Electron uses -[crashpad](https://chromium.googlesource.com/crashpad/crashpad/+/master/README.md) -to monitor and report crashes. On Linux, Electron uses -[breakpad](https://chromium.googlesource.com/breakpad/breakpad/+/master/). This -is an implementation detail driven by Chromium, and it may change in future. In -particular, crashpad is newer and will likely eventually replace breakpad on -all platforms. - -### Note about Node child processes on Linux - -If you are using the Node.js `child_process` module and want to report crashes -from those processes on Linux, there is an extra step you will need to take to -properly initialize the crash reporter in the child process. This is not -necessary on Mac or Windows, as those platforms use Crashpad, which -automatically monitors child processes. - -Since `require('electron')` is not available in Node child processes, the -following APIs are available on the `process` object in Node child processes. -Note that, on Linux, each Node child process has its own separate instance of -the breakpad crash reporter. This is dissimilar to renderer child processes, -which have a "stub" breakpad reporter which returns information to the main -process for reporting. - -#### `process.crashReporter.start(options)` - -See [`crashReporter.start()`](#crashreporterstartoptions). - -#### `process.crashReporter.getParameters()` - -See [`crashReporter.getParameters()`](#crashreportergetparameters). - -#### `process.crashReporter.addExtraParameter(key, value)` - -See [`crashReporter.addExtraParameter(key, value)`](#crashreporteraddextraparameterkey-value). - -#### `process.crashReporter.removeExtraParameter(key)` - -See [`crashReporter.removeExtraParameter(key)`](#crashreporterremoveextraparameterkey). +Electron uses [crashpad](https://chromium.googlesource.com/crashpad/crashpad/+/refs/heads/main/README.md) +to monitor and report crashes. ## Methods @@ -186,12 +152,6 @@ names must be no longer than 39 bytes, and values must be no longer than 20320 bytes. Keys with names longer than the maximum will be silently ignored. Key values longer than the maximum length will be truncated. -**Note:** On linux values that are longer than 127 bytes will be chunked into -multiple keys, each 127 bytes in length. E.g. `addExtraParameter('foo', 'a'.repeat(130))` -will result in two chunked keys `foo__1` and `foo__2`, the first will contain -the first 127 bytes and the second will contain the remaining 3 bytes. On -your crash reporting backend you should stitch together keys in this format. - ### `crashReporter.removeExtraParameter(key)` * `key` String - Parameter key, must be no longer than 39 bytes. @@ -203,6 +163,32 @@ will not include this parameter. Returns `Record` - The current 'extra' parameters of the crash reporter. +## In Node child processes + +Since `require('electron')` is not available in Node child processes, the +following APIs are available on the `process` object in Node child processes. + +#### `process.crashReporter.start(options)` + +See [`crashReporter.start()`](#crashreporterstartoptions). + +Note that if the crash reporter is started in the main process, it will +automatically monitor child processes, so it should not be started in the child +process. Only use this method if the main process does not initialize the crash +reporter. + +#### `process.crashReporter.getParameters()` + +See [`crashReporter.getParameters()`](#crashreportergetparameters). + +#### `process.crashReporter.addExtraParameter(key, value)` + +See [`crashReporter.addExtraParameter(key, value)`](#crashreporteraddextraparameterkey-value). + +#### `process.crashReporter.removeExtraParameter(key)` + +See [`crashReporter.removeExtraParameter(key)`](#crashreporterremoveextraparameterkey). + ## Crash Report Payload The crash reporter will send the following data to the `submitURL` as diff --git a/docs/breaking-changes.md b/docs/breaking-changes.md index de11e1eb4a4..03419e09ff8 100644 --- a/docs/breaking-changes.md +++ b/docs/breaking-changes.md @@ -12,6 +12,21 @@ This document uses the following convention to categorize breaking changes: * **Deprecated:** An API was marked as deprecated. The API will continue to function, but will emit a deprecation warning, and will be removed in a future release. * **Removed:** An API or feature was removed, and is no longer supported by Electron. +## Planned Breaking API Changes (15.0) + +### Behavior Changed: `crashReporter` implementation switched to Crashpad on Linux + +The underlying implementation of the `crashReporter` API on Linux has changed +from Breakpad to Crashpad, bringing it in line with Windows and Mac. As a +result of this, child processes are now automatically monitored, and calling +`process.crashReporter.start` in Node child processes is no longer needed (and +is not advisable, as it will start a second instance of the Crashpad reporter). + +There are also some subtle changes to how annotations will be reported on +Linux, including that long values will no longer be split between annotations +appended with `__1`, `__2` and so on, and instead will be truncated at the +(new, longer) annotation value limit. + ## Planned Breaking API Changes (14.0) ### Removed: `app.allowRendererProcessReuse` diff --git a/patches/config.json b/patches/config.json index b0156f7d1c0..e9a2297ba10 100644 --- a/patches/config.json +++ b/patches/config.json @@ -3,6 +3,8 @@ "src/electron/patches/boringssl": "src/third_party/boringssl/src", + "src/electron/patches/webrtc": "src/third_party/webrtc", + "src/electron/patches/v8": "src/v8", "src/electron/patches/node": "src/third_party/electron_node", diff --git a/patches/webrtc/.patches b/patches/webrtc/.patches new file mode 100644 index 00000000000..9cde1db8895 --- /dev/null +++ b/patches/webrtc/.patches @@ -0,0 +1 @@ +add_thread_local_to_x_error_trap_cc.patch diff --git a/patches/webrtc/add_thread_local_to_x_error_trap_cc.patch b/patches/webrtc/add_thread_local_to_x_error_trap_cc.patch new file mode 100644 index 00000000000..0be67e3ccaf --- /dev/null +++ b/patches/webrtc/add_thread_local_to_x_error_trap_cc.patch @@ -0,0 +1,24 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Jeremy Rose +Date: Tue, 27 Jul 2021 10:32:54 -0700 +Subject: add thread_local to x_error_trap.cc + +Per https://bugs.chromium.org/p/chromium/issues/detail?id=781618#c6. + +To fix this DCHECK firing: https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/modules/desktop_capture/linux/x_error_trap.cc;l=35;drc=25ab3228f3e473f2226f219531ec617d2daa175e + +diff --git a/modules/desktop_capture/linux/x_error_trap.cc b/modules/desktop_capture/linux/x_error_trap.cc +index 13233d827470d9d42be0333c3080e3d107f86fd5..62efb5b5b5194fc8961a27fe2a1efcd77e385d08 100644 +--- a/modules/desktop_capture/linux/x_error_trap.cc ++++ b/modules/desktop_capture/linux/x_error_trap.cc +@@ -19,8 +19,8 @@ namespace webrtc { + namespace { + + // TODO(sergeyu): This code is not thread safe. Fix it. Bug 2202. +-static bool g_xserver_error_trap_enabled = false; +-static int g_last_xserver_error_code = 0; ++static thread_local bool g_xserver_error_trap_enabled = false; ++static thread_local int g_last_xserver_error_code = 0; + + int XServerErrorHandler(Display* display, XErrorEvent* error_event) { + RTC_DCHECK(g_xserver_error_trap_enabled); diff --git a/shell/app/electron_main.cc b/shell/app/electron_main.cc index ff54fa9262b..5dfd0289911 100644 --- a/shell/app/electron_main.cc +++ b/shell/app/electron_main.cc @@ -39,6 +39,8 @@ #elif defined(OS_LINUX) // defined(OS_WIN) #include #include +#include "base/base_switches.h" +#include "base/command_line.h" #include "content/public/app/content_main.h" #include "shell/app/electron_main_delegate.h" // NOLINT #else // defined(OS_LINUX) @@ -304,9 +306,14 @@ int main(int argc, char* argv[]) { electron::ElectronMainDelegate delegate; content::ContentMainParams params(&delegate); + electron::ElectronCommandLine::Init(argc, argv); params.argc = argc; params.argv = const_cast(argv); - electron::ElectronCommandLine::Init(argc, argv); + base::CommandLine::Init(params.argc, params.argv); + // TODO(https://crbug.com/1176772): Remove when Chrome Linux is fully migrated + // to Crashpad. + base::CommandLine::ForCurrentProcess()->AppendSwitch( + ::switches::kEnableCrashpad); return content::ContentMain(params); } diff --git a/spec-main/api-crash-reporter-spec.ts b/spec-main/api-crash-reporter-spec.ts index 8f8f6af7dd0..ab88656831b 100644 --- a/spec-main/api-crash-reporter-spec.ts +++ b/spec-main/api-crash-reporter-spec.ts @@ -138,8 +138,11 @@ function waitForNewFileInDir (dir: string): Promise { // TODO(nornagon): Fix tests on linux/arm. ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_TESTS)('crashReporter module', function () { - for (const withLinuxCrashpad of (process.platform === 'linux' ? [false, true] : [false])) { - const crashpadExtraArgs = withLinuxCrashpad ? ['--enable-crashpad'] : []; + // TODO(nornagon): remove linux/breakpad tests once breakpad support is fully + // removed. + for (const enableLinuxCrashpad of (process.platform === 'linux' ? [false] : [false])) { + const withLinuxCrashpad = enableLinuxCrashpad || (process.platform === 'linux'); + const crashpadExtraArgs = enableLinuxCrashpad ? ['--enable-crashpad'] : []; describe(withLinuxCrashpad ? '(with crashpad)' : '', () => { describe('should send minidump', () => { it('when renderer crashes', async () => {