Merge pull request #10974 from electron/add_crashreport_tests

Update Crash Report Tests

atom/common/api/atom_api_crash_reporter.cc

@@ -31,14 +31,26 @@ struct Converter<CrashReporter::UploadReportResult> {
 
 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<std::string, std::string> GetParameters() {
+  return CrashReporter::GetInstance()->GetParameters();
+}
 
 void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> unused,
                 v8::Local<v8::Context> context, void* priv) {
@@ -46,6 +58,9 @@ void Initialize(v8::Local<v8::Object> exports, v8::Local<v8::Value> 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",

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<std::string, std::string> CrashReporter::GetParameters() const {
+  return upload_parameters_;
+}
+
 #if defined(OS_MACOSX) && defined(MAS_BUILD)
 // static
 CrashReporter* CrashReporter::GetInstance() {

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<std::string, std::string> GetParameters() const;
 
  protected:
   CrashReporter();

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 <map>
 #include <string>
 #include <vector>
 
@@ -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<std::string, std::string> GetParameters() const override;
 
  private:
   friend struct base::DefaultSingletonTraits<CrashReporterMac>;

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<std::string, std::string> CrashReporterMac::GetParameters() const {
+  if (simple_string_dictionary_) {
+    std::map<std::string, std::string> 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<CrashReporter::UploadReportResult>
 CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) {
   std::vector<CrashReporter::UploadReportResult> uploaded_reports;

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
+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.
-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
+### `crashReporter.removeExtraParameter(key)` _macOS_
-`start` you can call `start` again with the updated `extra` options.
+
+* `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
 

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) {
+    if (options == null) options = {}
-      options = {}
-    }
     this.productName = options.productName != null ? options.productName : app.getName()
-    let {companyName, extra, ignoreSystemCrashHandler, submitURL, uploadToServer} = options
 
-    if (uploadToServer == null) {
+    let {
-      // TODO: Remove deprecated autoSubmit property in 2.0
+      companyName,
-      uploadToServer = options.autoSubmit
+      extra,
-    }
+      ignoreSystemCrashHandler,
+      submitURL,
+      uploadToServer
+    } = options
 
-    if (uploadToServer == null) {
+    if (uploadToServer == null) uploadToServer = options.autoSubmit
-      uploadToServer = true
+    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.length > 0) ? reports[0] : null
-      return reports[0]
-    } else {
-      return 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()

spec/api-crash-reporter-spec.js

@@ -35,9 +35,7 @@ describe('crashReporter module', () => {
 
       beforeEach(() => {
         stopServer = null
-        w = new BrowserWindow(Object.assign({
+        w = new BrowserWindow(Object.assign({ show: false }, browserWindowOpts))
-          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('returns true when uploadToServer is set to true', () => {
-    it('can be read/set from the main process', () => {
       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 = () => {

spec/fixtures/api/crash.html

@@ -1,30 +1,34 @@
 <html>
+
 <body>
-<script type="text/javascript" charset="utf-8">
+  <script type="text/javascript" charset="utf-8">
-var url = require('url').parse(window.location.href, true);
+    const url = require('url').parse(window.location.href, true);
-var uploadToServer = !url.query.skipUpload;
+    const uploadToServer = !url.query.skipUpload;
-var port = url.query.port;
+    const port = url.query.port;
-var {crashReporter, ipcRenderer} = require('electron');
+    const {crashReporter, ipcRenderer} = require('electron');
-crashReporter.start({
-  productName: 'Zombies',
-  companyName: 'Umbrella Corporation',
-  submitURL: 'http://127.0.0.1:' + port,
-  uploadToServer: uploadToServer,
-  ignoreSystemCrashHandler: true,
-  extra: {
-    'extra1': 'extra1',
-    'extra2': 'extra2',
-  }
-});
 
-if (process.platform === 'win32') {
+    crashReporter.start({
-  ipcRenderer.sendSync('crash-service-pid', crashReporter._crashServiceProcess.pid)
+      productName: 'Zombies',
-}
+      companyName: 'Umbrella Corporation',
+      submitURL: 'http://127.0.0.1:' + port,
+      uploadToServer: uploadToServer,
+      ignoreSystemCrashHandler: true,
+      extra: {
+        'extra1': 'extra1',
+        'extra2': 'extra2',
+      }
+    })
 
-if (!uploadToServer) {
+    if (process.platform === 'win32') {
-  ipcRenderer.sendSync('list-existing-dumps')
+      ipcRenderer.sendSync('crash-service-pid', crashReporter._crashServiceProcess.pid)
-}
+    }
-setImmediate(function() { process.crash(); });
+
-</script>
+    if (!uploadToServer) {
+      ipcRenderer.sendSync('list-existing-dumps')
+    }
+
+    setImmediate(() => { process.crash() })
+  </script>
 </body>
-</html>
+
+</html>