From ebad8bd925c36721afc4a02c55fb9ddf868a5a68 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 20 Feb 2018 10:55:17 -0800 Subject: [PATCH] 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 Reviewed-by: Mark Mentovai --- handler/crash_report_upload_thread.cc | 19 ++--- handler/crash_report_upload_thread.h | 28 ++++--- handler/handler_main.cc | 73 ++++++++++++------- handler/mac/crash_report_exception_handler.cc | 7 +- handler/mac/crash_report_exception_handler.h | 3 +- handler/prune_crash_reports_thread.h | 9 ++- handler/win/crash_report_exception_handler.cc | 7 +- handler/win/crash_report_exception_handler.h | 3 +- util/BUILD.gn | 1 + util/thread/stoppable.h | 39 ++++++++++ util/util.gyp | 1 + 11 files changed, 135 insertions(+), 55 deletions(-) create mode 100644 util/thread/stoppable.h diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 290c5a3f..715c533a 100644 --- 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 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 index 69e7a3c2..2ec1147d 100644 --- 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 index c5e35bfc..451214a4 100644 --- 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_; + + 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(); + 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; - std::unique_ptr prune_thread; - if (options.periodic_tasks) { - prune_thread.reset(new PruneCrashReportThread( - database.get(), PruneCondition::GetDefault())); - prune_thread->Start(); + upload_thread.Reset(new CrashReportUploadThread( + database.get(), options.url, upload_thread_options)); + upload_thread.Get()->Start(); } - CrashReportExceptionHandler exception_handler(database.get(), - &upload_thread, - &options.annotations, - user_stream_sources); + ScopedStoppable prune_thread; + if (options.periodic_tasks) { + prune_thread.Reset(new PruneCrashReportThread( + database.get(), PruneCondition::GetDefault())); + prune_thread.Get()->Start(); + } + + CrashReportExceptionHandler exception_handler( + database.get(), + static_cast(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 index 2317df24..edebf87a 100644 --- 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 index cc314f1f..0b44de67 100644 --- 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 index 72b69fc6..0fd365b0 100644 --- a/handler/prune_crash_reports_thread.h +++ b/handler/prune_crash_reports_thread.h @@ -18,6 +18,7 @@ #include #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,6 +43,8 @@ class PruneCrashReportThread : public WorkerThread::Delegate { std::unique_ptr condition); ~PruneCrashReportThread(); + // Stoppable: + //! \brief Starts a dedicated pruning thread. //! //! The thread waits before running the initial prune, so as to not interfere @@ -49,7 +52,7 @@ class PruneCrashReportThread : public WorkerThread::Delegate { //! //! 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 index 726dcb39..6d53d810 100644 --- 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 index e1fb725d..c2781de3 100644 --- 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 index e39212cc..0cdaef82 100644 --- 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 new file mode 100644 index 00000000..e7a51277 --- /dev/null +++ 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 index 15925444..58cc27ec 100644 --- 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',