Browse Source

feat: allow compressing crash uploads on linux (#23597)

* chore: align crash patch with upstream

* feat: allow compressing crash uploads on linux

* update patches

Co-authored-by: Electron Bot <[email protected]>
Jeremy Apthorp 4 years ago
parent
commit
87a670f74d

+ 2 - 3
docs/api/crash-reporter.md

@@ -58,9 +58,8 @@ The `crashReporter` module has the following methods:
     Default is `false`.
   * `rateLimit` Boolean (optional) _macOS_ _Windows_ - If true, limit the
     number of crashes uploaded to 1/hour. Default is `false`.
-  * `compress` Boolean (optional) _macOS_ _Windows_ - If true, crash reports
-    will be compressed and uploaded with `Content-Encoding: gzip`. Not all
-    collection servers support compressed payloads. Default is `false`.
+  * `compress` Boolean (optional) - If true, crash reports will be compressed
+    and uploaded with `Content-Encoding: gzip`. Default is `false`.
   * `extra` Record<String, String> (optional) - Extra string key/value
     annotations that will be sent along with crash reports that are generated
     in the main process. Only string values are supported. Crashes generated in

+ 1 - 1
patches/chromium/.patches

@@ -91,7 +91,7 @@ web_contents.patch
 ui_gtk_public_header.patch
 crash_allow_embedder_to_set_crash_upload_url.patch
 crash_allow_setting_more_options.patch
-breakpad_disable_upload_compression.patch
 breakpad_treat_node_processes_as_browser_processes.patch
 upload_list_add_loadsync_method.patch
 breakpad_allow_getting_string_values_for_crash_keys.patch
+crash_allow_disabling_compression_on_linux.patch

+ 0 - 49
patches/chromium/breakpad_disable_upload_compression.patch

@@ -1,49 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Jeremy Apthorp <[email protected]>
-Date: Wed, 29 Apr 2020 16:28:35 -0700
-Subject: breakpad: disable upload compression
-
-Our prior implementation of breakpad uploading did not compress files on
-upload. In order to maintain that behavior, this disables compression in
-//components/crash.
-
-Ideally, this would be made configurable.
-
-diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
-index bb0c6aebb4fdb9b24de8292a3f1c8dc77f21e051..60a850ba3c819538f8fbf6cdcbe82290d73f1127 100644
---- a/components/crash/core/app/breakpad_linux.cc
-+++ b/components/crash/core/app/breakpad_linux.cc
-@@ -1330,6 +1330,7 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
- 
- #else  // defined(OS_CHROMEOS)
- 
-+  /*
-   // Compress |dumpfile| with gzip.
-   const pid_t gzip_child = sys_fork();
-   if (gzip_child < 0) {
-@@ -1373,13 +1374,16 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
-     WriteLog(msg, sizeof(msg) - 1);
-     sys__exit(1);
-   }
-+  */
- 
-   // The --header argument to wget looks like:
-   //   --header=Content-Encoding: gzip
-   //   --header=Content-Type: multipart/form-data; boundary=XYZ
-   // where the boundary has two fewer leading '-' chars
-+  /*
-   static const char header_content_encoding[] =
-       "--header=Content-Encoding: gzip";
-+  */
-   static const char header_msg[] =
-       "--header=Content-Type: multipart/form-data; boundary=";
-   const size_t header_content_type_size =
-@@ -1406,7 +1410,7 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
-   static const char kWgetBinary[] = "/usr/bin/wget";
-   const char* args[] = {
-       kWgetBinary,
--      header_content_encoding,
-+      //header_content_encoding,
-       header_content_type,
-       post_file,
-       g_upload_url,

+ 2 - 2
patches/chromium/breakpad_treat_node_processes_as_browser_processes.patch

@@ -10,7 +10,7 @@ breakpad independently, as a "browser" process. This patches
 crash annotation.
 
 diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
-index 60a850ba3c819538f8fbf6cdcbe82290d73f1127..c5b535cb42e586a7601293c76e18355ab96eef65 100644
+index bb0c6aebb4fdb9b24de8292a3f1c8dc77f21e051..4bd9bfebb4e5119a5cdc48bb322745cfd9f98c02 100644
 --- a/components/crash/core/app/breakpad_linux.cc
 +++ b/components/crash/core/app/breakpad_linux.cc
 @@ -718,8 +718,13 @@ bool CrashDone(const MinidumpDescriptor& minidump,
@@ -29,7 +29,7 @@ index 60a850ba3c819538f8fbf6cdcbe82290d73f1127..c5b535cb42e586a7601293c76e18355a
    info.distro = base::g_linux_distro;
    info.distro_length = my_strlen(base::g_linux_distro);
    info.upload = upload;
-@@ -2044,8 +2049,13 @@ void InitCrashReporter(const std::string& process_type) {
+@@ -2040,8 +2045,13 @@ void InitCrashReporter(const std::string& process_type) {
        process_type == kWebViewSingleProcessType ||
        process_type == kBrowserProcessType ||
  #endif

+ 147 - 0
patches/chromium/crash_allow_disabling_compression_on_linux.patch

@@ -0,0 +1,147 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Jeremy Apthorp <[email protected]>
+Date: Thu, 14 May 2020 16:52:09 -0700
+Subject: crash: allow disabling compression on linux
+
+This makes compression optional on breakpad_linux.
+
+Upstream attempted here
+https://chromium-review.googlesource.com/c/chromium/src/+/2198641, but
+was denied.
+
+Ultimately we should remove the option to disable compression, and
+subsequently remove this patch.
+
+diff --git a/components/crash/core/app/breakpad_linux.cc b/components/crash/core/app/breakpad_linux.cc
+index 4bd9bfebb4e5119a5cdc48bb322745cfd9f98c02..a897622de0ea7a9e79955d7b80dfeafe3ec6fc94 100644
+--- a/components/crash/core/app/breakpad_linux.cc
++++ b/components/crash/core/app/breakpad_linux.cc
+@@ -108,6 +108,8 @@ void SetUploadURL(const std::string& url) {
+   DCHECK(!g_upload_url);
+   g_upload_url = strdup(url.c_str());
+ }
++
++bool g_compress_uploads = true;
+ #endif
+ 
+ bool g_is_node = false;
+@@ -1335,56 +1337,60 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
+ 
+ #else  // defined(OS_CHROMEOS)
+ 
+-  // Compress |dumpfile| with gzip.
+-  const pid_t gzip_child = sys_fork();
+-  if (gzip_child < 0) {
+-    static const char msg[] = "sys_fork() for gzip process failed.\n";
+-    WriteLog(msg, sizeof(msg) - 1);
+-    sys__exit(1);
+-  }
+-  if (!gzip_child) {
+-    // gzip process.
+-    const char* args[] = {
+-      "/bin/gzip",
+-      "-f",  // Do not prompt to verify before overwriting.
+-      dumpfile,
+-      nullptr,
+-    };
+-    execve(args[0], const_cast<char**>(args), environ);
+-    static const char msg[] = "Cannot exec gzip.\n";
+-    WriteLog(msg, sizeof(msg) - 1);
+-    sys__exit(1);
+-  }
+-  // Wait for gzip process.
+-  int status = 0;
+-  if (sys_waitpid(gzip_child, &status, 0) != gzip_child ||
+-      !WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+-    static const char msg[] = "sys_waitpid() for gzip process failed.\n";
+-    WriteLog(msg, sizeof(msg) - 1);
+-    sys_kill(gzip_child, SIGKILL);
+-    sys__exit(1);
+-  }
++  if (g_compress_uploads) {
++    // Compress |dumpfile| with gzip.
++    const pid_t gzip_child = sys_fork();
++    if (gzip_child < 0) {
++      static const char msg[] = "sys_fork() for gzip process failed.\n";
++      WriteLog(msg, sizeof(msg) - 1);
++      sys__exit(1);
++    }
++    if (!gzip_child) {
++      // gzip process.
++      const char* args[] = {
++          "/bin/gzip",
++          "-f",  // Do not prompt to verify before overwriting.
++          dumpfile,
++          nullptr,
++      };
++      execve(args[0], const_cast<char**>(args), environ);
++      static const char msg[] = "Cannot exec gzip.\n";
++      WriteLog(msg, sizeof(msg) - 1);
++      sys__exit(1);
++    }
++    // Wait for gzip process.
++    int status = 0;
++    if (sys_waitpid(gzip_child, &status, 0) != gzip_child ||
++        !WIFEXITED(status) || WEXITSTATUS(status) != 0) {
++      static const char msg[] = "sys_waitpid() for gzip process failed.\n";
++      WriteLog(msg, sizeof(msg) - 1);
++      sys_kill(gzip_child, SIGKILL);
++      sys__exit(1);
++    }
+ 
+-  static const char kGzipExtension[] = ".gz";
+-  const size_t gzip_file_size = my_strlen(dumpfile) + sizeof(kGzipExtension);
+-  char* const gzip_file = reinterpret_cast<char*>(allocator->Alloc(
+-      gzip_file_size));
+-  my_strlcpy(gzip_file, dumpfile, gzip_file_size);
+-  my_strlcat(gzip_file, kGzipExtension, gzip_file_size);
++    static const char kGzipExtension[] = ".gz";
++    const size_t gzip_file_size = my_strlen(dumpfile) + sizeof(kGzipExtension);
++    char* const gzip_file =
++        reinterpret_cast<char*>(allocator->Alloc(gzip_file_size));
++    my_strlcpy(gzip_file, dumpfile, gzip_file_size);
++    my_strlcat(gzip_file, kGzipExtension, gzip_file_size);
+ 
+-  // Rename |gzip_file| to |dumpfile| (the original file was deleted by gzip).
+-  if (rename(gzip_file, dumpfile)) {
+-    static const char msg[] = "Failed to rename gzipped file.\n";
+-    WriteLog(msg, sizeof(msg) - 1);
+-    sys__exit(1);
++    // Rename |gzip_file| to |dumpfile| (the original file was deleted by gzip).
++    if (rename(gzip_file, dumpfile)) {
++      static const char msg[] = "Failed to rename gzipped file.\n";
++      WriteLog(msg, sizeof(msg) - 1);
++      sys__exit(1);
++    }
+   }
+ 
+   // The --header argument to wget looks like:
+   //   --header=Content-Encoding: gzip
+   //   --header=Content-Type: multipart/form-data; boundary=XYZ
+   // where the boundary has two fewer leading '-' chars
+-  static const char header_content_encoding[] =
++  static const char header_content_encoding_gzip[] =
+       "--header=Content-Encoding: gzip";
++  static const char header_content_encoding_identity[] =
++      "--header=Content-Encoding: identity";
+   static const char header_msg[] =
+       "--header=Content-Type: multipart/form-data; boundary=";
+   const size_t header_content_type_size =
+@@ -1411,7 +1417,8 @@ void ExecUploadProcessOrTerminate(const BreakpadInfo& info,
+   static const char kWgetBinary[] = "/usr/bin/wget";
+   const char* args[] = {
+       kWgetBinary,
+-      header_content_encoding,
++      g_compress_uploads ? header_content_encoding_gzip
++                         : header_content_encoding_identity,
+       header_content_type,
+       post_file,
+       g_upload_url,
+@@ -2054,6 +2061,7 @@ void InitCrashReporter(const std::string& process_type) {
+ 
+ #if !defined(OS_CHROMEOS)
+   SetUploadURL(GetCrashReporterClient()->GetUploadUrl());
++  g_compress_uploads = GetCrashReporterClient()->GetShouldCompressUploads();
+ #endif
+ 
+   if (is_browser_process) {