Don't spawn an upload thread if url is empty

Also automatically stop upload and prune threads on destruction. Bug: crashpad:30 Change-Id: I45a30944eb3052182da296e00a6d6041691ab772 Reviewed-on: https://chromium-review.googlesource.com/924456 Commit-Queue: Joshua Peraza <[email protected]> Reviewed-by: Mark Mentovai <[email protected]>
sbahra · Feb 20, 2018 · ebad8bd · ebad8bd
1 parent d8d0317
commit ebad8bd
Show file tree

Hide file tree

Showing 11 changed files with 134 additions and 54 deletions.
diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc
@@ -58,11 +58,18 @@ CrashReportUploadThread::CrashReportUploadThread(CrashReportDatabase* database,
                                             : WorkerThread::kIndefiniteWait,
               this),
       known_pending_report_uuids_(),
-      database_(database) {}
+      database_(database) {
+  DCHECK(!url_.empty());
+}
 
 CrashReportUploadThread::~CrashReportUploadThread() {
 }
 
+void CrashReportUploadThread::ReportPending(const UUID& report_uuid) {
+  known_pending_report_uuids_.PushBack(report_uuid);
+  thread_.DoWorkNow();
+}
+
 void CrashReportUploadThread::Start() {
   thread_.Start(
       options_.watch_pending_reports ? 0.0 : WorkerThread::kIndefiniteWait);
@@ -72,11 +79,6 @@ void CrashReportUploadThread::Stop() {
   thread_.Stop();
 }
 
-void CrashReportUploadThread::ReportPending(const UUID& report_uuid) {
-  known_pending_report_uuids_.PushBack(report_uuid);
-  thread_.DoWorkNow();
-}
-
 void CrashReportUploadThread::ProcessPendingReports() {
   std::vector<UUID> known_report_uuids = known_pending_report_uuids_.Drain();
   for (const UUID& report_uuid : known_report_uuids) {
@@ -140,9 +142,8 @@ void CrashReportUploadThread::ProcessPendingReport(
   Settings* const settings = database_->GetSettings();
 
   bool uploads_enabled;
-  if (url_.empty() ||
-      (!report.upload_explicitly_requested &&
-       (!settings->GetUploadsEnabled(&uploads_enabled) || !uploads_enabled))) {
+  if (!report.upload_explicitly_requested &&
+      (!settings->GetUploadsEnabled(&uploads_enabled) || !uploads_enabled)) {
     // Don’t attempt an upload if there’s no URL to upload to. Allow upload if
     // it has been explicitly requested by the user, otherwise, respect the
     // upload-enabled state stored in the database’s settings.

diff --git a/handler/crash_report_upload_thread.h b/handler/crash_report_upload_thread.h
@@ -22,6 +22,7 @@
 #include "client/crash_report_database.h"
 #include "util/misc/uuid.h"
 #include "util/stdlib/thread_safe_vector.h"
+#include "util/thread/stoppable.h"
 #include "util/thread/worker_thread.h"
 
 namespace crashpad {
@@ -39,7 +40,8 @@ namespace crashpad {
 //! It also catches reports that are added without a ReportPending() signal
 //! being caught. This may happen if crash reports are added to the database by
 //! other processes.
-class CrashReportUploadThread : public WorkerThread::Delegate {
+class CrashReportUploadThread : public WorkerThread::Delegate,
+                                public Stoppable {
  public:
    //! \brief Options to be passed to the CrashReportUploadThread constructor.
    struct Options {
@@ -70,11 +72,22 @@ class CrashReportUploadThread : public WorkerThread::Delegate {
                           const Options& options);
   ~CrashReportUploadThread();
 
+  //! \brief Informs the upload thread that a new pending report has been added
+  //!     to the database.
+  //!
+  //! \param[in] report_uuid The unique identifier of the newly added pending
+  //!     report.
+  //!
+  //! This method may be called from any thread.
+  void ReportPending(const UUID& report_uuid);
+
+  // Stoppable:
+
   //! \brief Starts a dedicated upload thread, which executes ThreadMain().
   //!
   //! This method may only be be called on a newly-constructed object or after
   //! a call to Stop().
-  void Start();
+  void Start() override;
 
   //! \brief Stops the upload thread.
   //!
@@ -88,16 +101,7 @@ class CrashReportUploadThread : public WorkerThread::Delegate {
   //!
   //! This method may be called from any thread other than the upload thread.
   //! It is expected to only be called from the same thread that called Start().
-  void Stop();
-
-  //! \brief Informs the upload thread that a new pending report has been added
-  //!     to the database.
-  //!
-  //! \param[in] report_uuid The unique identifier of the newly added pending
-  //!     report.
-  //!
-  //! This method may be called from any thread.
-  void ReportPending(const UUID& report_uuid);
+  void Stop() override;
 
  private:
   //! \brief The result code from UploadReport().

diff --git a/handler/handler_main.cc b/handler/handler_main.cc
@@ -417,6 +417,26 @@ void MonitorSelf(const Options& options) {
   ReinstallCrashHandler();
 }
 
+class ScopedStoppable {
+ public:
+  ScopedStoppable() = default;
+
+  ~ScopedStoppable() {
+    if (stoppable_) {
+      stoppable_->Stop();
+    }
+  }
+
+  void Reset(Stoppable* stoppable) { stoppable_.reset(stoppable); }
+
+  Stoppable* Get() { return stoppable_.get(); }
+
+ private:
+  std::unique_ptr<Stoppable> stoppable_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedStoppable);
+};
+
 }  // namespace
 
 int HandlerMain(int argc,
@@ -770,31 +790,35 @@ int HandlerMain(int argc,
     return ExitFailure();
   }
 
-  // TODO(scottmg): options.rate_limit should be removed when we have a
-  // configurable database setting to control upload limiting.
-  // See https://crashpad.chromium.org/bug/23.
-  CrashReportUploadThread::Options upload_thread_options;
-  upload_thread_options.identify_client_via_url =
-      options.identify_client_via_url;
-  upload_thread_options.rate_limit = options.rate_limit;
-  upload_thread_options.upload_gzip = options.upload_gzip;
-  upload_thread_options.watch_pending_reports = options.periodic_tasks;
-  CrashReportUploadThread upload_thread(database.get(),
-                                        options.url,
-                                        upload_thread_options);
-  upload_thread.Start();
-
-  std::unique_ptr<PruneCrashReportThread> prune_thread;
+  ScopedStoppable upload_thread;
+  if (!options.url.empty()) {
+    // TODO(scottmg): options.rate_limit should be removed when we have a
+    // configurable database setting to control upload limiting.
+    // See https://crashpad.chromium.org/bug/23.
+    CrashReportUploadThread::Options upload_thread_options;
+    upload_thread_options.identify_client_via_url =
+        options.identify_client_via_url;
+    upload_thread_options.rate_limit = options.rate_limit;
+    upload_thread_options.upload_gzip = options.upload_gzip;
+    upload_thread_options.watch_pending_reports = options.periodic_tasks;
+
+    upload_thread.Reset(new CrashReportUploadThread(
+        database.get(), options.url, upload_thread_options));
+    upload_thread.Get()->Start();
+  }
+
+  ScopedStoppable prune_thread;
   if (options.periodic_tasks) {
-    prune_thread.reset(new PruneCrashReportThread(
+    prune_thread.Reset(new PruneCrashReportThread(
         database.get(), PruneCondition::GetDefault()));
-    prune_thread->Start();
+    prune_thread.Get()->Start();
   }
 
-  CrashReportExceptionHandler exception_handler(database.get(),
-                                                &upload_thread,
-                                                &options.annotations,
-                                                user_stream_sources);
+  CrashReportExceptionHandler exception_handler(
+      database.get(),
+      static_cast<CrashReportUploadThread*>(upload_thread.Get()),
+      &options.annotations,
+      user_stream_sources);
 
 #if defined(OS_WIN)
   if (options.initial_client_data.IsValid()) {
@@ -805,11 +829,6 @@ int HandlerMain(int argc,
 
   exception_handler_server.Run(&exception_handler);
 
-  upload_thread.Stop();
-  if (prune_thread) {
-    prune_thread->Stop();
-  }
-
   return EXIT_SUCCESS;
 }
 

diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc
@@ -187,7 +187,12 @@ kern_return_t CrashReportExceptionHandler::CatchMachException(
       return KERN_FAILURE;
     }
 
-    upload_thread_->ReportPending(uuid);
+    if (upload_thread_) {
+      upload_thread_->ReportPending(uuid);
+    } else {
+      database_->SkipReportUpload(
+          uuid, Metrics::CrashSkippedReason::kUploadsDisabled);
+    }
   }
 
   if (client_options.system_crash_reporter_forwarding != TriState::kDisabled &&

diff --git a/handler/mac/crash_report_exception_handler.h b/handler/mac/crash_report_exception_handler.h
@@ -36,7 +36,8 @@ class CrashReportExceptionHandler : public UniversalMachExcServer::Interface {
   //!
   //! \param[in] database The database to store crash reports in. Weak.
   //! \param[in] upload_thread The upload thread to notify when a new crash
-  //!     report is written into \a database.
+  //!     report is written into \a database. Report upload is skipped if this
+  //!     value is `nullptr`.
   //! \param[in] process_annotations A map of annotations to insert as
   //!     process-level annotations into each crash report that is written. Do
   //!     not confuse this with module-level annotations, which are under the

diff --git a/handler/prune_crash_reports_thread.h b/handler/prune_crash_reports_thread.h
@@ -18,6 +18,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "util/thread/stoppable.h"
 #include "util/thread/worker_thread.h"
 
 namespace crashpad {
@@ -31,7 +32,7 @@ class PruneCondition;
 //! After the thread is started, the database is pruned using the condition
 //! every 24 hours. Upon calling Start(), the thread waits 10 minutes before
 //! performing the initial prune operation.
-class PruneCrashReportThread : public WorkerThread::Delegate {
+class PruneCrashReportThread : public WorkerThread::Delegate, public Stoppable {
  public:
   //! \brief Constructs a new object.
   //!
@@ -42,14 +43,16 @@ class PruneCrashReportThread : public WorkerThread::Delegate {
                          std::unique_ptr<PruneCondition> condition);
   ~PruneCrashReportThread();
 
+  // Stoppable:
+
   //! \brief Starts a dedicated pruning thread.
   //!
   //! The thread waits before running the initial prune, so as to not interfere
   //! with any startup-related IO performed by the client.
   //!
   //! This method may only be be called on a newly-constructed object or after
   //! a call to Stop().
-  void Start();
+  void Start() override;
 
   //! \brief Stops the pruning thread.
   //!
@@ -58,7 +61,7 @@ class PruneCrashReportThread : public WorkerThread::Delegate {
   //!
   //! This method may be called from any thread other than the pruning thread.
   //! It is expected to only be called from the same thread that called Start().
-  void Stop();
+  void Stop() override;
 
  private:
   // WorkerThread::Delegate:

diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc
@@ -124,7 +124,12 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException(
       return termination_code;
     }
 
-    upload_thread_->ReportPending(uuid);
+    if (upload_thread_) {
+      upload_thread_->ReportPending(uuid);
+    } else {
+      database_->SkipReportUpload(
+          uuid, Metrics::CrashSkippedReason::kUploadsDisabled);
+    }
   }
 
   Metrics::ExceptionCaptureResult(Metrics::CaptureResult::kSuccess);

diff --git a/handler/win/crash_report_exception_handler.h b/handler/win/crash_report_exception_handler.h
@@ -37,7 +37,8 @@ class CrashReportExceptionHandler : public ExceptionHandlerServer::Delegate {
   //!
   //! \param[in] database The database to store crash reports in. Weak.
   //! \param[in] upload_thread The upload thread to notify when a new crash
-  //!     report is written into \a database.
+  //!     report is written into \a database. Report upload is skipped if this
+  //!     value is `nullptr`.
   //! \param[in] process_annotations A map of annotations to insert as
   //!     process-level annotations into each crash report that is written. Do
   //!     not confuse this with module-level annotations, which are under the

diff --git a/util/BUILD.gn b/util/BUILD.gn
@@ -144,6 +144,7 @@ static_library("util") {
     "string/split_string.cc",
     "string/split_string.h",
     "synchronization/semaphore.h",
+    "thread/stoppable.h",
     "thread/thread.cc",
     "thread/thread.h",
     "thread/thread_log_messages.cc",

diff --git a/util/thread/stoppable.h b/util/thread/stoppable.h
@@ -0,0 +1,39 @@
+// Copyright 2018 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef CRASHPAD_UTIL_THREAD_STOPPABLE_H_
+#define CRASHPAD_UTIL_THREAD_STOPPABLE_H_
+
+#include "base/macros.h"
+
+namespace crashpad {
+
+//! \brief An interface for operations that may be Started and Stopped.
+class Stoppable {
+ public:
+  virtual ~Stoppable() = default;
+
+  //! \brief Starts the operation.
+  virtual void Start() = 0;
+
+  //! \brief Stops the operation.
+  virtual void Stop() = 0;
+
+ protected:
+  Stoppable() = default;
+};
+
+}  // namespace crashpad
+
+#endif  // CRASHPAD_UTIL_THREAD_STOPPABLE_H_
diff --git a/util/util.gyp b/util/util.gyp
@@ -228,6 +228,7 @@
         'synchronization/semaphore_posix.cc',
         'synchronization/semaphore_win.cc',
         'synchronization/semaphore.h',
+        'thread/stoppable.h',
         'thread/thread.cc',
         'thread/thread.h',
         'thread/thread_log_messages.cc',