Merge pull request #8648 from electron/set-extra-crash-reporter-data

Support updating crash reporter extra parameters

atom/common/api/atom_api_crash_reporter.cc

@@ -31,19 +31,27 @@ struct Converter<CrashReporter::UploadReportResult> {
 
 namespace {
 
+void SetExtraParameter(const std::string& key, mate::Arguments* args) {
+  std::string value;
+  if (args->GetNext(&value))
+    CrashReporter::GetInstance()->SetExtraParameter(key, value);
+  else
+    CrashReporter::GetInstance()->RemoveExtraParameter(key);
+}
+
 
 void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
                 v8::Local<v8::Context> context, void* priv) {
   mate::Dictionary dict(context->GetIsolate(), exports);
-  auto report = base::Unretained(CrashReporter::GetInstance());
-  dict.SetMethod("start",
-                 base::Bind(&CrashReporter::Start, report));
-  dict.SetMethod("_getUploadedReports",
-                 base::Bind(&CrashReporter::GetUploadedReports, report));
-  dict.SetMethod("_setUploadToServer",
-                 base::Bind(&CrashReporter::SetUploadToServer, report));
-  dict.SetMethod("_getUploadToServer",
-                 base::Bind(&CrashReporter::GetUploadToServer, report));
+  auto reporter = base::Unretained(CrashReporter::GetInstance());
+  dict.SetMethod("start", base::Bind(&CrashReporter::Start, reporter));
+  dict.SetMethod("setExtraParameter", &SetExtraParameter);
+  dict.SetMethod("getUploadedReports",
+                 base::Bind(&CrashReporter::GetUploadedReports, reporter));
+  dict.SetMethod("setUploadToServer",
+                 base::Bind(&CrashReporter::SetUploadToServer, reporter));
+  dict.SetMethod("getUploadToServer",
+                 base::Bind(&CrashReporter::GetUploadToServer, reporter));
 }
 
 }  // namespace

atom/common/crash_reporter/crash_reporter.cc

@@ -86,6 +86,13 @@ void CrashReporter::InitBreakpad(const std::string& product_name,
 void CrashReporter::SetUploadParameters() {
 }
 
+void CrashReporter::SetExtraParameter(const std::string& key,
+                                      const std::string& value) {
+}
+
+void CrashReporter::RemoveExtraParameter(const std::string& key) {
+}
+
 #if defined(OS_MACOSX) && defined(MAS_BUILD)
 // static
 CrashReporter* CrashReporter::GetInstance() {

atom/common/crash_reporter/crash_reporter.h

@@ -37,6 +37,9 @@ class CrashReporter {
 
   virtual void SetUploadToServer(bool upload_to_server);
   virtual bool GetUploadToServer();
+  virtual void SetExtraParameter(const std::string& key,
+                                 const std::string& value);
+  virtual void RemoveExtraParameter(const std::string& key);
 
  protected:
   CrashReporter();

atom/common/crash_reporter/crash_reporter_linux.cc

@@ -64,13 +64,15 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name,
                                       bool skip_system_crash_handler) {
   EnableCrashDumping(crashes_dir);
 
-  crash_keys_.SetKeyValue("prod", ATOM_PRODUCT_NAME);
-  crash_keys_.SetKeyValue("ver", version.c_str());
+  crash_keys_.reset(new CrashKeyStorage());
+
+  crash_keys_->SetKeyValue("prod", ATOM_PRODUCT_NAME);
+  crash_keys_->SetKeyValue("ver", version.c_str());
   upload_url_ = submit_url;
 
   for (StringMap::const_iterator iter = upload_parameters_.begin();
        iter != upload_parameters_.end(); ++iter)
-    crash_keys_.SetKeyValue(iter->first.c_str(), iter->second.c_str());
+    crash_keys_->SetKeyValue(iter->first.c_str(), iter->second.c_str());
 }
 
 void CrashReporterLinux::SetUploadParameters() {
@@ -120,7 +122,7 @@ bool CrashReporterLinux::CrashDone(const MinidumpDescriptor& minidump,
   info.oom_size = base::g_oom_size;
   info.pid = self->pid_;
   info.upload_url = self->upload_url_.c_str();
-  info.crash_keys = &self->crash_keys_;
+  info.crash_keys = self->crash_keys_.get();
   HandleCrashDump(info);
   return true;
 }

atom/common/crash_reporter/crash_reporter_linux.h

@@ -49,7 +49,7 @@ class CrashReporterLinux : public CrashReporter {
                         const bool succeeded);
 
   std::unique_ptr<google_breakpad::ExceptionHandler> breakpad_;
-  CrashKeyStorage crash_keys_;
+  std::unique_ptr<CrashKeyStorage> crash_keys_;
 
   uint64_t process_start_time_;
   pid_t pid_;

atom/common/crash_reporter/crash_reporter_mac.h

@@ -34,6 +34,9 @@ class CrashReporterMac : public CrashReporter {
   void SetUploadParameters() override;
   void SetUploadToServer(bool upload_to_server) override;
   bool GetUploadToServer() override;
+  void SetExtraParameter(const std::string& key,
+                         const std::string& value) override;
+  void RemoveExtraParameter(const std::string& key) override;
 
  private:
   friend struct base::DefaultSingletonTraits<CrashReporterMac>;

atom/common/crash_reporter/crash_reporter_mac.mm

@@ -100,6 +100,21 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key,
   simple_string_dictionary_->SetKeyValue(key.data(), value.data());
 }
 
+void CrashReporterMac::SetExtraParameter(const std::string& key,
+                                         const std::string& value) {
+  if (simple_string_dictionary_)
+    SetCrashKeyValue(key, value);
+  else
+    upload_parameters_[key] = value;
+}
+
+void CrashReporterMac::RemoveExtraParameter(const std::string& key) {
+  if (simple_string_dictionary_)
+    simple_string_dictionary_->RemoveKey(key.data());
+  else
+    upload_parameters_.erase(key);
+}
+
 std::vector<CrashReporter::UploadReportResult>
 CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) {
   std::vector<CrashReporter::UploadReportResult> uploaded_reports;

docs/api/crash-reporter.md

@@ -44,14 +44,14 @@ The `crashReporter` module has the following methods:
     Default is `true`.
   * `ignoreSystemCrashHandler` Boolean (optional) - Default is `false`.
   * `extra` Object (optional) - An object you can define that will be sent along with the
-    report. Only string properties are sent correctly, Nested objects are not
+    report. Only string properties are sent correctly. Nested objects are not
     supported.
 
 You are required to call this method before using any other `crashReporter` APIs
 and in each process (main/renderer) from which you want to collect crash reports.
 You can pass different options to `crashReporter.start` when calling from different processes.
 
-**Note** Child processes created via the `child_process` module will not have access to the Electron modules. 
+**Note** Child processes created via the `child_process` module will not have access to the Electron modules.
 Therefore, to collect crash reports from them, use `process.crashReporter.start` instead. Pass the same options as above
 along with an additional one called `crashesDirectory` that should point to a directory to store the crash
 reports temporarily. You can test this out by calling `process.crash()` to crash the child process.
@@ -60,6 +60,10 @@ reports temporarily. You can test this out by calling `process.crash()` to crash
 This will start the process that will monitor and send the crash reports. Replace `submitURL`, `productName`
 and `crashesDirectory` with appropriate values.
 
+**Note:** If you need send additional/updated `extra` parameters after your
+first call `start` you can call `setExtraParameter` on macOS or call `start`
+again with the new/updated `extra` parameters on Linux and Windows.
+
 ```js
  const args = [
    `--reporter-url=${submitURL}`,
@@ -111,6 +115,18 @@ called before `start` is called.
 
 **Note:** This API can only be called from the main process.
 
+### `crashReporter.setExtraParameter(key, value)` _macOS_
+
+* `key` String - Parameter key.
+* `value` String - Parameter value. Specifying `null` or `undefined` will
+  remove the key from the extra parameters.
+
+Set an extra parameter to set 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.
+
 ## Crash Report Payload
 
 The crash reporter will send the following data to the `submitURL` as

lib/common/api/crash-reporter.js

@@ -75,7 +75,7 @@ class CrashReporter {
   }
 
   getUploadedReports () {
-    return binding._getUploadedReports(this.getCrashesDirectory())
+    return binding.getUploadedReports(this.getCrashesDirectory())
   }
 
   getCrashesDirectory () {
@@ -104,7 +104,7 @@ class CrashReporter {
 
   getUploadToServer () {
     if (process.type === 'browser') {
-      return binding._getUploadToServer()
+      return binding.getUploadToServer()
     } else {
       throw new Error('getUploadToServer can only be called from the main process')
     }
@@ -112,11 +112,15 @@ class CrashReporter {
 
   setUploadToServer (uploadToServer) {
     if (process.type === 'browser') {
-      return binding._setUploadToServer(uploadToServer)
+      return binding.setUploadToServer(uploadToServer)
     } else {
       throw new Error('setUploadToServer can only be called from the main process')
     }
   }
+
+  setExtraParameter (key, value) {
+    binding.setExtraParameter(key, value)
+  }
 }
 
 module.exports = new CrashReporter()

spec/api-crash-reporter-spec.js

@@ -35,7 +35,7 @@ describe('crashReporter module', function () {
   }
 
   it('should send minidump when renderer crashes', function (done) {
-    if (process.platform !== 'darwin') return done()
+    if (process.env.APPVEYOR === 'True') return done()
     if (process.env.TRAVIS === 'true') return done()
 
     this.timeout(120000)
@@ -55,7 +55,7 @@ describe('crashReporter module', function () {
   })
 
   it('should send minidump when node processes crash', function (done) {
-    if (process.platform !== 'darwin') return done()
+    if (process.env.APPVEYOR === 'True') return done()
     if (process.env.TRAVIS === 'true') return done()
 
     this.timeout(120000)
@@ -72,6 +72,26 @@ describe('crashReporter module', function () {
     })
   })
 
+  it('should send minidump with updated extra parameters', function (done) {
+    if (process.env.APPVEYOR === 'True') return done()
+    if (process.env.TRAVIS === 'true') return done()
+
+    this.timeout(10000)
+
+    startServer({
+      callback (port) {
+        const crashUrl = url.format({
+          protocol: 'file',
+          pathname: path.join(fixtures, 'api', 'crash-restart.html'),
+          search: '?port=' + port
+        })
+        w.loadURL(crashUrl)
+      },
+      processType: 'renderer',
+      done: done
+    })
+  })
+
   describe('.start(options)', function () {
     it('requires that the companyName and submitURL options be specified', function () {
       assert.throws(function () {
@@ -111,7 +131,7 @@ describe('crashReporter module', function () {
         crashReporter.start({
           companyName: 'Umbrella Corporation',
           submitURL: 'http://127.0.0.1/crashes',
-          autoSubmit: true
+          uploadToServer: true
         })
         assert.equal(crashReporter.getUploadToServer(), true)
         crashReporter.setUploadToServer(false)
@@ -155,6 +175,7 @@ const startServer = ({callback, processType, done}) => {
       assert.equal(fields.platform, process.platform)
       assert.equal(fields.extra1, 'extra1')
       assert.equal(fields.extra2, 'extra2')
+      assert.equal(fields.extra3, undefined)
       assert.equal(fields._productName, 'Zombies')
       assert.equal(fields._companyName, 'Umbrella Corporation')
       assert.equal(fields._version, app.getVersion())
@@ -165,6 +186,7 @@ const startServer = ({callback, processType, done}) => {
           assert.equal(crashReporter.getLastCrashReport().id, reportId)
           assert.notEqual(crashReporter.getUploadedReports().length, 0)
           assert.equal(crashReporter.getUploadedReports()[0].id, reportId)
+          req.socket.destroy()
           done()
         }, done)
       })

spec/fixtures/api/crash-restart.html

@@ -0,0 +1,46 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+
+const {port} = require('url').parse(window.location.href, true).query
+const {crashReporter} = require('electron')
+
+crashReporter.start({
+  productName: 'Zombies',
+  companyName: 'Umbrella Corporation',
+  submitURL: 'http://127.0.0.1:' + port,
+  uploadToServer: true,
+  ignoreSystemCrashHandler: true,
+  extra: {
+    extra1: 'extra1',
+    extra2: 'initial',
+    extra3: 'extra3'
+  }
+})
+
+setImmediate(() => {
+  if (process.platform === 'darwin') {
+    crashReporter.setExtraParameter('extra2', 'extra2')
+    crashReporter.setExtraParameter('extra3', null)
+  } else {
+    crashReporter.start({
+      productName: 'Zombies',
+      companyName: 'Umbrella Corporation',
+      submitURL: 'http://127.0.0.1:' + port,
+      uploadToServer: true,
+      ignoreSystemCrashHandler: true,
+      extra: {
+        extra1: 'extra1',
+        extra2: 'extra2'
+      }
+    })
+  }
+
+  setImmediate(() => {
+    process.crash()
+  })
+})
+
+</script>
+</body>
+</html>

spec/fixtures/api/crash.html

@@ -7,7 +7,7 @@ crashReporter.start({
   productName: 'Zombies',
   companyName: 'Umbrella Corporation',
   submitURL: 'http://127.0.0.1:' + port,
-  autoSubmit: true,
+  uploadToServer: true,
   ignoreSystemCrashHandler: true,
   extra: {
     'extra1': 'extra1',