diff --git a/atom/common/api/atom_api_crash_reporter.cc b/atom/common/api/atom_api_crash_reporter.cc index 0edd787e558c..f435cdf030b3 100644 --- a/atom/common/api/atom_api_crash_reporter.cc +++ b/atom/common/api/atom_api_crash_reporter.cc @@ -31,14 +31,26 @@ struct Converter { namespace { +// TODO(2.0) Remove void SetExtraParameter(const std::string& key, mate::Arguments* args) { std::string value; if (args->GetNext(&value)) - CrashReporter::GetInstance()->SetExtraParameter(key, value); + CrashReporter::GetInstance()->AddExtraParameter(key, value); else CrashReporter::GetInstance()->RemoveExtraParameter(key); } +void AddExtraParameter(const std::string& key, const std::string& value) { + CrashReporter::GetInstance()->AddExtraParameter(key, value); +} + +void RemoveExtraParameter(const std::string& key) { + CrashReporter::GetInstance()->RemoveExtraParameter(key); +} + +std::map GetParameters() { + return CrashReporter::GetInstance()->GetParameters(); +} void Initialize(v8::Local exports, v8::Local unused, v8::Local context, void* priv) { @@ -46,6 +58,9 @@ void Initialize(v8::Local exports, v8::Local unused, auto reporter = base::Unretained(CrashReporter::GetInstance()); dict.SetMethod("start", base::Bind(&CrashReporter::Start, reporter)); dict.SetMethod("setExtraParameter", &SetExtraParameter); + dict.SetMethod("addExtraParameter", &AddExtraParameter); + dict.SetMethod("removeExtraParameter", &RemoveExtraParameter); + dict.SetMethod("getParameters", &GetParameters); dict.SetMethod("getUploadedReports", base::Bind(&CrashReporter::GetUploadedReports, reporter)); dict.SetMethod("setUploadToServer", diff --git a/atom/common/crash_reporter/crash_reporter.cc b/atom/common/crash_reporter/crash_reporter.cc index d901f83fa49f..88930f0d3ce5 100644 --- a/atom/common/crash_reporter/crash_reporter.cc +++ b/atom/common/crash_reporter/crash_reporter.cc @@ -86,13 +86,17 @@ void CrashReporter::InitBreakpad(const std::string& product_name, void CrashReporter::SetUploadParameters() { } -void CrashReporter::SetExtraParameter(const std::string& key, +void CrashReporter::AddExtraParameter(const std::string& key, const std::string& value) { } void CrashReporter::RemoveExtraParameter(const std::string& key) { } +std::map CrashReporter::GetParameters() const { + return upload_parameters_; +} + #if defined(OS_MACOSX) && defined(MAS_BUILD) // static CrashReporter* CrashReporter::GetInstance() { diff --git a/atom/common/crash_reporter/crash_reporter.h b/atom/common/crash_reporter/crash_reporter.h index 2bdcd9b02d3b..99c4c9818d8c 100644 --- a/atom/common/crash_reporter/crash_reporter.h +++ b/atom/common/crash_reporter/crash_reporter.h @@ -37,9 +37,10 @@ class CrashReporter { virtual void SetUploadToServer(bool upload_to_server); virtual bool GetUploadToServer(); - virtual void SetExtraParameter(const std::string& key, + virtual void AddExtraParameter(const std::string& key, const std::string& value); virtual void RemoveExtraParameter(const std::string& key); + virtual std::map GetParameters() const; protected: CrashReporter(); diff --git a/atom/common/crash_reporter/crash_reporter_mac.h b/atom/common/crash_reporter/crash_reporter_mac.h index 583c99a86e9c..c1b2431af6f5 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.h +++ b/atom/common/crash_reporter/crash_reporter_mac.h @@ -5,6 +5,7 @@ #ifndef ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_MAC_H_ #define ATOM_COMMON_CRASH_REPORTER_CRASH_REPORTER_MAC_H_ +#include #include #include @@ -34,9 +35,10 @@ class CrashReporterMac : public CrashReporter { void SetUploadParameters() override; void SetUploadToServer(bool upload_to_server) override; bool GetUploadToServer() override; - void SetExtraParameter(const std::string& key, + void AddExtraParameter(const std::string& key, const std::string& value) override; void RemoveExtraParameter(const std::string& key) override; + std::map GetParameters() const override; private: friend struct base::DefaultSingletonTraits; diff --git a/atom/common/crash_reporter/crash_reporter_mac.mm b/atom/common/crash_reporter/crash_reporter_mac.mm index 990e1b3b1957..3f82d2466fb4 100644 --- a/atom/common/crash_reporter/crash_reporter_mac.mm +++ b/atom/common/crash_reporter/crash_reporter_mac.mm @@ -105,12 +105,13 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key, simple_string_dictionary_->SetKeyValue(key.data(), value.data()); } -void CrashReporterMac::SetExtraParameter(const std::string& key, +void CrashReporterMac::AddExtraParameter(const std::string& key, const std::string& value) { - if (simple_string_dictionary_) + if (simple_string_dictionary_) { SetCrashKeyValue(key, value); - else + } else { upload_parameters_[key] = value; + } } void CrashReporterMac::RemoveExtraParameter(const std::string& key) { @@ -120,6 +121,20 @@ void CrashReporterMac::RemoveExtraParameter(const std::string& key) { upload_parameters_.erase(key); } +std::map CrashReporterMac::GetParameters() const { + if (simple_string_dictionary_) { + std::map ret; + crashpad::SimpleStringDictionary::Iterator iter(*simple_string_dictionary_); + for(;;) { + const auto entry = iter.Next(); + if (!entry) break; + ret[entry->key] = entry->value; + } + return ret; + } + return upload_parameters_; +} + std::vector CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) { std::vector uploaded_reports; diff --git a/docs/api/crash-reporter.md b/docs/api/crash-reporter.md index 0c1c0fdec0ef..46c32f5632a3 100644 --- a/docs/api/crash-reporter.md +++ b/docs/api/crash-reporter.md @@ -116,18 +116,23 @@ called before `start` is called. **Note:** This API can only be called from the main process. -### `crashReporter.setExtraParameter(key, value)` _macOS_ +### `crashReporter.addExtraParameter(key, value)` _macOS_ * `key` String - Parameter key, must be less than 64 characters long. * `value` String - Parameter value, must be less than 64 characters long. - Specifying `null` or `undefined` will remove the key from the extra - parameters. Set an extra parameter to be sent with the crash report. The values -specified here will be sent in addition to any values set via the `extra` option -when `start` was called. This API is only available on macOS, if you need to -add/update extra parameters on Linux and Windows after your first call to -`start` you can call `start` again with the updated `extra` options. +specified here will be sent in addition to any values set via the `extra` option when `start` was called. This API is only available on macOS, if you need to add/update extra parameters on Linux and Windows after your first call to `start` you can call `start` again with the updated `extra` options. + +### `crashReporter.removeExtraParameter(key)` _macOS_ + +* `key` String - Parameter key, must be less than 64 characters long. + +Remove a extra parameter from the current set of parameters so that it will not be sent with the crash report. + +### `crashReporter.getParameters()` + +See all of the current parameters being passed to the crash reporter. ## Crash Report Payload diff --git a/lib/common/api/crash-reporter.js b/lib/common/api/crash-reporter.js index 7a54e24fbc2a..cc284e8aceca 100644 --- a/lib/common/api/crash-reporter.js +++ b/lib/common/api/crash-reporter.js @@ -4,41 +4,31 @@ 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 {app, deprecate} = process.type === 'browser' ? electron : electron.remote const binding = process.atomBinding('crash_reporter') class CrashReporter { start (options) { - if (options == null) { - options = {} - } + if (options == null) options = {} this.productName = options.productName != null ? options.productName : app.getName() - let {companyName, extra, ignoreSystemCrashHandler, submitURL, uploadToServer} = options - if (uploadToServer == null) { - // TODO: Remove deprecated autoSubmit property in 2.0 - uploadToServer = options.autoSubmit - } + let { + companyName, + extra, + ignoreSystemCrashHandler, + submitURL, + uploadToServer + } = options - if (uploadToServer == null) { - uploadToServer = true - } + if (uploadToServer == null) uploadToServer = options.autoSubmit + if (uploadToServer == null) uploadToServer = true + if (ignoreSystemCrashHandler == null) ignoreSystemCrashHandler = false + if (extra == null) extra = {} + + if (extra._productName == null) extra._productName = this.getProductName() + if (extra._companyName == null) extra._companyName = companyName + if (extra._version == null) extra._version = app.getVersion() - if (ignoreSystemCrashHandler == null) { - ignoreSystemCrashHandler = false - } - if (extra == null) { - extra = {} - } - if (extra._productName == null) { - extra._productName = this.getProductName() - } - if (extra._companyName == null) { - extra._companyName = companyName - } - if (extra._version == null) { - extra._version = app.getVersion() - } if (companyName == null) { throw new Error('companyName is a required option to crashReporter.start') } @@ -47,15 +37,16 @@ class CrashReporter { } if (process.platform === 'win32') { + const env = { + ELECTRON_INTERNAL_CRASH_SERVICE: 1 + } const args = [ '--reporter-url=' + submitURL, '--application-name=' + this.getProductName(), '--crashes-directory=' + this.getCrashesDirectory(), '--v=1' ] - const env = { - ELECTRON_INTERNAL_CRASH_SERVICE: 1 - } + this._crashServiceProcess = spawn(process.execPath, args, { env: env, detached: true @@ -67,11 +58,7 @@ class CrashReporter { getLastCrashReport () { const reports = this.getUploadedReports() - if (reports.length > 0) { - return reports[0] - } else { - return null - } + return (reports.length > 0) ? reports[0] : null } getUploadedReports () { @@ -79,7 +66,7 @@ class CrashReporter { } getCrashesDirectory () { - const crashesDir = this.getProductName() + ' Crashes' + const crashesDir = `${this.getProductName()} Crashes` return path.join(this.getTempDirectory(), crashesDir) } @@ -95,7 +82,6 @@ class CrashReporter { try { this.tempDirectory = app.getPath('temp') } catch (error) { - // app.getPath may throw so fallback to OS temp directory this.tempDirectory = os.tmpdir() } } @@ -118,9 +104,26 @@ class CrashReporter { } } + // TODO(2.0) Remove setExtraParameter (key, value) { + if (!process.noDeprecations) { + deprecate.warn('crashReporter.setExtraParameter', + 'crashReporter.addExtraParameter or crashReporter.removeExtraParameter') + } binding.setExtraParameter(key, value) } + + addExtraParameter (key, value) { + binding.addExtraParameter(key, value) + } + + removeExtraParameter (key) { + binding.removeExtraParameter(key) + } + + getParameters (key, value) { + return binding.getParameters() + } } module.exports = new CrashReporter() diff --git a/spec/api-crash-reporter-spec.js b/spec/api-crash-reporter-spec.js index 0598677fea26..abc3b15498f0 100644 --- a/spec/api-crash-reporter-spec.js +++ b/spec/api-crash-reporter-spec.js @@ -35,9 +35,7 @@ describe('crashReporter module', () => { beforeEach(() => { stopServer = null - w = new BrowserWindow(Object.assign({ - show: false - }, browserWindowOpts)) + w = new BrowserWindow(Object.assign({ show: false }, browserWindowOpts)) }) afterEach(() => closeWindow(w).then(() => { w = null })) @@ -73,7 +71,6 @@ describe('crashReporter module', () => { done: done }) }) - it('should send minidump when node processes crash', function (done) { if (process.env.APPVEYOR === 'True') return done() if (process.env.TRAVIS === 'true') return done() @@ -106,7 +103,6 @@ describe('crashReporter module', () => { done: done }) }) - it('should not send minidump if uploadToServer is false', function (done) { this.timeout(120000) @@ -168,7 +164,6 @@ describe('crashReporter module', () => { 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() @@ -180,12 +175,12 @@ describe('crashReporter module', () => { const crashUrl = url.format({ protocol: 'file', pathname: path.join(fixtures, 'api', 'crash-restart.html'), - search: '?port=' + port + search: `?port=${port}` }) w.loadURL(crashUrl) }, processType: 'renderer', - done: done + done: done() }) }) }) @@ -199,7 +194,22 @@ describe('crashReporter module', () => { } }) - describe('.start(options)', () => { + describe('getProductName', () => { + it('returns the product name if one is specified', () => { + const name = crashReporter.getProductName() + const expectedName = (process.platform === 'darwin') ? 'Electron Test' : 'Zombies' + assert.equal(name, expectedName) + }) + }) + + describe('getTempDirectory', () => { + it('returns temp directory for app if one is specified', () => { + const tempDir = crashReporter.getTempDirectory() + assert.equal(tempDir, app.getPath('temp')) + }) + }) + + describe('start(options)', () => { it('requires that the companyName and submitURL options be specified', () => { assert.throws(() => { crashReporter.start({companyName: 'Missing submitURL'}) @@ -208,7 +218,6 @@ describe('crashReporter module', () => { crashReporter.start({submitURL: 'Missing companyName'}) }, /companyName is a required option to crashReporter\.start/) }) - it('can be called multiple times', () => { assert.doesNotThrow(() => { crashReporter.start({ @@ -224,12 +233,41 @@ describe('crashReporter module', () => { }) }) - describe('.get/setUploadToServer', () => { + describe('getCrashesDirectory', () => { + it('correctly returns the directory', () => { + const crashesDir = crashReporter.getCrashesDirectory() + let dir + if (process.platform === 'win32') { + dir = `${app.getPath('temp')}/Zombies Crashes` + } else { + dir = `${app.getPath('temp')}/Electron Test Crashes` + } + assert.equal(crashesDir, dir) + }) + }) + + describe('getUploadedReports', () => { + it('returns an array of reports', () => { + const reports = crashReporter.getUploadedReports() + assert(typeof reports === 'object') + }) + }) + + describe('getLastCrashReport', () => { + it('correctly returns the most recent report', () => { + if (process.env.TRAVIS === 'False') { + const reports = crashReporter.getUploadedReports() + const lastReport = reports[0] + assert(lastReport != null) + } + }) + }) + + describe('getUploadToServer()', () => { it('throws an error when called from the renderer process', () => { assert.throws(() => require('electron').crashReporter.getUploadToServer()) }) - - it('can be read/set from the main process', () => { + it('returns true when uploadToServer is set to true', () => { if (process.platform === 'darwin') { crashReporter.start({ companyName: 'Umbrella Corporation', @@ -237,13 +275,87 @@ describe('crashReporter module', () => { uploadToServer: true }) assert.equal(crashReporter.getUploadToServer(), true) + } + }) + it('returns false when uploadToServer is set to false', () => { + if (process.platform === 'darwin') { + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes', + uploadToServer: true + }) crashReporter.setUploadToServer(false) assert.equal(crashReporter.getUploadToServer(), false) - } else { + } + }) + }) + + describe('setUploadToServer(uploadToServer)', () => { + it('throws an error when called from the renderer process', () => { + assert.throws(() => require('electron').crashReporter.setUploadToServer('arg')) + }) + it('sets uploadToServer false when called with false', () => { + if (process.platform === 'darwin') { + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes', + uploadToServer: true + }) + crashReporter.setUploadToServer(false) + assert.equal(crashReporter.getUploadToServer(), false) + } + }) + it('sets uploadToServer true when called with true', () => { + if (process.platform === 'darwin') { + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes', + uploadToServer: false + }) + crashReporter.setUploadToServer(true) assert.equal(crashReporter.getUploadToServer(), true) } }) }) + + describe('Parameters', () => { + it('returns all of the current parameters', () => { + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes' + }) + + const parameters = crashReporter.getParameters() + assert(typeof parameters === 'object') + }) + it('adds a parameter to current parameters', () => { + // only run on MacOS + if (process.platform !== 'darwin') return + + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes' + }) + + crashReporter.addExtraParameter('hello', 'world') + assert('hello' in crashReporter.getParameters()) + }) + it('removes a parameter from current parameters', () => { + // only run on MacOS + if (process.platform !== 'darwin') return + + crashReporter.start({ + companyName: 'Umbrella Corporation', + submitURL: 'http://127.0.0.1/crashes' + }) + + crashReporter.addExtraParameter('hello', 'world') + assert('hello' in crashReporter.getParameters()) + + crashReporter.removeExtraParameter('hello') + assert(!('hello' in crashReporter.getParameters())) + }) + }) }) const waitForCrashReport = () => { diff --git a/spec/fixtures/api/crash.html b/spec/fixtures/api/crash.html index 8b64eddd25b7..a3dbeb6ded43 100644 --- a/spec/fixtures/api/crash.html +++ b/spec/fixtures/api/crash.html @@ -1,30 +1,34 @@ + - + if (process.platform === 'win32') { + ipcRenderer.sendSync('crash-service-pid', crashReporter._crashServiceProcess.pid) + } + + if (!uploadToServer) { + ipcRenderer.sendSync('list-existing-dumps') + } + + setImmediate(() => { process.crash() }) + - + + \ No newline at end of file