Merge pull request #7500 from electron/store-crashes-in-configured-temp-dir

Store crash reports in configured temp dir

atom/browser/api/atom_api_app.cc

@@ -607,7 +607,7 @@ base::FilePath App::GetPath(mate::Arguments* args, const std::string& name) {
   if (key >= 0)
     succeed = PathService::Get(key, &path);
   if (!succeed)
-    args->ThrowError("Failed to get path");
+    args->ThrowError("Failed to get '" + name + "' path");
   return path;
 }
 
@@ -615,7 +615,7 @@ void App::SetPath(mate::Arguments* args,
                   const std::string& name,
                   const base::FilePath& path) {
   if (!path.IsAbsolute()) {
-    args->ThrowError("path must be absolute");
+    args->ThrowError("Path must be absolute");
     return;
   }
 

atom/common/api/atom_api_crash_reporter.cc

@@ -6,6 +6,7 @@
 #include <string>
 
 #include "atom/common/crash_reporter/crash_reporter.h"
+#include "atom/common/native_mate_converters/file_path_converter.h"
 #include "base/bind.h"
 #include "native_mate/dictionary.h"
 

atom/common/crash_reporter/crash_reporter.cc

@@ -25,13 +25,14 @@ CrashReporter::~CrashReporter() {
 void CrashReporter::Start(const std::string& product_name,
                           const std::string& company_name,
                           const std::string& submit_url,
+                          const base::FilePath& crashes_dir,
                           bool auto_submit,
                           bool skip_system_crash_handler,
                           const StringMap& extra_parameters) {
   SetUploadParameters(extra_parameters);
 
   InitBreakpad(product_name, ATOM_VERSION_STRING, company_name, submit_url,
-               auto_submit, skip_system_crash_handler);
+               crashes_dir, auto_submit, skip_system_crash_handler);
 }
 
 void CrashReporter::SetUploadParameters(const StringMap& parameters) {
@@ -43,11 +44,12 @@ void CrashReporter::SetUploadParameters(const StringMap& parameters) {
 }
 
 std::vector<CrashReporter::UploadReportResult>
-CrashReporter::GetUploadedReports(const std::string& path) {
+CrashReporter::GetUploadedReports(const base::FilePath& crashes_dir) {
   std::string file_content;
   std::vector<CrashReporter::UploadReportResult> result;
-  if (base::ReadFileToString(base::FilePath::FromUTF8Unsafe(path),
+  base::FilePath uploads_path =
-        &file_content)) {
+      crashes_dir.Append(FILE_PATH_LITERAL("uploads.log"));
+  if (base::ReadFileToString(uploads_path, &file_content)) {
     std::vector<std::string> reports = base::SplitString(
         file_content, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
     for (const std::string& report : reports) {
@@ -68,6 +70,7 @@ void CrashReporter::InitBreakpad(const std::string& product_name,
                                  const std::string& version,
                                  const std::string& company_name,
                                  const std::string& submit_url,
+                                 const base::FilePath& crashes_dir,
                                  bool auto_submit,
                                  bool skip_system_crash_handler) {
 }

atom/common/crash_reporter/crash_reporter.h

@@ -10,6 +10,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/files/file_path.h"
 #include "base/macros.h"
 
 namespace crash_reporter {
@@ -24,12 +25,13 @@ class CrashReporter {
   void Start(const std::string& product_name,
              const std::string& company_name,
              const std::string& submit_url,
+             const base::FilePath& crashes_dir,
              bool auto_submit,
              bool skip_system_crash_handler,
              const StringMap& extra_parameters);
 
   virtual std::vector<CrashReporter::UploadReportResult> GetUploadedReports(
-      const std::string& path);
+      const base::FilePath& crashes_dir);
 
  protected:
   CrashReporter();
@@ -39,6 +41,7 @@ class CrashReporter {
                             const std::string& version,
                             const std::string& company_name,
                             const std::string& submit_url,
+                            const base::FilePath& crashes_dir,
                             bool auto_submit,
                             bool skip_system_crash_handler);
   virtual void SetUploadParameters();

atom/common/crash_reporter/crash_reporter_linux.cc

@@ -17,7 +17,6 @@
 #include "base/logging.h"
 #include "base/memory/singleton.h"
 #include "base/process/memory.h"
-#include "base/strings/stringprintf.h"
 #include "vendor/breakpad/src/client/linux/handler/exception_handler.h"
 #include "vendor/breakpad/src/common/linux/linux_libc_support.h"
 
@@ -60,9 +59,10 @@ void CrashReporterLinux::InitBreakpad(const std::string& product_name,
                                       const std::string& version,
                                       const std::string& company_name,
                                       const std::string& submit_url,
+                                      const base::FilePath& crashes_dir,
                                       bool auto_submit,
                                       bool skip_system_crash_handler) {
-  EnableCrashDumping(product_name);
+  EnableCrashDumping(crashes_dir);
 
   crash_keys_.SetKeyValue("prod", ATOM_PRODUCT_NAME);
   crash_keys_.SetKeyValue("ver", version.c_str());
@@ -77,16 +77,13 @@ void CrashReporterLinux::SetUploadParameters() {
   upload_parameters_["platform"] = "linux";
 }
 
-void CrashReporterLinux::EnableCrashDumping(const std::string& product_name) {
+void CrashReporterLinux::EnableCrashDumping(const base::FilePath& crashes_dir) {
-  std::string dump_dir = "/tmp/" + product_name + " Crashes";
+  base::CreateDirectory(crashes_dir);
-  base::FilePath dumps_path(dump_dir);
-  base::CreateDirectory(dumps_path);
 
-  std::string log_file = base::StringPrintf(
+  std::string log_file = crashes_dir.Append("uploads.log").value();
-      "%s/%s", dump_dir.c_str(), "uploads.log");
   strncpy(g_crash_log_path, log_file.c_str(), sizeof(g_crash_log_path));
 
-  MinidumpDescriptor minidump_descriptor(dumps_path.value());
+  MinidumpDescriptor minidump_descriptor(crashes_dir.value());
   minidump_descriptor.set_size_limit(kMaxMinidumpFileSize);
 
   breakpad_.reset(new ExceptionHandler(

atom/common/crash_reporter/crash_reporter_linux.h

@@ -31,6 +31,7 @@ class CrashReporterLinux : public CrashReporter {
                     const std::string& version,
                     const std::string& company_name,
                     const std::string& submit_url,
+                    const base::FilePath& crashes_dir,
                     bool auto_submit,
                     bool skip_system_crash_handler) override;
   void SetUploadParameters() override;
@@ -41,7 +42,7 @@ class CrashReporterLinux : public CrashReporter {
   CrashReporterLinux();
   virtual ~CrashReporterLinux();
 
-  void EnableCrashDumping(const std::string& product_name);
+  void EnableCrashDumping(const base::FilePath& crashes_dir);
 
   static bool CrashDone(const google_breakpad::MinidumpDescriptor& minidump,
                         void* context,

atom/common/crash_reporter/crash_reporter_mac.h

@@ -27,6 +27,7 @@ class CrashReporterMac : public CrashReporter {
                     const std::string& version,
                     const std::string& company_name,
                     const std::string& submit_url,
+                    const base::FilePath& crashes_dir,
                     bool auto_submit,
                     bool skip_system_crash_handler) override;
   void SetUploadParameters() override;
@@ -42,7 +43,7 @@ class CrashReporterMac : public CrashReporter {
                         const base::StringPiece& value);
 
   std::vector<UploadReportResult> GetUploadedReports(
-      const std::string& path) override;
+      const base::FilePath& crashes_dir) override;
 
   std::unique_ptr<crashpad::SimpleStringDictionary> simple_string_dictionary_;
 

atom/common/crash_reporter/crash_reporter_mac.mm

@@ -6,7 +6,6 @@
 
 #include <memory>
 
-#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/mac/bundle_locations.h"
 #include "base/mac/mac_util.h"
@@ -31,15 +30,14 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name,
                                     const std::string& version,
                                     const std::string& company_name,
                                     const std::string& submit_url,
+                                    const base::FilePath& crashes_dir,
                                     bool auto_submit,
                                     bool skip_system_crash_handler) {
-  // check whether crashpad has been initilized.
+  // check whether crashpad has been initialized.
-  // Only need to initilize once.
+  // Only need to initialize once.
   if (simple_string_dictionary_)
     return;
 
-  std::string dump_dir = "/tmp/" + product_name + " Crashes";
-  base::FilePath database_path(dump_dir);
   if (is_browser_) {
     @autoreleasepool {
       base::FilePath framework_bundle_path = base::mac::FrameworkBundlePath();
@@ -47,7 +45,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name,
           framework_bundle_path.Append("Resources").Append("crashpad_handler");
 
       crashpad::CrashpadClient crashpad_client;
-      if (crashpad_client.StartHandler(handler_path, database_path,
+      if (crashpad_client.StartHandler(handler_path, crashes_dir,
                                        submit_url,
                                        StringMap(),
                                        std::vector<std::string>(),
@@ -76,7 +74,7 @@ void CrashReporterMac::InitBreakpad(const std::string& product_name,
   }
   if (is_browser_) {
     std::unique_ptr<crashpad::CrashReportDatabase> database =
-        crashpad::CrashReportDatabase::Initialize(database_path);
+        crashpad::CrashReportDatabase::Initialize(crashes_dir);
     if (database) {
       database->GetSettings()->SetUploadsEnabled(auto_submit);
     }
@@ -93,16 +91,15 @@ void CrashReporterMac::SetCrashKeyValue(const base::StringPiece& key,
 }
 
 std::vector<CrashReporter::UploadReportResult>
-CrashReporterMac::GetUploadedReports(const std::string& path) {
+CrashReporterMac::GetUploadedReports(const base::FilePath& crashes_dir) {
   std::vector<CrashReporter::UploadReportResult> uploaded_reports;
 
-  base::FilePath file_path(path);
+  if (!base::PathExists(crashes_dir)) {
-  if (!base::PathExists(file_path)) {
     return uploaded_reports;
   }
   // Load crashpad database.
   std::unique_ptr<crashpad::CrashReportDatabase> database =
-    crashpad::CrashReportDatabase::Initialize(file_path);
+    crashpad::CrashReportDatabase::Initialize(crashes_dir);
   DCHECK(database);
 
   std::vector<crashpad::CrashReportDatabase::Report> completed_reports;

atom/common/crash_reporter/crash_reporter_win.cc

@@ -149,16 +149,11 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name,
                                     const std::string& version,
                                     const std::string& company_name,
                                     const std::string& submit_url,
+                                    const base::FilePath& crashes_dir,
                                     bool auto_submit,
                                     bool skip_system_crash_handler) {
   skip_system_crash_handler_ = skip_system_crash_handler;
 
-  base::FilePath temp_dir;
-  if (!base::GetTempDir(&temp_dir)) {
-    LOG(ERROR) << "Cannot get temp directory";
-    return;
-  }
-
   base::string16 pipe_name = base::ReplaceStringPlaceholders(
       kPipeNameFormat, base::UTF8ToUTF16(product_name), NULL);
   base::string16 wait_name = base::ReplaceStringPlaceholders(
@@ -177,7 +172,7 @@ void CrashReporterWin::InitBreakpad(const std::string& product_name,
   breakpad_.reset();
 
   breakpad_.reset(new google_breakpad::ExceptionHandler(
-      temp_dir.value(),
+      crashes_dir.DirName().value(),
       FilterCallback,
       MinidumpCallback,
       this,

atom/common/crash_reporter/crash_reporter_win.h

@@ -27,6 +27,7 @@ class CrashReporterWin : public CrashReporter {
                     const std::string& version,
                     const std::string& company_name,
                     const std::string& submit_url,
+                    const base::FilePath& crashes_dir,
                     bool auto_submit,
                     bool skip_system_crash_handler) override;
   void SetUploadParameters() override;

atom/common/crash_reporter/win/crash_service_main.cc

@@ -16,6 +16,7 @@ namespace crash_service {
 namespace {
 
 const char kApplicationName[] = "application-name";
+const char kCrashesDirectory[] = "crashes-directory";
 
 const wchar_t kPipeNameFormat[] = L"\\\\.\\pipe\\$1 Crash Service";
 const wchar_t kStandardLogFile[] = L"operation_log.txt";
@@ -25,17 +26,11 @@ void InvalidParameterHandler(const wchar_t*, const wchar_t*, const wchar_t*,
   // noop.
 }
 
-bool GetCrashServiceDirectory(const std::wstring& application_name,
+bool CreateCrashServiceDirectory(const base::FilePath& temp_dir) {
-                              base::FilePath* dir) {
-  base::FilePath temp_dir;
-  if (!base::GetTempDir(&temp_dir))
-    return false;
-  temp_dir = temp_dir.Append(application_name + L" Crashes");
   if (!base::PathExists(temp_dir)) {
     if (!base::CreateDirectory(temp_dir))
       return false;
   }
-  *dir = temp_dir;
   return true;
 }
 
@@ -59,9 +54,16 @@ int Main(const wchar_t* cmd) {
   std::wstring application_name = cmd_line.GetSwitchValueNative(
       kApplicationName);
 
+  if (!cmd_line.HasSwitch(kCrashesDirectory)) {
+    LOG(ERROR) << "Crashes directory path must be specified with --"
+               << kCrashesDirectory;
+    return 1;
+  }
+
   // We use/create a directory under the user's temp folder, for logging.
-  base::FilePath operating_dir;
+  base::FilePath operating_dir(
-  GetCrashServiceDirectory(application_name, &operating_dir);
+      cmd_line.GetSwitchValueNative(kCrashesDirectory));
+  CreateCrashServiceDirectory(operating_dir);
   base::FilePath log_file = operating_dir.Append(kStandardLogFile);
 
   // Logging to stderr (to help with debugging failures on the

docs/api/crash-reporter.md

@@ -22,6 +22,12 @@ following projects:
 * [socorro](https://github.com/mozilla/socorro)
 * [mini-breakpad-server](https://github.com/electron/mini-breakpad-server)
 
+Crash reports are saved locally in an application-specific temp directory folder.
+For a `productName` of `YourName`, crash reports will be stored in a folder
+named `YourName Crashes` inside the temp directory. You can customize this temp
+directory location for your app by calling the `app.setPath('temp', '/my/custom/temp')`
+API before starting the crash reporter.
+
 ## Methods
 
 The `crash-reporter` module has the following methods:

lib/common/api/crash-reporter.js

@@ -1,30 +1,20 @@
 'use strict'
 
+const {spawn} = require('child_process')
 const os = require('os')
 const path = require('path')
-const spawn = require('child_process').spawn
 const electron = require('electron')
+const {app} = process.type === 'browser' ? electron : electron.remote
 const binding = process.atomBinding('crash_reporter')
 
-var CrashReporter = (function () {
+class CrashReporter {
-  function CrashReporter () {}
+  start (options) {
-
-  CrashReporter.prototype.start = function (options) {
-    var app, args, autoSubmit, companyName, env, extra, ignoreSystemCrashHandler, start, submitURL
     if (options == null) {
       options = {}
     }
-    this.productName = options.productName
+    this.productName = options.productName != null ? options.productName : app.getName()
-    companyName = options.companyName
+    let {autoSubmit, companyName, extra, ignoreSystemCrashHandler, submitURL} = options
-    submitURL = options.submitURL
-    autoSubmit = options.autoSubmit
-    ignoreSystemCrashHandler = options.ignoreSystemCrashHandler
-    extra = options.extra
 
-    app = (process.type === 'browser' ? electron : electron.remote).app
-    if (this.productName == null) {
-      this.productName = app.getName()
-    }
     if (autoSubmit == null) {
       autoSubmit = true
     }
@@ -35,7 +25,7 @@ var CrashReporter = (function () {
       extra = {}
     }
     if (extra._productName == null) {
-      extra._productName = this.productName
+      extra._productName = this.getProductName()
     }
     if (extra._companyName == null) {
       extra._companyName = companyName
@@ -49,12 +39,15 @@ var CrashReporter = (function () {
     if (submitURL == null) {
       throw new Error('submitURL is a required option to crashReporter.start')
     }
-    start = () => {
+
-      binding.start(this.productName, companyName, submitURL, autoSubmit, ignoreSystemCrashHandler, extra)
-    }
     if (process.platform === 'win32') {
-      args = ['--reporter-url=' + submitURL, '--application-name=' + this.productName, '--v=1']
+      const args = [
-      env = {
+        '--reporter-url=' + submitURL,
+        '--application-name=' + this.getProductName(),
+        '--crashes-directory=' + this.getCrashesDirectory(),
+        '--v=1'
+      ]
+      const env = {
         ELECTRON_INTERNAL_CRASH_SERVICE: 1
       }
       spawn(process.execPath, args, {
@@ -62,12 +55,12 @@ var CrashReporter = (function () {
         detached: true
       })
     }
-    return start()
+
+    binding.start(this.getProductName(), companyName, submitURL, this.getCrashesDirectory(), autoSubmit, ignoreSystemCrashHandler, extra)
   }
 
-  CrashReporter.prototype.getLastCrashReport = function () {
+  getLastCrashReport () {
-    var reports
+    const reports = this.getUploadedReports()
-    reports = this.getUploadedReports()
     if (reports.length > 0) {
       return reports[0]
     } else {
@@ -75,14 +68,33 @@ var CrashReporter = (function () {
     }
   }
 
-  CrashReporter.prototype.getUploadedReports = function () {
+  getUploadedReports () {
-    var log, tmpdir
+    return binding._getUploadedReports(this.getCrashesDirectory())
-    tmpdir = process.platform === 'win32' ? os.tmpdir() : '/tmp'
-    log = process.platform === 'darwin' ? path.join(tmpdir, this.productName + ' Crashes') : path.join(tmpdir, this.productName + ' Crashes', 'uploads.log')
-    return binding._getUploadedReports(log)
   }
 
-  return CrashReporter
+  getCrashesDirectory () {
-})()
+    const crashesDir = this.getProductName() + ' Crashes'
+    return path.join(this.getTempDirectory(), crashesDir)
+  }
+
+  getProductName () {
+    if (this.productName == null) {
+      this.productName = app.getName()
+    }
+    return this.productName
+  }
+
+  getTempDirectory () {
+    if (this.tempDirectory == null) {
+      try {
+        this.tempDirectory = app.getPath('temp')
+      } catch (error) {
+        // app.getPath may throw so fallback to OS temp directory
+        this.tempDirectory = os.tmpdir()
+      }
+    }
+    return this.tempDirectory
+  }
+}
 
 module.exports = new CrashReporter()

spec/api-app-spec.js

@@ -334,4 +334,23 @@ describe('app module', function () {
       assert.equal(typeof app.isAccessibilitySupportEnabled(), 'boolean')
     })
   })
+
+  describe('getPath(name)', function () {
+    it('returns paths that exist', function () {
+      assert.equal(fs.existsSync(app.getPath('exe')), true)
+      assert.equal(fs.existsSync(app.getPath('home')), true)
+      assert.equal(fs.existsSync(app.getPath('temp')), true)
+    })
+
+    it('throws an error when the name is invalid', function () {
+      assert.throws(function () {
+        app.getPath('does-not-exist')
+      }, /Failed to get 'does-not-exist' path/)
+    })
+
+    it('returns the overridden path', function () {
+      app.setPath('music', __dirname)
+      assert.equal(app.getPath('music'), __dirname)
+    })
+  })
 })

spec/api-crash-reporter-spec.js

@@ -2,25 +2,30 @@ const assert = require('assert')
 const http = require('http')
 const multiparty = require('multiparty')
 const path = require('path')
+const temp = require('temp').track()
 const url = require('url')
 const {closeWindow} = require('./window-helpers')
 
-const remote = require('electron').remote
+const {remote} = require('electron')
-const app = remote.require('electron').app
+const {app, BrowserWindow, crashReporter} = remote.require('electron')
-const crashReporter = remote.require('electron').crashReporter
-const BrowserWindow = remote.require('electron').BrowserWindow
 
 describe('crashReporter module', function () {
   var fixtures = path.resolve(__dirname, 'fixtures')
   var w = null
+  var originalTempDirectory = null
+  var tempDirectory = null
 
   beforeEach(function () {
     w = new BrowserWindow({
       show: false
     })
+    tempDirectory = temp.mkdirSync('electronCrashReporterSpec-')
+    originalTempDirectory = app.getPath('temp')
+    app.setPath('temp', tempDirectory)
   })
 
   afterEach(function () {
+    app.setPath('temp', originalTempDirectory)
     return closeWindow(w).then(function () { w = null })
   })
 
@@ -54,11 +59,14 @@ describe('crashReporter module', function () {
         assert.equal(fields['_companyName'], 'Umbrella Corporation')
         assert.equal(fields['_version'], app.getVersion())
 
-        res.end('abc-123-def', () => {
+        const reportId = 'abc-123-def-456-abc-789-abc-123-abcd'
-          assert.equal(crashReporter.getLastCrashReport().id, 'abc-123-def')
+        res.end(reportId, () => {
-          assert.notEqual(crashReporter.getUploadedReports().length, 0)
+          waitForCrashReport().then(() => {
-          assert.equal(crashReporter.getUploadedReports()[0].id, 'abc-123-def')
+            assert.equal(crashReporter.getLastCrashReport().id, reportId)
-          done()
+            assert.notEqual(crashReporter.getUploadedReports().length, 0)
+            assert.equal(crashReporter.getUploadedReports()[0].id, reportId)
+            done()
+          }, done)
         })
       })
     })
@@ -110,3 +118,20 @@ describe('crashReporter module', function () {
     })
   })
 })
+
+const waitForCrashReport = () => {
+  return new Promise((resolve, reject) => {
+    let times = 0
+    const checkForReport = () => {
+      if (crashReporter.getLastCrashReport() != null) {
+        resolve()
+      } else if (times >= 10) {
+        reject(new Error('No crash report available'))
+      } else {
+        times++
+        setTimeout(checkForReport, 100)
+      }
+    }
+    checkForReport()
+  })
+}

spec/static/main.js

@@ -41,7 +41,7 @@ ipcMain.on('message', function (event, ...args) {
 })
 
 // Set productName so getUploadedReports() uses the right directory in specs
-if (process.platform === 'win32') {
+if (process.platform !== 'darwin') {
   crashReporter.productName = 'Zombies'
 }