From 87a670f74dd7db4456230af16a893ce0577dd91e Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Mon, 18 May 2020 08:09:50 -0700 Subject: [PATCH] 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 --- docs/api/crash-reporter.md | 5 +- patches/chromium/.patches | 2 +- .../breakpad_disable_upload_compression.patch | 49 ------ ..._node_processes_as_browser_processes.patch | 4 +- ...allow_disabling_compression_on_linux.patch | 147 ++++++++++++++++++ 5 files changed, 152 insertions(+), 55 deletions(-) delete mode 100644 patches/chromium/breakpad_disable_upload_compression.patch create mode 100644 patches/chromium/crash_allow_disabling_compression_on_linux.patch diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 73b38782329b..3e3fcf4f1fe0 100644 --- a/docs/api/crash-reporter.md +++ b/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 (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 diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 46b0b202f256..6accd46a7fe8 100644 --- a/patches/chromium/.patches +++ b/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 diff --git a/patches/chromium/breakpad_disable_upload_compression.patch b/patches/chromium/breakpad_disable_upload_compression.patch deleted file mode 100644 index 9843f547d187..000000000000 --- a/patches/chromium/breakpad_disable_upload_compression.patch +++ /dev/null @@ -1,49 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: Jeremy Apthorp -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, diff --git a/patches/chromium/breakpad_treat_node_processes_as_browser_processes.patch b/patches/chromium/breakpad_treat_node_processes_as_browser_processes.patch index e5eef0452c55..185a9d905506 100644 --- a/patches/chromium/breakpad_treat_node_processes_as_browser_processes.patch +++ b/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 diff --git a/patches/chromium/crash_allow_disabling_compression_on_linux.patch b/patches/chromium/crash_allow_disabling_compression_on_linux.patch new file mode 100644 index 000000000000..05f0a271c59c --- /dev/null +++ b/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 +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(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(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(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(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) {