From f0d447cd681c276acc4ab560650770a270b030c0 Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Wed, 29 Mar 2017 09:16:27 -0300 Subject: [PATCH 1/3] Pass `uploadToServer` to windows crash reporter --- atom/common/crash_reporter/crash_reporter_win.cc | 11 ++++++++--- atom/common/crash_reporter/crash_reporter_win.h | 3 ++- atom/common/crash_reporter/win/crash_service.cc | 2 +- docs/api/crash-reporter.md | 2 +- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 25969dc32e..b13108dbfe 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -179,7 +179,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name, google_breakpad::ExceptionHandler::HANDLER_ALL, kSmallDumpType, pipe_name.c_str(), - GetCustomInfo(product_name, version, company_name))); + GetCustomInfo(product_name, version, company_name, upload_to_server))); if (!breakpad_->IsOutOfProcess()) LOG(ERROR) << "Cannot initialize out-of-process crash handler"; @@ -238,14 +238,19 @@ bool CrashReporterWin::MinidumpCallback(const wchar_t* dump_path, google_breakpad::CustomClientInfo* CrashReporterWin::GetCustomInfo( const std::string& product_name, const std::string& version, - const std::string& company_name) { + const std::string& company_name, + bool upload_to_server) { custom_info_entries_.clear(); - custom_info_entries_.reserve(2 + upload_parameters_.size()); + custom_info_entries_.reserve(3 + upload_parameters_.size()); custom_info_entries_.push_back(google_breakpad::CustomInfoEntry( L"prod", L"Electron")); custom_info_entries_.push_back(google_breakpad::CustomInfoEntry( L"ver", base::UTF8ToWide(version).c_str())); + if (!upload_to_server) { + custom_info_entries_.push_back(google_breakpad::CustomInfoEntry( + L"skip_upload", L"1")); + } for (StringMap::const_iterator iter = upload_parameters_.begin(); iter != upload_parameters_.end(); ++iter) { diff --git a/atom/common/crash_reporter/crash_reporter_win.h b/atom/common/crash_reporter/crash_reporter_win.h index 3fc139aca1..5070df2060 100644 --- a/atom/common/crash_reporter/crash_reporter_win.h +++ b/atom/common/crash_reporter/crash_reporter_win.h @@ -56,7 +56,8 @@ class CrashReporterWin : public CrashReporter { google_breakpad::CustomClientInfo* GetCustomInfo( const std::string& product_name, const std::string& version, - const std::string& company_name); + const std::string& company_name, + bool upload_to_server); // Custom information to be passed to crash handler. std::vector custom_info_entries_; diff --git a/atom/common/crash_reporter/win/crash_service.cc b/atom/common/crash_reporter/win/crash_service.cc index 5782fd72a3..a306a567a7 100644 --- a/atom/common/crash_reporter/win/crash_service.cc +++ b/atom/common/crash_reporter/win/crash_service.cc @@ -391,7 +391,7 @@ void CrashService::OnClientDumpRequest(void* context, LOG(ERROR) << "could not write custom info file"; } - if (!self->sender_) + if (!self->sender_ || map.find(L"skip_upload") != map.end()) return; // Send the crash dump using a worker thread. This operation has retry diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 98bf0f3662..2f0848c119 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -40,7 +40,7 @@ The `crashReporter` module has the following methods: * `companyName` String (optional) * `submitURL` String - URL that crash reports will be sent to as POST. * `productName` String (optional) - Defaults to `app.getName()`. - * `uploadToServer` Boolean (optional) _Linux_ _macOS_ - Whether crash reports should be sent to the server + * `uploadToServer` Boolean (optional) - Whether crash reports should be sent to the server Default is `true`. * `ignoreSystemCrashHandler` Boolean (optional) - Default is `false`. * `extra` Object (optional) - An object you can define that will be sent along with the From 460fb9cdb93a160a8e3ebf8d3351ad87551a4906 Mon Sep 17 00:00:00 2001 From: Thiago de Arruda Date: Thu, 30 Mar 2017 11:11:49 -0300 Subject: [PATCH 2/3] Add tests for `uploadToServer` option. --- spec/api-crash-reporter-spec.js | 89 +++++++++++++++++++++++++++++++++ spec/fixtures/api/crash.html | 12 +++-- 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index dc4b981501..555445ee78 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -1,5 +1,6 @@ const assert = require('assert') const childProcess = require('child_process') +const fs = require('fs') const http = require('http') const multiparty = require('multiparty') const path = require('path') @@ -73,6 +74,93 @@ describe('crashReporter module', function () { }) }) + it('should not send minidump if uploadToServer is false', function (done) { + this.timeout(120000) + + if (process.platform === 'darwin') { + crashReporter.setUploadToServer(false) + } + + let server + let dumpFile + let crashesDir + const testDone = (uploaded) => { + if (uploaded) { + return done(new Error('fail')) + } + server.close() + if (process.platform === 'darwin') { + crashReporter.setUploadToServer(true) + } + assert(fs.existsSync(dumpFile)) + fs.unlinkSync(dumpFile) + done() + } + + let pollInterval + const pollDumpFile = () => { + fs.readdir(crashesDir, (err, files) => { + if (err) { + return + } + let dumps = files.filter((f) => /\.dmp$/.test(f)) + if (!dumps.length) { + return + } + assert.equal(1, dumps.length) + dumpFile = path.join(crashesDir, dumps[0]) + clearInterval(pollInterval) + // dump file should not be deleted when not uploading, so we wait + // 500 ms and assert it still exists in `testDone` + setTimeout(testDone, 500) + }) + } + + remote.ipcMain.once('set-crash-directory', (event, dir) => { + if (process.platform === 'linux') { + crashesDir = dir + } else { + crashesDir = crashReporter.getCrashesDirectory() + if (process.platform === 'darwin') { + // crashpad uses an extra subdirectory + crashesDir = path.join(crashesDir, 'completed') + } + } + + // Before starting, remove all dump files in the crash directory. + // This is required because: + // - mac crashpad not seem to allow changing the crash directory after + // the first "start" call. + // - Other tests in this suite may leave dumps there. + // - We want to verify in `testDone` that a dump file is created and + // not deleted. + fs.readdir(crashesDir, (err, files) => { + if (!err) { + for (let file of files) { + if (/\.dmp$/.test(file)) { + fs.unlinkSync(path.join(crashesDir, file)) + } + } + } + event.returnValue = null // allow the renderer to crash + pollInterval = setInterval(pollDumpFile, 100) + }) + }) + + server = startServer({ + callback (port) { + const crashUrl = url.format({ + protocol: 'file', + pathname: path.join(fixtures, 'api', 'crash.html'), + search: `?port=${port}&skipUpload=1` + }) + w.loadURL(crashUrl) + }, + processType: 'renderer', + done: testDone.bind(null, true) + }) + }) + it('should send minidump with updated extra parameters', function (done) { if (process.env.APPVEYOR === 'True') return done() if (process.env.TRAVIS === 'true') return done() @@ -215,4 +303,5 @@ const startServer = ({callback, processType, done}) => { } callback(port) }) + return server } diff --git a/spec/fixtures/api/crash.html b/spec/fixtures/api/crash.html index 2b1cea6e9a..6f013a2c7b 100644 --- a/spec/fixtures/api/crash.html +++ b/spec/fixtures/api/crash.html @@ -1,20 +1,24 @@ From d677a436ec91eec708e78bfcb1e1d96827dfc6fa Mon Sep 17 00:00:00 2001 From: Kevin Sawicki Date: Fri, 31 Mar 2017 10:42:43 -0700 Subject: [PATCH 3/3] :art: --- atom/common/crash_reporter/crash_reporter_win.cc | 2 +- spec/api-crash-reporter-spec.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index b13108dbfe..a7908ef30c 100644 --- a/atom/common/crash_reporter/crash_reporter_win.cc +++ b/atom/common/crash_reporter/crash_reporter_win.cc @@ -249,7 +249,7 @@ google_breakpad::CustomClientInfo* CrashReporterWin::GetCustomInfo( L"ver", base::UTF8ToWide(version).c_str())); if (!upload_to_server) { custom_info_entries_.push_back(google_breakpad::CustomInfoEntry( - L"skip_upload", L"1")); + L"skip_upload", L"1")); } for (StringMap::const_iterator iter = upload_parameters_.begin(); diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 555445ee78..8a51b5ef2d 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -103,7 +103,7 @@ describe('crashReporter module', function () { if (err) { return } - let dumps = files.filter((f) => /\.dmp$/.test(f)) + const dumps = files.filter((file) => /\.dmp$/.test(file)) if (!dumps.length) { return } @@ -136,7 +136,7 @@ describe('crashReporter module', function () { // not deleted. fs.readdir(crashesDir, (err, files) => { if (!err) { - for (let file of files) { + for (const file of files) { if (/\.dmp$/.test(file)) { fs.unlinkSync(path.join(crashesDir, file)) }