feat: switch to crashpad on linux (electron#30278)

docs/api/crash-reporter.md

@@ -19,56 +19,22 @@ 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/)
 * [Sentry](https://docs.sentry.io/clients/electron)
 * [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.
-
-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).
+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.
 
-#### `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<String, String>` - 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

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`

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",

patches/webrtc/.patches

@@ -0,0 +1 @@
+add_thread_local_to_x_error_trap_cc.patch

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 <nornagon@nornagon.net>
+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);

shell/app/electron_main.cc

@@ -39,6 +39,8 @@
 #elif defined(OS_LINUX)  // defined(OS_WIN)
 #include <unistd.h>
 #include <cstdio>
+#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<const char**>(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);
 }
 

spec-main/api-crash-reporter-spec.ts

@@ -138,8 +138,11 @@ function waitForNewFileInDir (dir: string): Promise<string[]> {
 
 // 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 () => {