From 0107cab3d17ab964a61203a6307802cc9a6764e4 Mon Sep 17 00:00:00 2001 From: Robo Date: Thu, 30 Jul 2020 23:20:52 -0700 Subject: [PATCH] fix: save crash reports locally when uploadToServer: false on linux (#24778) (#24788) * fix: generate dumps under crashDumps folder in linux * Update spec-main/api-crash-reporter-spec.ts Co-authored-by: Jeremy Rose Co-authored-by: Jeremy Rose --- shell/app/electron_crash_reporter_client.cc | 12 ++++- shell/browser/electron_browser_client.cc | 4 +- spec-main/api-crash-reporter-spec.ts | 55 ++++++++++++++++----- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/shell/app/electron_crash_reporter_client.cc b/shell/app/electron_crash_reporter_client.cc index 5c211396e302d..47a803efcee75 100644 --- a/shell/app/electron_crash_reporter_client.cc +++ b/shell/app/electron_crash_reporter_client.cc @@ -11,6 +11,7 @@ #include "base/command_line.h" #include "base/environment.h" #include "base/files/file_path.h" +#include "base/files/file_util.h" #include "base/logging.h" #include "base/path_service.h" #include "base/strings/string_split.h" @@ -147,7 +148,14 @@ bool ElectronCrashReporterClient::GetCrashDumpLocation( #else bool ElectronCrashReporterClient::GetCrashDumpLocation( base::FilePath* crash_dir) { - return base::PathService::Get(electron::DIR_CRASH_DUMPS, crash_dir); + bool result = base::PathService::Get(electron::DIR_CRASH_DUMPS, crash_dir); + { + base::ThreadRestrictions::ScopedAllowIO allow_io; + if (result && !base::PathExists(*crash_dir)) { + return base::CreateDirectory(*crash_dir); + } + } + return result; } #endif @@ -159,7 +167,7 @@ bool ElectronCrashReporterClient::GetCrashMetricsLocation( #endif // OS_MACOSX || OS_LINUX bool ElectronCrashReporterClient::IsRunningUnattended() { - return false; + return !collect_stats_consent_; } bool ElectronCrashReporterClient::GetCollectStatsConsent() { diff --git a/shell/browser/electron_browser_client.cc b/shell/browser/electron_browser_client.cc index f378ec84567bc..f3b98b8e4c46d 100644 --- a/shell/browser/electron_browser_client.cc +++ b/shell/browser/electron_browser_client.cc @@ -60,6 +60,7 @@ #include "services/network/public/cpp/features.h" #include "services/network/public/cpp/resource_request_body.h" #include "services/service_manager/public/cpp/binder_map.h" +#include "shell/app/electron_crash_reporter_client.h" #include "shell/app/manifests.h" #include "shell/browser/api/electron_api_app.h" #include "shell/browser/api/electron_api_crash_reporter.h" @@ -284,8 +285,9 @@ breakpad::CrashHandlerHostLinux* CreateCrashHandlerHost( base::PathService::Get(electron::DIR_CRASH_DUMPS, &dumps_path); { ANNOTATE_SCOPED_MEMORY_LEAK; + bool upload = ElectronCrashReporterClient::Get()->GetCollectStatsConsent(); breakpad::CrashHandlerHostLinux* crash_handler = - new breakpad::CrashHandlerHostLinux(process_type, dumps_path, true); + new breakpad::CrashHandlerHostLinux(process_type, dumps_path, upload); crash_handler->StartUploaderThread(); return crash_handler; } diff --git a/spec-main/api-crash-reporter-spec.ts b/spec-main/api-crash-reporter-spec.ts index b2f621d8f871a..09dead4617b43 100644 --- a/spec-main/api-crash-reporter-spec.ts +++ b/spec-main/api-crash-reporter-spec.ts @@ -260,7 +260,7 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ require('electron').crashReporter.start({ submitURL: `http://127.0.0.1:${port}`, ignoreSystemCrashHandler: true, - extra: { 'longParam': 'a'.repeat(130) } + extra: { longParam: 'a'.repeat(130) } }); setTimeout(() => process.crash()); }, port); @@ -438,7 +438,7 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ await remotely(() => { require('electron').crashReporter.start({ submitURL: 'http://127.0.0.1', - extra: { 'extra1': 'hi' } + extra: { extra1: 'hi' } }); }); const parameters = await remotely(() => require('electron').crashReporter.getParameters()); @@ -471,8 +471,8 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ crashReporter.start({ submitURL: 'http://' }); const bw = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } }); bw.loadURL('about:blank'); - await bw.webContents.executeJavaScript(`require('electron').crashReporter.addExtraParameter('hello', 'world')`); - return bw.webContents.executeJavaScript(`require('electron').crashReporter.getParameters()`); + await bw.webContents.executeJavaScript('require(\'electron\').crashReporter.addExtraParameter(\'hello\', \'world\')'); + return bw.webContents.executeJavaScript('require(\'electron\').crashReporter.getParameters()'); }); if (process.platform === 'linux') { // On Linux, 'getParameters' will also include the global parameters, @@ -542,11 +542,10 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ } } - for (const crashingProcess of ['main', 'renderer', 'sandboxed-renderer', 'node']) { - // TODO(nornagon): breakpad on linux disables itself when uploadToServer - // is false, so we should figure out a different way to test the crash - // dump dir on linux. - ifdescribe(process.platform !== 'linux')(`when ${crashingProcess} crashes`, () => { + const processList = process.platform === 'linux' ? ['main', 'renderer', 'sandboxed-renderer'] + : ['main', 'renderer', 'sandboxed-renderer', 'node']; + for (const crashingProcess of processList) { + describe(`when ${crashingProcess} crashes`, () => { it('stores crashes in the crash dump directory when uploadToServer: false', async () => { const { remotely } = await startRemoteControlApp(); const crashesDir = await remotely(() => { @@ -554,12 +553,27 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ crashReporter.start({ submitURL: 'http://127.0.0.1', uploadToServer: false, ignoreSystemCrashHandler: true }); return crashReporter.getCrashesDirectory(); }); - const reportsDir = process.platform === 'darwin' ? path.join(crashesDir, 'completed') : path.join(crashesDir, 'reports'); + let reportsDir = crashesDir; + if (process.platform === 'darwin') { + reportsDir = path.join(crashesDir, 'completed'); + } else if (process.platform === 'win32') { + reportsDir = path.join(crashesDir, 'reports'); + } const newFileAppeared = waitForNewFileInDir(reportsDir); crash(crashingProcess, remotely); const newFiles = await newFileAppeared; expect(newFiles.length).to.be.greaterThan(0); - expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.dmp$/); + if (process.platform === 'linux') { + if (crashingProcess === 'main') { + expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{8}-[0-9a-f]{8}\.dmp$/); + } else { + const process = crashingProcess === 'sandboxed-renderer' ? 'renderer' : crashingProcess; + const regex = RegExp(`chromium-${process}-minidump-[0-9a-f]{16}.dmp`); + expect(newFiles[0]).to.match(regex); + } + } else { + expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.dmp$/); + } }); it('respects an overridden crash dump directory', async () => { @@ -573,12 +587,27 @@ ifdescribe(!isLinuxOnArm && !process.mas && !process.env.DISABLE_CRASH_REPORTER_ }, crashesDir); expect(remoteCrashesDir).to.equal(crashesDir); - const reportsDir = process.platform === 'darwin' ? path.join(crashesDir, 'completed') : path.join(crashesDir, 'reports'); + let reportsDir = crashesDir; + if (process.platform === 'darwin') { + reportsDir = path.join(crashesDir, 'completed'); + } else if (process.platform === 'win32') { + reportsDir = path.join(crashesDir, 'reports'); + } const newFileAppeared = waitForNewFileInDir(reportsDir); crash(crashingProcess, remotely); const newFiles = await newFileAppeared; expect(newFiles.length).to.be.greaterThan(0); - expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.dmp$/); + if (process.platform === 'linux') { + if (crashingProcess === 'main') { + expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{8}-[0-9a-f]{8}\.dmp$/); + } else { + const process = crashingProcess !== 'sandboxed-renderer' ? crashingProcess : 'renderer'; + const regex = RegExp(`chromium-${process}-minidump-[0-9a-f]{16}.dmp`); + expect(newFiles[0]).to.match(regex); + } + } else { + expect(newFiles[0]).to.match(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\.dmp$/); + } }); }); }