Merge pull request #9053 from electron/enable-crash-reporter-upload-windows

Pass `uploadToServer` option to windows crash reporter

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) {

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<google_breakpad::CustomInfoEntry> custom_info_entries_;

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

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

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
 }

spec/fixtures/api/crash.html

@@ -1,20 +1,24 @@
 <html>
 <body>
 <script type="text/javascript" charset="utf-8">
-var port = require('url').parse(window.location.href, true).query.port;
-var crashReporter = require('electron').crashReporter;
+var url = require('url').parse(window.location.href, true);
+var uploadToServer = !url.query.skipUpload;
+var port = url.query.port;
+var {crashReporter, ipcRenderer} = require('electron');
 crashReporter.start({
   productName: 'Zombies',
   companyName: 'Umbrella Corporation',
   submitURL: 'http://127.0.0.1:' + port,
-  uploadToServer: true,
+  uploadToServer: uploadToServer,
   ignoreSystemCrashHandler: true,
   extra: {
     'extra1': 'extra1',
     'extra2': 'extra2',
   }
 });
-
+if (!uploadToServer) {
+  ipcRenderer.sendSync('set-crash-directory', crashReporter.getCrashesDirectory())
+}
 setImmediate(function() { process.crash(); });
 </script>
 </body>