diff --git a/atom/common/crash_reporter/crash_reporter_win.cc b/atom/common/crash_reporter/crash_reporter_win.cc index 25969dc32e..a7908ef30c 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 diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index dc4b981501..8a51b5ef2d 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 + } + const dumps = files.filter((file) => /\.dmp$/.test(file)) + 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 (const 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 @@