From ac0658bbf18a5cbcb292f582366515f98812887f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 13:31:16 -0700 Subject: [PATCH 01/23] Use PathService for temp dir path for crashes --- atom/common/crash_reporter/crash_reporter.cc | 25 ++++++++++++++++--- atom/common/crash_reporter/crash_reporter.h | 7 +++++- .../crash_reporter/crash_reporter_linux.cc | 8 ++++-- .../crash_reporter/crash_reporter_mac.mm | 18 ++++++++----- .../crash_reporter/crash_reporter_win.cc | 2 +- lib/common/api/crash-reporter.js | 5 +--- 6 files changed, 48 insertions(+), 17 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 57bd5ef1890d..b7f7eaced266 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -8,6 +8,7 @@ #include "atom/common/atom_version.h" #include "base/command_line.h" #include "base/files/file_util.h" +#include "base/path_service.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "content/public/common/content_switches.h" @@ -34,6 +35,19 @@ void CrashReporter::Start(const std::string& product_name, auto_submit, skip_system_crash_handler); } +bool CrashReporter::GetTempDirectory(base::FilePath* path) { + return PathService::Get(base::DIR_TEMP, path); +} + +bool CrashReporter::GetCrashesDirectory( + const std::string& product_name, base::FilePath* path) { + if (GetTempDirectory(path)) { + *path = path->Append(product_name + " Crashes"); + return true; + } + return false; +} + void CrashReporter::SetUploadParameters(const StringMap& parameters) { upload_parameters_ = parameters; upload_parameters_["process_type"] = is_browser_ ? "browser" : "renderer"; @@ -43,10 +57,15 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) { } std::vector -CrashReporter::GetUploadedReports(const std::string& path) { - std::string file_content; +CrashReporter::GetUploadedReports(const std::string& product_name) { std::vector result; - if (base::ReadFileToString(base::FilePath::FromUTF8Unsafe(path), + + base::FilePath crashes_dir; + if (!GetCrashesDirectory(product_name, &crashes_dir)) + return result; + + std::string file_content; + if (base::ReadFileToString(crashes_dir.Append("uploads.log"), &file_content)) { std::vector reports = base::SplitString( file_content, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index eebbe16dca82..5f59468e78a4 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -10,6 +10,7 @@ #include #include +#include "base/files/file_path.h" #include "base/macros.h" namespace crash_reporter { @@ -29,7 +30,7 @@ class CrashReporter { const StringMap& extra_parameters); virtual std::vector GetUploadedReports( - const std::string& path); + const std::string& product_name); protected: CrashReporter(); @@ -43,6 +44,10 @@ class CrashReporter { bool skip_system_crash_handler); virtual void SetUploadParameters(); + bool GetTempDirectory(base::FilePath* path); + bool GetCrashesDirectory(const std::string& product_name, + base::FilePath* path); + StringMap upload_parameters_; bool is_browser_; diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index c25f00582ffb..faaa5f4806a9 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -78,8 +78,12 @@ void CrashReporterLinux::SetUploadParameters() { } void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) { - std::string dump_dir = "/tmp/" + product_name + " Crashes"; - base::FilePath dumps_path(dump_dir); + base::FilePath dumps_path; + if (!GetCrashesDirectory(&dumps_path)) { + LOG(ERROR) << "Cannot get temp directory"; + return; + } + base::CreateDirectory(dumps_path); std::string log_file = base::StringPrintf( diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index ee2ce036048f..e5bfc434fb7a 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -33,13 +33,17 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, const std::string& submit_url, bool auto_submit, bool skip_system_crash_handler) { - // check whether crashpad has been initilized. + // check whether crashpad has been initialized. // Only need to initilize once. if (simple_string_dictionary_) return; - std::string dump_dir = "/tmp/" + product_name + " Crashes"; - base::FilePath database_path(dump_dir); + base::FilePath database_path; + if (!GetCrashesDirectory(product_name, &database_path)) { + LOG(ERROR) << "Cannot get temp directory"; + return; + } + if (is_browser_) { @autoreleasepool { base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath(); @@ -93,13 +97,15 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, } std::vector -CrashReporterMac::GetUploadedReports(const std::string& path) { +CrashReporterMac::GetUploadedReports(const std::string& product_name) { std::vector uploaded_reports; - base::FilePath file_path(path); - if (!base::PathExists(file_path)) { + base::FilePath file_path; + if (!GetCrashesDirectory(product_name, &file_path) || + !base::PathExists(file_path)) { return uploaded_reports; } + // Load crashpad database. std::unique_ptr database = crashpad::CrashReportDatabase::Initialize(file_path); diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 3261f9c9c6a3..4e3d2efe949a 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -154,7 +154,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, skip_system_crash_handler_ = skip_system_crash_handler; base::FilePath temp_dir; - if (!base::GetTempDir(&temp_dir)) { + if (!GetTempDirectory(&temp_dir)) { LOG(ERROR) << "Cannot get temp directory"; return; } diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index 13d7a07ad9cf..2ca645fc1053 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -76,10 +76,7 @@ var CrashReporter = (function () { } CrashReporter.prototype.getUploadedReports = function () { - var log, tmpdir - tmpdir = process.platform === 'win32' ? os.tmpdir() : '/tmp' - log = process.platform === 'darwin' ? path.join(tmpdir, this.productName + ' Crashes') : path.join(tmpdir, this.productName + ' Crashes', 'uploads.log') - return binding._getUploadedReports(log) + return binding._getUploadedReports(this.productName) } return CrashReporter From f282b51c98a9a3170f6a6fd38603b14ed0cce0cc Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 13:40:39 -0700 Subject: [PATCH 02/23] :art: Use ES6 class, destructuring, and let/const --- lib/common/api/crash-reporter.js | 42 ++++++++++++-------------------- spec/api-crash-reporter-spec.js | 6 ++--- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index 2ca645fc1053..fd7e30b702e6 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -1,27 +1,18 @@ 'use strict' -const os = require('os') -const path = require('path') -const spawn = require('child_process').spawn +const {spawn} = require('child_process') const electron = require('electron') const binding = process.atomBinding('crash_reporter') -var CrashReporter = (function () { - function CrashReporter () {} - - CrashReporter.prototype.start = function (options) { - var app, args, autoSubmit, companyName, env, extra, ignoreSystemCrashHandler, start, submitURL +class CrashReporter { + start (options) { if (options == null) { options = {} } this.productName = options.productName - companyName = options.companyName - submitURL = options.submitURL - autoSubmit = options.autoSubmit - ignoreSystemCrashHandler = options.ignoreSystemCrashHandler - extra = options.extra + let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options - app = (process.type === 'browser' ? electron : electron.remote).app + const app = (process.type === 'browser' ? electron : electron.remote).app if (this.productName == null) { this.productName = app.getName() } @@ -43,18 +34,17 @@ var CrashReporter = (function () { if (extra._version == null) { extra._version = app.getVersion() } + if (companyName == null) { throw new Error('companyName is a required option to crashReporter.start') } if (submitURL == null) { throw new Error('submitURL is a required option to crashReporter.start') } - start = () => { - binding.start(this.productName, companyName, submitURL, autoSubmit, ignoreSystemCrashHandler, extra) - } + if (process.platform === 'win32') { - args = ['--reporter-url=' + submitURL, '--application-name=' + this.productName, '--v=1'] - env = { + const args = ['--reporter-url=' + submitURL, '--application-name=' + this.productName, '--v=1'] + const env = { ELECTRON_INTERNAL_CRASH_SERVICE: 1 } spawn(process.execPath, args, { @@ -62,12 +52,12 @@ var CrashReporter = (function () { detached: true }) } - return start() + + binding.start(this.productName, companyName, submitURL, autoSubmit, ignoreSystemCrashHandler, extra) } - CrashReporter.prototype.getLastCrashReport = function () { - var reports - reports = this.getUploadedReports() + getLastCrashReport () { + const reports = this.getUploadedReports() if (reports.length > 0) { return reports[0] } else { @@ -75,11 +65,9 @@ var CrashReporter = (function () { } } - CrashReporter.prototype.getUploadedReports = function () { + getUploadedReports () { return binding._getUploadedReports(this.productName) } - - return CrashReporter -})() +} module.exports = new CrashReporter() diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 286b7d2f6368..1c8e9c2c76d3 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -5,10 +5,8 @@ const path = require('path') const url = require('url') const {closeWindow} = require('./window-helpers') -const remote = require('electron').remote -const app = remote.require('electron').app -const crashReporter = remote.require('electron').crashReporter -const BrowserWindow = remote.require('electron').BrowserWindow +const {remote} = require('electron') +const {app, BrowserWindow, crashReporter} = remote.require('electron') describe('crashReporter module', function () { var fixtures = path.resolve(__dirname, 'fixtures') From 1afe501a36ff5ef9797a4c65a697b19373c5dde5 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 13:53:13 -0700 Subject: [PATCH 03/23] Add tests for crash reports in custom temp directory --- spec/api-crash-reporter-spec.js | 10 ++++++++++ spec/package.json | 3 ++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 1c8e9c2c76d3..a501c1133421 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -1,7 +1,10 @@ const assert = require('assert') +const fs = require('fs') const http = require('http') const multiparty = require('multiparty') const path = require('path') +const rimraf = require('rimraf') +const temp = require('temp').track() const url = require('url') const {closeWindow} = require('./window-helpers') @@ -11,14 +14,20 @@ const {app, BrowserWindow, crashReporter} = remote.require('electron') describe('crashReporter module', function () { var fixtures = path.resolve(__dirname, 'fixtures') var w = null + var originalTempDirectory = null + var tempDirectory = null beforeEach(function () { w = new BrowserWindow({ show: false }) + tempDirectory = temp.mkdirSync('electronCrashReporterSpec-') + originalTempDirectory = app.getPath('temp') + app.setPath('temp', tempDirectory) }) afterEach(function () { + app.setPath('temp', originalTempDirectory) return closeWindow(w).then(function () { w = null }) }) @@ -56,6 +65,7 @@ describe('crashReporter module', function () { assert.equal(crashReporter.getLastCrashReport().id, 'abc-123-def') assert.notEqual(crashReporter.getUploadedReports().length, 0) assert.equal(crashReporter.getUploadedReports()[0].id, 'abc-123-def') + assert.equal(fs.existsSync(tempDirectory, 'Zombies Crahses'), true) done() }) }) diff --git a/spec/package.json b/spec/package.json index 32b41b55c74c..99692ffb23a4 100644 --- a/spec/package.json +++ b/spec/package.json @@ -6,10 +6,11 @@ "devDependencies": { "basic-auth": "^1.0.0", "graceful-fs": "3.0.5", - "mocha": "2.1.0", "mkdirp": "0.5.1", + "mocha": "2.1.0", "multiparty": "4.1.2", "q": "0.9.7", + "rimraf": "^2.5.4", "temp": "0.8.1", "walkdir": "0.0.7", "ws": "0.7.2", From 81733a523ecd0f678e99d90aa062a618c9bef2cb Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 14:00:27 -0700 Subject: [PATCH 04/23] DRY up temp directory logging --- atom/common/crash_reporter/crash_reporter.cc | 5 ++++- atom/common/crash_reporter/crash_reporter_linux.cc | 4 +--- atom/common/crash_reporter/crash_reporter_mac.mm | 6 ++---- atom/common/crash_reporter/crash_reporter_win.cc | 4 +--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index b7f7eaced266..7a962daf95ef 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -36,7 +36,10 @@ void CrashReporter::Start(const std::string& product_name, } bool CrashReporter::GetTempDirectory(base::FilePath* path) { - return PathService::Get(base::DIR_TEMP, path); + bool success = PathService::Get(base::DIR_TEMP, path); + if (!success) + LOG(ERROR) << "Cannot get temp directory"; + return success; } bool CrashReporter::GetCrashesDirectory( diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index faaa5f4806a9..4913e4b3d228 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -79,10 +79,8 @@ void CrashReporterLinux::SetUploadParameters() { void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) { base::FilePath dumps_path; - if (!GetCrashesDirectory(&dumps_path)) { - LOG(ERROR) << "Cannot get temp directory"; + if (!GetCrashesDirectory(&dumps_path)) return; - } base::CreateDirectory(dumps_path); diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index e5bfc434fb7a..99e8846e3a0c 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -34,15 +34,13 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, bool auto_submit, bool skip_system_crash_handler) { // check whether crashpad has been initialized. - // Only need to initilize once. + // Only need to initialize once. if (simple_string_dictionary_) return; base::FilePath database_path; - if (!GetCrashesDirectory(product_name, &database_path)) { - LOG(ERROR) << "Cannot get temp directory"; + if (!GetCrashesDirectory(product_name, &database_path)) return; - } if (is_browser_) { @autoreleasepool { diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 4e3d2efe949a..27af3e1abb96 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -154,10 +154,8 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, skip_system_crash_handler_ = skip_system_crash_handler; base::FilePath temp_dir; - if (!GetTempDirectory(&temp_dir)) { - LOG(ERROR) << "Cannot get temp directory"; + if (!GetTempDirectory(&temp_dir)) return; - } base::string16 pipe_name = base::ReplaceStringPlaceholders( kPipeNameFormat, base::UTF8ToUTF16(product_name), NULL); From eafc694bba35fd2147539de84621ae2838fc337b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 14:10:45 -0700 Subject: [PATCH 05/23] Mention crash report locations --- docs/api/crash-reporter.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 78a54734382e..092180f49542 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -22,6 +22,12 @@ following projects: * [socorro](https://github.com/mozilla/socorro) * [mini-breakpad-server](https://github.com/electron/mini-breakpad-server) +Crash reports are saved locally in an application-specific temp directory folder. +For a `productName` of `YourName`, crash reports will be stored in a folder +named `YourName Crashes` inside the temp directory. You can customize this temp +directory location for your app by calling the `app.setPath('temp', '/my/custom/temp')` +API before starting the crash reporter. + ## Methods The `crash-reporter` module has the following methods: From 773bfea38695575ad3be5958791087702f2fc53f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 14:12:44 -0700 Subject: [PATCH 06/23] Remove unused rimraf spec dependency --- spec/api-crash-reporter-spec.js | 1 - spec/package.json | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index a501c1133421..92ccefe05e21 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -3,7 +3,6 @@ const fs = require('fs') const http = require('http') const multiparty = require('multiparty') const path = require('path') -const rimraf = require('rimraf') const temp = require('temp').track() const url = require('url') const {closeWindow} = require('./window-helpers') diff --git a/spec/package.json b/spec/package.json index 99692ffb23a4..32b41b55c74c 100644 --- a/spec/package.json +++ b/spec/package.json @@ -6,11 +6,10 @@ "devDependencies": { "basic-auth": "^1.0.0", "graceful-fs": "3.0.5", - "mkdirp": "0.5.1", "mocha": "2.1.0", + "mkdirp": "0.5.1", "multiparty": "4.1.2", "q": "0.9.7", - "rimraf": "^2.5.4", "temp": "0.8.1", "walkdir": "0.0.7", "ws": "0.7.2", From 69a7025c96bfb53bdc7773c1153fc3f45b2d9cf7 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 14:19:22 -0700 Subject: [PATCH 07/23] Fix Linux compiler errors --- atom/common/crash_reporter/crash_reporter_linux.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index 4913e4b3d228..e1d2389f36b2 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -79,13 +79,13 @@ void CrashReporterLinux::SetUploadParameters() { void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) { base::FilePath dumps_path; - if (!GetCrashesDirectory(&dumps_path)) + if (!GetCrashesDirectory(product_name, &dumps_path)) return; base::CreateDirectory(dumps_path); std::string log_file = base::StringPrintf( - "%s/%s", dump_dir.c_str(), "uploads.log"); + "%s/%s", dumps_path.value().c_str(), "uploads.log"); strncpy(g_crash_log_path, log_file.c_str(), sizeof(g_crash_log_path)); MinidumpDescriptor minidump_descriptor(dumps_path.value()); From b3b856f47641fc7470a57ed956b6791b034d0c8b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 14:43:20 -0700 Subject: [PATCH 08/23] Set productName in main process on Linux --- spec/static/main.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/static/main.js b/spec/static/main.js index 77c2febb756f..e910e4de2cd5 100644 --- a/spec/static/main.js +++ b/spec/static/main.js @@ -41,7 +41,7 @@ ipcMain.on('message', function (event, ...args) { }) // Set productName so getUploadedReports() uses the right directory in specs -if (process.platform === 'win32') { +if (process.platform !== 'darwin') { crashReporter.productName = 'Zombies' } From a0db48451041d771b54f7e9513c80ebfa85c0607 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 15:23:51 -0700 Subject: [PATCH 09/23] Use app.getPath directly for temp path --- atom/common/crash_reporter/crash_reporter.cc | 34 ++++++------------- atom/common/crash_reporter/crash_reporter.h | 10 +++--- .../crash_reporter/crash_reporter_linux.cc | 11 +++--- .../crash_reporter/crash_reporter_linux.h | 3 +- .../crash_reporter/crash_reporter_mac.h | 3 +- .../crash_reporter/crash_reporter_mac.mm | 15 ++++---- .../crash_reporter/crash_reporter_win.cc | 7 ++-- lib/common/api/crash-reporter.js | 6 ++-- 8 files changed, 37 insertions(+), 52 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 7a962daf95ef..f7ddd76b449f 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -8,7 +8,6 @@ #include "atom/common/atom_version.h" #include "base/command_line.h" #include "base/files/file_util.h" -#include "base/path_service.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "content/public/common/content_switches.h" @@ -26,29 +25,19 @@ CrashReporter::~CrashReporter() { void CrashReporter::Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters) { SetUploadParameters(extra_parameters); InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url, - auto_submit, skip_system_crash_handler); + temp_path, auto_submit, skip_system_crash_handler); } -bool CrashReporter::GetTempDirectory(base::FilePath* path) { - bool success = PathService::Get(base::DIR_TEMP, path); - if (!success) - LOG(ERROR) << "Cannot get temp directory"; - return success; -} - -bool CrashReporter::GetCrashesDirectory( - const std::string& product_name, base::FilePath* path) { - if (GetTempDirectory(path)) { - *path = path->Append(product_name + " Crashes"); - return true; - } - return false; +base::FilePath CrashReporter::GetCrashesDirectory( + const std::string& product_name, const std::string& temp_dir) { + return base::FilePath(temp_dir).Append(product_name + " Crashes"); } void CrashReporter::SetUploadParameters(const StringMap& parameters) { @@ -60,16 +49,14 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) { } std::vector -CrashReporter::GetUploadedReports(const std::string& product_name) { +CrashReporter::GetUploadedReports(const std::string& product_name, + const std::string& temp_path) { std::vector result; - base::FilePath crashes_dir; - if (!GetCrashesDirectory(product_name, &crashes_dir)) - return result; - + base::FilePath uploads_path = + GetCrashesDirectory(product_name, temp_path).Append("uploads.log"); std::string file_content; - if (base::ReadFileToString(crashes_dir.Append("uploads.log"), - &file_content)) { + if (base::ReadFileToString(uploads_path, &file_content)) { std::vector reports = base::SplitString( file_content, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); for (const std::string& report : reports) { @@ -90,6 +77,7 @@ void CrashReporter::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler) { } diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 5f59468e78a4..84208dfb56c7 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -25,12 +25,14 @@ class CrashReporter { void Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters); virtual std::vector GetUploadedReports( - const std::string& product_name); + const std::string& product_name, + const std::string& temp_path); protected: CrashReporter(); @@ -40,13 +42,13 @@ class CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler); virtual void SetUploadParameters(); - bool GetTempDirectory(base::FilePath* path); - bool GetCrashesDirectory(const std::string& product_name, - base::FilePath* path); + base::FilePath GetCrashesDirectory(const std::string& product_name, + const std::string& temp_dir); StringMap upload_parameters_; bool is_browser_; diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index e1d2389f36b2..e2f4c80e9b44 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -60,9 +60,10 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) { - EnableCrashDumping(product_name); + EnableCrashDumping(product_name, temp_dir); crash_keys_.SetKeyValue("prod", ATOM_PRODUCT_NAME); crash_keys_.SetKeyValue("ver", version.c_str()); @@ -77,11 +78,9 @@ void CrashReporterLinux::SetUploadParameters() { upload_parameters_["platform"] = "linux"; } -void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) { - base::FilePath dumps_path; - if (!GetCrashesDirectory(product_name, &dumps_path)) - return; - +void CrashReporterLinux::EnableCrashDumping(const std::string& product_name, + const std::string& temp_dir) { + base::FilePath dumps_path = GetCrashesDirectory(product_name, temp_dir); base::CreateDirectory(dumps_path); std::string log_file = base::StringPrintf( diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index de5a50e7191e..a8f1a68ac6bd 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -41,7 +41,8 @@ class CrashReporterLinux : public CrashReporter { CrashReporterLinux(); virtual ~CrashReporterLinux(); - void EnableCrashDumping(const std::string& product_name); + void EnableCrashDumping(const std::string& product_name, + const std::string& temp_dir); static bool CrashDone(const google_breakpad::MinidumpDescriptor& minidump, void* context, diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index 8acf50285d36..89fb235eb1c1 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -27,6 +27,7 @@ class CrashReporterMac : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -42,7 +43,7 @@ class CrashReporterMac : public CrashReporter { const base::StringPiece& value); std::vector GetUploadedReports( - const std::string& path) override; + const std::string& path, const std::string& temp_path) override; std::unique_ptr simple_string_dictionary_; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 99e8846e3a0c..8c09e831edee 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -31,6 +31,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) { // check whether crashpad has been initialized. @@ -38,10 +39,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, if (simple_string_dictionary_) return; - base::FilePath database_path; - if (!GetCrashesDirectory(product_name, &database_path)) - return; - + base::FilePath database_path = GetCrashesDirectory(product_name, temp_dir); if (is_browser_) { @autoreleasepool { base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath(); @@ -95,15 +93,14 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, } std::vector -CrashReporterMac::GetUploadedReports(const std::string& product_name) { +CrashReporterMac::GetUploadedReports(const std::string& product_name, + const std::string& temp_path) { std::vector uploaded_reports; - base::FilePath file_path; - if (!GetCrashesDirectory(product_name, &file_path) || - !base::PathExists(file_path)) { + base::FilePath file_path = GetCrashesDirectory(product_name, temp_path); + if (!base::PathExists(file_path)) { return uploaded_reports; } - // Load crashpad database. std::unique_ptr database = crashpad::CrashReportDatabase::Initialize(file_path); diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 27af3e1abb96..e2fe6727e8ad 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -149,14 +149,11 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_path, bool auto_submit, bool skip_system_crash_handler) { skip_system_crash_handler_ = skip_system_crash_handler; - base::FilePath temp_dir; - if (!GetTempDirectory(&temp_dir)) - return; - base::string16 pipe_name = base::ReplaceStringPlaceholders( kPipeNameFormat, base::UTF8ToUTF16(product_name), NULL); base::string16 wait_name = base::ReplaceStringPlaceholders( @@ -175,7 +172,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, breakpad_.reset(); breakpad_.reset(new google_breakpad::ExceptionHandler( - temp_dir.value(), + temp_path, FilterCallback, MinidumpCallback, this, diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index fd7e30b702e6..ca3d33af62f9 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -13,6 +13,7 @@ class CrashReporter { let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options const app = (process.type === 'browser' ? electron : electron.remote).app + this.tempDirectory = app.getPath('temp') if (this.productName == null) { this.productName = app.getName() } @@ -34,7 +35,6 @@ class CrashReporter { if (extra._version == null) { extra._version = app.getVersion() } - if (companyName == null) { throw new Error('companyName is a required option to crashReporter.start') } @@ -53,7 +53,7 @@ class CrashReporter { }) } - binding.start(this.productName, companyName, submitURL, autoSubmit, ignoreSystemCrashHandler, extra) + binding.start(this.productName, companyName, submitURL, this.tempDirectory, autoSubmit, ignoreSystemCrashHandler, extra) } getLastCrashReport () { @@ -66,7 +66,7 @@ class CrashReporter { } getUploadedReports () { - return binding._getUploadedReports(this.productName) + return binding._getUploadedReports(this.productName, this.tempDirectory) } } From 76abb2e18dcc6fa65990ea7a517a541257628131 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 15:29:36 -0700 Subject: [PATCH 10/23] Add missing temp_dir arg to override --- atom/common/crash_reporter/crash_reporter_linux.h | 1 + atom/common/crash_reporter/crash_reporter_win.h | 1 + 2 files changed, 2 insertions(+) diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index a8f1a68ac6bd..f87ae8c6a995 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -31,6 +31,7 @@ class CrashReporterLinux : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index 806c3de317b2..a2265aaaa117 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -27,6 +27,7 @@ class CrashReporterWin : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, + const std::string& temp_dir bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; From 56d9ce34e4acf831262e05c9f102248147a3fddd Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 15:38:26 -0700 Subject: [PATCH 11/23] Use valid Linux report id --- spec/api-crash-reporter-spec.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 92ccefe05e21..5749efb68242 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -60,10 +60,11 @@ describe('crashReporter module', function () { assert.equal(fields['_companyName'], 'Umbrella Corporation') assert.equal(fields['_version'], app.getVersion()) - res.end('abc-123-def', () => { - assert.equal(crashReporter.getLastCrashReport().id, 'abc-123-def') + const reportId = 'abc-123-def-456-abc-789-abc-123-abcd' + res.end(reportId, () => { + assert.equal(crashReporter.getLastCrashReport().id, reportId) assert.notEqual(crashReporter.getUploadedReports().length, 0) - assert.equal(crashReporter.getUploadedReports()[0].id, 'abc-123-def') + assert.equal(crashReporter.getUploadedReports()[0].id, reportId) assert.equal(fs.existsSync(tempDirectory, 'Zombies Crahses'), true) done() }) From 43702e0f8e26eed97cc8c9950de409f9401e4373 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 15:39:47 -0700 Subject: [PATCH 12/23] Support calling getUploadedReports on unstarted crash reporter --- lib/common/api/crash-reporter.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index ca3d33af62f9..ac0c98d8c6e2 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -2,6 +2,7 @@ const {spawn} = require('child_process') const electron = require('electron') +const {app} = process.type === 'browser' ? electron : electron.remote const binding = process.atomBinding('crash_reporter') class CrashReporter { @@ -12,7 +13,6 @@ class CrashReporter { this.productName = options.productName let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options - const app = (process.type === 'browser' ? electron : electron.remote).app this.tempDirectory = app.getPath('temp') if (this.productName == null) { this.productName = app.getName() @@ -66,7 +66,9 @@ class CrashReporter { } getUploadedReports () { - return binding._getUploadedReports(this.productName, this.tempDirectory) + const productName = this.productName != null ? this.productName : app.getName() + const tempDirectory = this.tempDirectory != null ? this.tempDirectory : app.getPath('temp') + return binding._getUploadedReports(productName, tempDirectory) } } From 2fbb98a97c3184b500fa65e597c9684b12a9929c Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 15:56:38 -0700 Subject: [PATCH 13/23] Remove directory assert --- spec/api-crash-reporter-spec.js | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 5749efb68242..bb114f982001 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -65,7 +65,6 @@ describe('crashReporter module', function () { assert.equal(crashReporter.getLastCrashReport().id, reportId) assert.notEqual(crashReporter.getUploadedReports().length, 0) assert.equal(crashReporter.getUploadedReports()[0].id, reportId) - assert.equal(fs.existsSync(tempDirectory, 'Zombies Crahses'), true) done() }) }) From 0380d3ae50c9290c396523a35c6e16cef859d853 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 16:15:49 -0700 Subject: [PATCH 14/23] Pass crashes directory to crash service process --- atom/common/api/atom_api_crash_reporter.cc | 3 +++ atom/common/crash_reporter/crash_reporter.h | 6 +++--- .../crash_reporter/win/crash_service_main.cc | 20 ++++++++++--------- lib/common/api/crash-reporter.js | 7 ++++++- spec/api-crash-reporter-spec.js | 1 - 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/atom/common/api/atom_api_crash_reporter.cc b/atom/common/api/atom_api_crash_reporter.cc index 184a70c72c14..b416bdaaf0bb 100644 --- a/atom/common/api/atom_api_crash_reporter.cc +++ b/atom/common/api/atom_api_crash_reporter.cc @@ -6,6 +6,7 @@ #include #include "atom/common/crash_reporter/crash_reporter.h" +#include "atom/common/native_mate_converters/file_path_converter.h" #include "base/bind.h" #include "native_mate/dictionary.h" @@ -39,6 +40,8 @@ void Initialize(v8::Local exports, v8::Local unused, base::Bind(&CrashReporter::Start, report)); dict.SetMethod("_getUploadedReports", base::Bind(&CrashReporter::GetUploadedReports, report)); + dict.SetMethod("_getCrashesDirectory", + base::Bind(&CrashReporter::GetCrashesDirectory, report)); } } // namespace diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 84208dfb56c7..40a1f5a47e5b 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -34,6 +34,9 @@ class CrashReporter { const std::string& product_name, const std::string& temp_path); + base::FilePath GetCrashesDirectory(const std::string& product_name, + const std::string& temp_dir); + protected: CrashReporter(); virtual ~CrashReporter(); @@ -47,9 +50,6 @@ class CrashReporter { bool skip_system_crash_handler); virtual void SetUploadParameters(); - base::FilePath GetCrashesDirectory(const std::string& product_name, - const std::string& temp_dir); - StringMap upload_parameters_; bool is_browser_; diff --git a/atom/common/crash_reporter/win/crash_service_main.cc b/atom/common/crash_reporter/win/crash_service_main.cc index c6325f090ade..c672c0e6209b 100644 --- a/atom/common/crash_reporter/win/crash_service_main.cc +++ b/atom/common/crash_reporter/win/crash_service_main.cc @@ -16,6 +16,7 @@ namespace crash_service { namespace { const char kApplicationName[] = "application-name"; +const char kCrashesDirectory[] = "crashes-directory"; const wchar_t kPipeNameFormat[] = L"\\\\.\\pipe\\$1 Crash Service"; const wchar_t kStandardLogFile[] = L"operation_log.txt"; @@ -25,17 +26,11 @@ void InvalidParameterHandler(const wchar_t*, const wchar_t*, const wchar_t*, // noop. } -bool GetCrashServiceDirectory(const std::wstring& application_name, - base::FilePath* dir) { - base::FilePath temp_dir; - if (!base::GetTempDir(&temp_dir)) - return false; - temp_dir = temp_dir.Append(application_name + L" Crashes"); +bool CreateCrashServiceDirectory(const base::FilePath& temp_dir) { if (!base::PathExists(temp_dir)) { if (!base::CreateDirectory(temp_dir)) return false; } - *dir = temp_dir; return true; } @@ -59,9 +54,16 @@ int Main(const wchar_t* cmd) { std::wstring application_name = cmd_line.GetSwitchValueNative( kApplicationName); + if (!cmd_line.HasSwitch(kCrashesDirectory)) { + LOG(ERROR) << "Crashes directory path must be specified with --" + << kCrashesDirectory; + return 1; + } + // We use/create a directory under the user's temp folder, for logging. - base::FilePath operating_dir; - GetCrashServiceDirectory(application_name, &operating_dir); + base::FilePath operating_dir( + cmd_line.GetSwitchValueNative(kCrashesDirectory)); + GetCrashServiceDirectory(operating_dir); base::FilePath log_file = operating_dir.Append(kStandardLogFile); // Logging to stderr (to help with debugging failures on the diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index ac0c98d8c6e2..ca429dfe1abf 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -43,7 +43,12 @@ class CrashReporter { } if (process.platform === 'win32') { - const args = ['--reporter-url=' + submitURL, '--application-name=' + this.productName, '--v=1'] + const args = [ + '--reporter-url=' + submitURL, + '--application-name=' + this.productName, + '--crashes-directory=' + bindings._getCrashesDirectory(this.productName, this.tempDirectory), + '--v=1' + ] const env = { ELECTRON_INTERNAL_CRASH_SERVICE: 1 } diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index bb114f982001..72e9317d7e62 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -1,5 +1,4 @@ const assert = require('assert') -const fs = require('fs') const http = require('http') const multiparty = require('multiparty') const path = require('path') From 883c4b63d05bbcd1f7612752b299368a43148b8b Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 16:21:39 -0700 Subject: [PATCH 15/23] temp_path -> temp_dir --- atom/common/crash_reporter/crash_reporter.cc | 10 +++++----- atom/common/crash_reporter/crash_reporter.h | 6 +++--- atom/common/crash_reporter/crash_reporter_mac.h | 4 ++-- atom/common/crash_reporter/crash_reporter_mac.mm | 4 ++-- atom/common/crash_reporter/crash_reporter_win.cc | 4 ++-- atom/common/crash_reporter/crash_reporter_win.h | 2 +- lib/common/api/crash-reporter.js | 2 +- 7 files changed, 16 insertions(+), 16 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index f7ddd76b449f..2506e28a05d8 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -25,14 +25,14 @@ CrashReporter::~CrashReporter() { void CrashReporter::Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters) { SetUploadParameters(extra_parameters); InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url, - temp_path, auto_submit, skip_system_crash_handler); + temp_dir, auto_submit, skip_system_crash_handler); } base::FilePath CrashReporter::GetCrashesDirectory( @@ -50,11 +50,11 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) { std::vector CrashReporter::GetUploadedReports(const std::string& product_name, - const std::string& temp_path) { + const std::string& temp_dir) { std::vector result; base::FilePath uploads_path = - GetCrashesDirectory(product_name, temp_path).Append("uploads.log"); + GetCrashesDirectory(product_name, temp_dir).Append("uploads.log"); std::string file_content; if (base::ReadFileToString(uploads_path, &file_content)) { std::vector reports = base::SplitString( @@ -77,7 +77,7 @@ void CrashReporter::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) { } diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 40a1f5a47e5b..b154a8d9333a 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -25,14 +25,14 @@ class CrashReporter { void Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters); virtual std::vector GetUploadedReports( const std::string& product_name, - const std::string& temp_path); + const std::string& temp_dir); base::FilePath GetCrashesDirectory(const std::string& product_name, const std::string& temp_dir); @@ -45,7 +45,7 @@ class CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler); virtual void SetUploadParameters(); diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index 89fb235eb1c1..2cf189f3bf90 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -27,7 +27,7 @@ class CrashReporterMac : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -43,7 +43,7 @@ class CrashReporterMac : public CrashReporter { const base::StringPiece& value); std::vector GetUploadedReports( - const std::string& path, const std::string& temp_path) override; + const std::string& path, const std::string& temp_dir) override; std::unique_ptr simple_string_dictionary_; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 8c09e831edee..071c4fd28a95 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -94,10 +94,10 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, std::vector CrashReporterMac::GetUploadedReports(const std::string& product_name, - const std::string& temp_path) { + const std::string& temp_dir) { std::vector uploaded_reports; - base::FilePath file_path = GetCrashesDirectory(product_name, temp_path); + base::FilePath file_path = GetCrashesDirectory(product_name, temp_dir); if (!base::PathExists(file_path)) { return uploaded_reports; } diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index e2fe6727e8ad..644556c1805a 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -149,7 +149,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_path, + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) { skip_system_crash_handler_ = skip_system_crash_handler; @@ -172,7 +172,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, breakpad_.reset(); breakpad_.reset(new google_breakpad::ExceptionHandler( - temp_path, + temp_dir, FilterCallback, MinidumpCallback, this, diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index a2265aaaa117..ec47cb7e665c 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -27,7 +27,7 @@ class CrashReporterWin : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir + const std::string& temp_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index ca429dfe1abf..def77e72f60e 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -46,7 +46,7 @@ class CrashReporter { const args = [ '--reporter-url=' + submitURL, '--application-name=' + this.productName, - '--crashes-directory=' + bindings._getCrashesDirectory(this.productName, this.tempDirectory), + '--crashes-directory=' + binding._getCrashesDirectory(this.productName, this.tempDirectory), '--v=1' ] const env = { From 9d1d1f21e94a42b5be09d5898a5aa46271cf5eed Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 16:40:19 -0700 Subject: [PATCH 16/23] Use FilePath for crash reporter temp directory --- atom/common/crash_reporter/crash_reporter.cc | 15 ++++++++------- atom/common/crash_reporter/crash_reporter.h | 8 ++++---- .../common/crash_reporter/crash_reporter_linux.cc | 8 ++++---- atom/common/crash_reporter/crash_reporter_linux.h | 4 ++-- atom/common/crash_reporter/crash_reporter_mac.h | 4 ++-- atom/common/crash_reporter/crash_reporter_mac.mm | 8 ++++---- atom/common/crash_reporter/crash_reporter_win.cc | 4 ++-- atom/common/crash_reporter/crash_reporter_win.h | 2 +- 8 files changed, 27 insertions(+), 26 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 2506e28a05d8..5b621c793f33 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -25,19 +25,19 @@ CrashReporter::~CrashReporter() { void CrashReporter::Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters) { SetUploadParameters(extra_parameters); InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url, - temp_dir, auto_submit, skip_system_crash_handler); + temp_path, auto_submit, skip_system_crash_handler); } base::FilePath CrashReporter::GetCrashesDirectory( - const std::string& product_name, const std::string& temp_dir) { - return base::FilePath(temp_dir).Append(product_name + " Crashes"); + const std::string& product_name, const base::FilePath& temp_path) { + return temp_path.Append(product_name + " Crashes"); } void CrashReporter::SetUploadParameters(const StringMap& parameters) { @@ -50,11 +50,12 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) { std::vector CrashReporter::GetUploadedReports(const std::string& product_name, - const std::string& temp_dir) { + const base::FilePath& temp_path) { std::vector result; base::FilePath uploads_path = - GetCrashesDirectory(product_name, temp_dir).Append("uploads.log"); + GetCrashesDirectory(product_name, temp_path) + .Append(FILE_PATH_LITERAL("uploads.log")); std::string file_content; if (base::ReadFileToString(uploads_path, &file_content)) { std::vector reports = base::SplitString( @@ -77,7 +78,7 @@ void CrashReporter::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) { } diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index b154a8d9333a..97389248b1c3 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -25,17 +25,17 @@ class CrashReporter { void Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters); virtual std::vector GetUploadedReports( const std::string& product_name, - const std::string& temp_dir); + const base::FilePath& temp_path); base::FilePath GetCrashesDirectory(const std::string& product_name, - const std::string& temp_dir); + const base::FilePath& temp_path); protected: CrashReporter(); @@ -45,7 +45,7 @@ class CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler); virtual void SetUploadParameters(); diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index e2f4c80e9b44..c670d9122afc 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -60,10 +60,10 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) { - EnableCrashDumping(product_name, temp_dir); + EnableCrashDumping(product_name, temp_path); crash_keys_.SetKeyValue("prod", ATOM_PRODUCT_NAME); crash_keys_.SetKeyValue("ver", version.c_str()); @@ -79,8 +79,8 @@ void CrashReporterLinux::SetUploadParameters() { } void CrashReporterLinux::EnableCrashDumping(const std::string& product_name, - const std::string& temp_dir) { - base::FilePath dumps_path = GetCrashesDirectory(product_name, temp_dir); + const base::FilePath& temp_path) { + base::FilePath dumps_path = GetCrashesDirectory(product_name, temp_path); base::CreateDirectory(dumps_path); std::string log_file = base::StringPrintf( diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index f87ae8c6a995..8e4fb7686849 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -31,7 +31,7 @@ class CrashReporterLinux : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -43,7 +43,7 @@ class CrashReporterLinux : public CrashReporter { virtual ~CrashReporterLinux(); void EnableCrashDumping(const std::string& product_name, - const std::string& temp_dir); + const base::FilePath& temp_path); static bool CrashDone(const google_breakpad::MinidumpDescriptor& minidump, void* context, diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index 2cf189f3bf90..a04279944c78 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -27,7 +27,7 @@ class CrashReporterMac : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -43,7 +43,7 @@ class CrashReporterMac : public CrashReporter { const base::StringPiece& value); std::vector GetUploadedReports( - const std::string& path, const std::string& temp_dir) override; + const std::string& path, const base::FilePath& temp_path) override; std::unique_ptr simple_string_dictionary_; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 071c4fd28a95..19e59cac3026 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -31,7 +31,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) { // check whether crashpad has been initialized. @@ -39,7 +39,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, if (simple_string_dictionary_) return; - base::FilePath database_path = GetCrashesDirectory(product_name, temp_dir); + base::FilePath database_path = GetCrashesDirectory(product_name, temp_path); if (is_browser_) { @autoreleasepool { base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath(); @@ -94,10 +94,10 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, std::vector CrashReporterMac::GetUploadedReports(const std::string& product_name, - const std::string& temp_dir) { + const base::FilePath& temp_path) { std::vector uploaded_reports; - base::FilePath file_path = GetCrashesDirectory(product_name, temp_dir); + base::FilePath file_path = GetCrashesDirectory(product_name, temp_path); if (!base::PathExists(file_path)) { return uploaded_reports; } diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 644556c1805a..c8f3491732d9 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -149,7 +149,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) { skip_system_crash_handler_ = skip_system_crash_handler; @@ -172,7 +172,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, breakpad_.reset(); breakpad_.reset(new google_breakpad::ExceptionHandler( - temp_dir, + temp_path.value(), FilterCallback, MinidumpCallback, this, diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index ec47cb7e665c..1b9955ff43ad 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -27,7 +27,7 @@ class CrashReporterWin : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const std::string& temp_dir, + const base::FilePath& temp_path, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; From 84b7bb29f03367b29dfc594219ae7021babe4243 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Wed, 5 Oct 2016 16:55:13 -0700 Subject: [PATCH 17/23] Fix Windows compiler errors --- atom/common/crash_reporter/crash_reporter.cc | 3 ++- atom/common/crash_reporter/win/crash_service_main.cc | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 5b621c793f33..30b3852a6902 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -11,6 +11,7 @@ #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" #include "content/public/common/content_switches.h" +#include "base/strings/utf_string_conversions.h" namespace crash_reporter { @@ -37,7 +38,7 @@ void CrashReporter::Start(const std::string& product_name, base::FilePath CrashReporter::GetCrashesDirectory( const std::string& product_name, const base::FilePath& temp_path) { - return temp_path.Append(product_name + " Crashes"); + return temp_path.Append(base::UTF8ToUTF16(product_name + " Crashes")); } void CrashReporter::SetUploadParameters(const StringMap& parameters) { diff --git a/atom/common/crash_reporter/win/crash_service_main.cc b/atom/common/crash_reporter/win/crash_service_main.cc index c672c0e6209b..abeb2c1060e8 100644 --- a/atom/common/crash_reporter/win/crash_service_main.cc +++ b/atom/common/crash_reporter/win/crash_service_main.cc @@ -63,7 +63,7 @@ int Main(const wchar_t* cmd) { // We use/create a directory under the user's temp folder, for logging. base::FilePath operating_dir( cmd_line.GetSwitchValueNative(kCrashesDirectory)); - GetCrashServiceDirectory(operating_dir); + CreateCrashServiceDirectory(operating_dir); base::FilePath log_file = operating_dir.Append(kStandardLogFile); // Logging to stderr (to help with debugging failures on the From 30c6ca6751d6e79f5042c82b23e7369dc2ebf20f Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 09:14:16 -0700 Subject: [PATCH 18/23] Only call UTF8ToUTF16 on Windows --- atom/browser/api/atom_api_app.cc | 4 ++-- atom/common/crash_reporter/crash_reporter.cc | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/atom/browser/api/atom_api_app.cc b/atom/browser/api/atom_api_app.cc index c3869c07fea7..5b3528002850 100644 --- a/atom/browser/api/atom_api_app.cc +++ b/atom/browser/api/atom_api_app.cc @@ -601,7 +601,7 @@ base::FilePath App::GetPath(mate::Arguments* args, const std::string& name) { if (key >= 0) succeed = PathService::Get(key, &path); if (!succeed) - args->ThrowError("Failed to get path"); + args->ThrowError("Failed to get '" + name + "' path"); return path; } @@ -609,7 +609,7 @@ void App::SetPath(mate::Arguments* args, const std::string& name, const base::FilePath& path) { if (!path.IsAbsolute()) { - args->ThrowError("path must be absolute"); + args->ThrowError("Path must be absolute"); return; } diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 30b3852a6902..e6638ecccb1d 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -38,7 +38,12 @@ void CrashReporter::Start(const std::string& product_name, base::FilePath CrashReporter::GetCrashesDirectory( const std::string& product_name, const base::FilePath& temp_path) { - return temp_path.Append(base::UTF8ToUTF16(product_name + " Crashes")); + std::string folder_name = product_name + " Crashes"; +#if defined(OS_WIN) + return temp_path.Append(base::UTF8ToUTF16(folder_name)); +#else + return temp_path.Append(folder_name); +#endif } void CrashReporter::SetUploadParameters(const StringMap& parameters) { From 4a8dcec63a76af18393b0cc3a23693235fc9fb2d Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 09:45:58 -0700 Subject: [PATCH 19/23] Wait for crash report to become available --- spec/api-crash-reporter-spec.js | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 72e9317d7e62..fe8c3f6c62d9 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -61,10 +61,12 @@ describe('crashReporter module', function () { const reportId = 'abc-123-def-456-abc-789-abc-123-abcd' res.end(reportId, () => { - assert.equal(crashReporter.getLastCrashReport().id, reportId) - assert.notEqual(crashReporter.getUploadedReports().length, 0) - assert.equal(crashReporter.getUploadedReports()[0].id, reportId) - done() + waitForCrashReport().then(() => { + assert.equal(crashReporter.getLastCrashReport().id, reportId) + assert.notEqual(crashReporter.getUploadedReports().length, 0) + assert.equal(crashReporter.getUploadedReports()[0].id, reportId) + done() + }, done) }) }) }) @@ -116,3 +118,20 @@ describe('crashReporter module', function () { }) }) }) + +const waitForCrashReport = () => { + return new Promise((resolve, reject) => { + let times = 0 + const checkForReport = () => { + if (crashReporter.getLastCrashReport() != null) { + resolve() + } else if (times >= 10) { + reject(new Error('No crash report available')) + } else { + times++ + setTimeout(checkForReport, 100) + } + } + checkForReport() + }) +} From f61ace74bb28aa9eb75ef4d860a2609b5f1bf9f2 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 09:50:06 -0700 Subject: [PATCH 20/23] Sort includes --- atom/common/crash_reporter/crash_reporter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index e6638ecccb1d..79ec2f9d8d6c 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -10,8 +10,8 @@ #include "base/files/file_util.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" -#include "content/public/common/content_switches.h" #include "base/strings/utf_string_conversions.h" +#include "content/public/common/content_switches.h" namespace crash_reporter { From fbbffe03a5bb42b9b384773fd548d706f5b949b9 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 09:57:25 -0700 Subject: [PATCH 21/23] Add getPath specs --- spec/api-app-spec.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/api-app-spec.js b/spec/api-app-spec.js index 84c207713e7b..6cc6f777739a 100644 --- a/spec/api-app-spec.js +++ b/spec/api-app-spec.js @@ -334,4 +334,23 @@ describe('app module', function () { assert.equal(typeof app.isAccessibilitySupportEnabled(), 'boolean') }) }) + + describe('getPath(name)', function () { + it('returns paths that exist', function () { + assert.equal(fs.existsSync(app.getPath('exe')), true) + assert.equal(fs.existsSync(app.getPath('home')), true) + assert.equal(fs.existsSync(app.getPath('temp')), true) + }) + + it('throws an error when the name is invalid', function () { + assert.throws(function () { + app.getPath('does-not-exist') + }, /Failed to get 'does-not-exist' path/) + }) + + it('returns the overridden path', function () { + app.setPath('music', __dirname) + assert.equal(app.getPath('music'), __dirname) + }) + }) }) From 16e3991ffa300ea1855e49d24b6046b5c0322448 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 10:02:46 -0700 Subject: [PATCH 22/23] Guard against app.getPath throwing with OS fallback --- lib/common/api/crash-reporter.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index def77e72f60e..312868e1834a 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -1,6 +1,7 @@ 'use strict' const {spawn} = require('child_process') +const os = require('os') const electron = require('electron') const {app} = process.type === 'browser' ? electron : electron.remote const binding = process.atomBinding('crash_reporter') @@ -13,7 +14,7 @@ class CrashReporter { this.productName = options.productName let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options - this.tempDirectory = app.getPath('temp') + this.tempDirectory = getTempPath() if (this.productName == null) { this.productName = app.getName() } @@ -72,9 +73,18 @@ class CrashReporter { getUploadedReports () { const productName = this.productName != null ? this.productName : app.getName() - const tempDirectory = this.tempDirectory != null ? this.tempDirectory : app.getPath('temp') + const tempDirectory = this.tempDirectory != null ? this.tempDirectory : getTempPath() return binding._getUploadedReports(productName, tempDirectory) } } +const getTempPath = () => { + try { + return app.getPath('temp') + } catch (error) { + // app.getPath may throw so fallback to OS temp directory + return os.tmpdir() + } +} + module.exports = new CrashReporter() From d39182b41a7eb74199970d9567f519565d331155 Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Thu, 6 Oct 2016 10:39:57 -0700 Subject: [PATCH 23/23] Pass crashes directory instead of product name and temp dir --- atom/common/api/atom_api_crash_reporter.cc | 2 - atom/common/crash_reporter/crash_reporter.cc | 28 +++-------- atom/common/crash_reporter/crash_reporter.h | 10 ++-- .../crash_reporter/crash_reporter_linux.cc | 16 +++---- .../crash_reporter/crash_reporter_linux.h | 5 +- .../crash_reporter/crash_reporter_mac.h | 4 +- .../crash_reporter/crash_reporter_mac.mm | 16 +++---- .../crash_reporter/crash_reporter_win.cc | 4 +- .../crash_reporter/crash_reporter_win.h | 2 +- lib/common/api/crash-reporter.js | 48 +++++++++++-------- 10 files changed, 58 insertions(+), 77 deletions(-) diff --git a/atom/common/api/atom_api_crash_reporter.cc b/atom/common/api/atom_api_crash_reporter.cc index b416bdaaf0bb..5dff9e667fe6 100644 --- a/atom/common/api/atom_api_crash_reporter.cc +++ b/atom/common/api/atom_api_crash_reporter.cc @@ -40,8 +40,6 @@ void Initialize(v8::Local exports, v8::Local unused, base::Bind(&CrashReporter::Start, report)); dict.SetMethod("_getUploadedReports", base::Bind(&CrashReporter::GetUploadedReports, report)); - dict.SetMethod("_getCrashesDirectory", - base::Bind(&CrashReporter::GetCrashesDirectory, report)); } } // namespace diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index 79ec2f9d8d6c..1254f3fde6cf 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -10,7 +10,6 @@ #include "base/files/file_util.h" #include "base/strings/string_number_conversions.h" #include "base/strings/string_split.h" -#include "base/strings/utf_string_conversions.h" #include "content/public/common/content_switches.h" namespace crash_reporter { @@ -26,24 +25,14 @@ CrashReporter::~CrashReporter() { void CrashReporter::Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters) { SetUploadParameters(extra_parameters); InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url, - temp_path, auto_submit, skip_system_crash_handler); -} - -base::FilePath CrashReporter::GetCrashesDirectory( - const std::string& product_name, const base::FilePath& temp_path) { - std::string folder_name = product_name + " Crashes"; -#if defined(OS_WIN) - return temp_path.Append(base::UTF8ToUTF16(folder_name)); -#else - return temp_path.Append(folder_name); -#endif + crashes_dir, auto_submit, skip_system_crash_handler); } void CrashReporter::SetUploadParameters(const StringMap& parameters) { @@ -55,14 +44,11 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) { } std::vector -CrashReporter::GetUploadedReports(const std::string& product_name, - const base::FilePath& temp_path) { - std::vector result; - - base::FilePath uploads_path = - GetCrashesDirectory(product_name, temp_path) - .Append(FILE_PATH_LITERAL("uploads.log")); +CrashReporter::GetUploadedReports(const base::FilePath& crashes_dir) { std::string file_content; + std::vector result; + base::FilePath uploads_path = + crashes_dir.Append(FILE_PATH_LITERAL("uploads.log")); if (base::ReadFileToString(uploads_path, &file_content)) { std::vector reports = base::SplitString( file_content, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); @@ -84,7 +70,7 @@ void CrashReporter::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) { } diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 97389248b1c3..ef153523acfd 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -25,17 +25,13 @@ class CrashReporter { void Start(const std::string& product_name, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler, const StringMap& extra_parameters); virtual std::vector GetUploadedReports( - const std::string& product_name, - const base::FilePath& temp_path); - - base::FilePath GetCrashesDirectory(const std::string& product_name, - const base::FilePath& temp_path); + const base::FilePath& crashes_dir); protected: CrashReporter(); @@ -45,7 +41,7 @@ class CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler); virtual void SetUploadParameters(); diff --git a/atom/common/crash_reporter/crash_reporter_linux.cc b/atom/common/crash_reporter/crash_reporter_linux.cc index c670d9122afc..2442d7e168e3 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.cc +++ b/atom/common/crash_reporter/crash_reporter_linux.cc @@ -17,7 +17,6 @@ #include "base/logging.h" #include "base/memory/singleton.h" #include "base/process/memory.h" -#include "base/strings/stringprintf.h" #include "vendor/breakpad/src/client/linux/handler/exception_handler.h" #include "vendor/breakpad/src/common/linux/linux_libc_support.h" @@ -60,10 +59,10 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) { - EnableCrashDumping(product_name, temp_path); + EnableCrashDumping(crashes_dir); crash_keys_.SetKeyValue("prod", ATOM_PRODUCT_NAME); crash_keys_.SetKeyValue("ver", version.c_str()); @@ -78,16 +77,13 @@ void CrashReporterLinux::SetUploadParameters() { upload_parameters_["platform"] = "linux"; } -void CrashReporterLinux::EnableCrashDumping(const std::string& product_name, - const base::FilePath& temp_path) { - base::FilePath dumps_path = GetCrashesDirectory(product_name, temp_path); - base::CreateDirectory(dumps_path); +void CrashReporterLinux::EnableCrashDumping(const base::FilePath& crashes_dir) { + base::CreateDirectory(crashes_dir); - std::string log_file = base::StringPrintf( - "%s/%s", dumps_path.value().c_str(), "uploads.log"); + std::string log_file = crashes_dir.Append("uploads.log").value(); strncpy(g_crash_log_path, log_file.c_str(), sizeof(g_crash_log_path)); - MinidumpDescriptor minidump_descriptor(dumps_path.value()); + MinidumpDescriptor minidump_descriptor(crashes_dir.value()); minidump_descriptor.set_size_limit(kMaxMinidumpFileSize); breakpad_.reset(new ExceptionHandler( diff --git a/atom/common/crash_reporter/crash_reporter_linux.h b/atom/common/crash_reporter/crash_reporter_linux.h index 8e4fb7686849..36a369973211 100644 --- a/atom/common/crash_reporter/crash_reporter_linux.h +++ b/atom/common/crash_reporter/crash_reporter_linux.h @@ -31,7 +31,7 @@ class CrashReporterLinux : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -42,8 +42,7 @@ class CrashReporterLinux : public CrashReporter { CrashReporterLinux(); virtual ~CrashReporterLinux(); - void EnableCrashDumping(const std::string& product_name, - const base::FilePath& temp_path); + void EnableCrashDumping(const base::FilePath& crashes_dir); static bool CrashDone(const google_breakpad::MinidumpDescriptor& minidump, void* context, diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index a04279944c78..ed4888bbaf1b 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -27,7 +27,7 @@ class CrashReporterMac : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; @@ -43,7 +43,7 @@ class CrashReporterMac : public CrashReporter { const base::StringPiece& value); std::vector GetUploadedReports( - const std::string& path, const base::FilePath& temp_path) override; + const base::FilePath& crashes_dir) override; std::unique_ptr simple_string_dictionary_; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 19e59cac3026..82e3ea140c73 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -6,7 +6,6 @@ #include -#include "base/files/file_path.h" #include "base/files/file_util.h" #include "base/mac/bundle_locations.h" #include "base/mac/mac_util.h" @@ -31,7 +30,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) { // check whether crashpad has been initialized. @@ -39,7 +38,6 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, if (simple_string_dictionary_) return; - base::FilePath database_path = GetCrashesDirectory(product_name, temp_path); if (is_browser_) { @autoreleasepool { base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath(); @@ -47,7 +45,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, framework_bundle_path.Append("Resources").Append("crashpad_handler"); crashpad::CrashpadClient crashpad_client; - if (crashpad_client.StartHandler(handler_path, database_path, + if (crashpad_client.StartHandler(handler_path, crashes_dir, submit_url, StringMap(), std::vector(), @@ -76,7 +74,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name, } if (is_browser_) { std::unique_ptr database = - crashpad::CrashReportDatabase::Initialize(database_path); + crashpad::CrashReportDatabase::Initialize(crashes_dir); if (database) { database->GetSettings()->SetUploadsEnabled(auto_submit); } @@ -93,17 +91,15 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, } std::vector -CrashReporterMac::GetUploadedReports(const std::string& product_name, - const base::FilePath& temp_path) { +CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) { std::vector uploaded_reports; - base::FilePath file_path = GetCrashesDirectory(product_name, temp_path); - if (!base::PathExists(file_path)) { + if (!base::PathExists(crashes_dir)) { return uploaded_reports; } // Load crashpad database. std::unique_ptr database = - crashpad::CrashReportDatabase::Initialize(file_path); + crashpad::CrashReportDatabase::Initialize(crashes_dir); DCHECK(database); std::vector completed_reports; diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index c8f3491732d9..a465edcb45ea 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -149,7 +149,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) { skip_system_crash_handler_ = skip_system_crash_handler; @@ -172,7 +172,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, breakpad_.reset(); breakpad_.reset(new google_breakpad::ExceptionHandler( - temp_path.value(), + crashes_dir.DirName().value(), FilterCallback, MinidumpCallback, this, diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index 1b9955ff43ad..20a537d89224 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -27,7 +27,7 @@ class CrashReporterWin : public CrashReporter { const std::string& version, const std::string& company_name, const std::string& submit_url, - const base::FilePath& temp_path, + const base::FilePath& crashes_dir, bool auto_submit, bool skip_system_crash_handler) override; void SetUploadParameters() override; diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index 312868e1834a..7749583f79cf 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -2,6 +2,7 @@ const {spawn} = require('child_process') const os = require('os') +const path = require('path') const electron = require('electron') const {app} = process.type === 'browser' ? electron : electron.remote const binding = process.atomBinding('crash_reporter') @@ -11,13 +12,9 @@ class CrashReporter { if (options == null) { options = {} } - this.productName = options.productName + this.productName = options.productName != null ? options.productName : app.getName() let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options - this.tempDirectory = getTempPath() - if (this.productName == null) { - this.productName = app.getName() - } if (autoSubmit == null) { autoSubmit = true } @@ -28,7 +25,7 @@ class CrashReporter { extra = {} } if (extra._productName == null) { - extra._productName = this.productName + extra._productName = this.getProductName() } if (extra._companyName == null) { extra._companyName = companyName @@ -46,8 +43,8 @@ class CrashReporter { if (process.platform === 'win32') { const args = [ '--reporter-url=' + submitURL, - '--application-name=' + this.productName, - '--crashes-directory=' + binding._getCrashesDirectory(this.productName, this.tempDirectory), + '--application-name=' + this.getProductName(), + '--crashes-directory=' + this.getCrashesDirectory(), '--v=1' ] const env = { @@ -59,7 +56,7 @@ class CrashReporter { }) } - binding.start(this.productName, companyName, submitURL, this.tempDirectory, autoSubmit, ignoreSystemCrashHandler, extra) + binding.start(this.getProductName(), companyName, submitURL, this.getCrashesDirectory(), autoSubmit, ignoreSystemCrashHandler, extra) } getLastCrashReport () { @@ -72,18 +69,31 @@ class CrashReporter { } getUploadedReports () { - const productName = this.productName != null ? this.productName : app.getName() - const tempDirectory = this.tempDirectory != null ? this.tempDirectory : getTempPath() - return binding._getUploadedReports(productName, tempDirectory) + return binding._getUploadedReports(this.getCrashesDirectory()) } -} -const getTempPath = () => { - try { - return app.getPath('temp') - } catch (error) { - // app.getPath may throw so fallback to OS temp directory - return os.tmpdir() + getCrashesDirectory () { + const crashesDir = this.getProductName() + ' Crashes' + return path.join(this.getTempDirectory(), crashesDir) + } + + getProductName () { + if (this.productName == null) { + this.productName = app.getName() + } + return this.productName + } + + getTempDirectory () { + if (this.tempDirectory == null) { + try { + this.tempDirectory = app.getPath('temp') + } catch (error) { + // app.getPath may throw so fallback to OS temp directory + this.tempDirectory = os.tmpdir() + } + } + return this.tempDirectory } }