From 8297b19a5e85d7c4a437b4601c7007bda8273758 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 14 Apr 2017 14:50:11 -0400 Subject: [PATCH 1/4] =?UTF-8?q?Don=E2=80=99t=20attempt=20to=20do=20periodi?= =?UTF-8?q?c=20tasks=20in=20a=20secondary=20crashpad=5Fhandler?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 76a67a37b1d0 adds crashpad_handler’s --monitor-self argument, which results in a second crashpad_handler instance running out of the same database as the initial crashpad_handler instance that it monitors. The two handlers start at nearly the same time, and will initially be on precisely the same schedule for periodic tasks such as scanning for new reports to upload and pruning the database. This is an unnecessary duplication of effort. This adds a new --no-periodic-tasks argument to crashpad_handler. When the first instance of crashpad_handler starts a second to monitor it, it will use this argument, which prevents the second instance from performing these tasks. When --no-periodic-tasks is in effect, crashpad_handler will still be able to upload crash reports that it knows about by virtue of having written them itself, but it will not scan the database for other pending reports to upload. Bug: crashpad:143 Test: crashpad_util_test ThreadSafeVector.ThreadSafeVector Change-Id: I7b249dd7b6d5782448d8071855818f986b98ab5a Reviewed-on: https://chromium-review.googlesource.com/473827 Reviewed-by: Robert Sesek --- handler/crash_report_upload_thread.cc | 54 +++++++++-- handler/crash_report_upload_thread.h | 40 +++++--- handler/crashpad_handler.md | 17 +++- handler/handler_main.cc | 30 ++++-- handler/mac/crash_report_exception_handler.cc | 2 +- handler/win/crash_report_exception_handler.cc | 2 +- util/stdlib/thread_safe_vector.h | 63 +++++++++++++ util/stdlib/thread_safe_vector_test.cc | 91 +++++++++++++++++++ util/synchronization/semaphore.h | 9 +- util/synchronization/semaphore_mac.cc | 8 ++ util/synchronization/semaphore_posix.cc | 6 ++ util/synchronization/semaphore_test.cc | 12 +++ util/synchronization/semaphore_win.cc | 7 ++ util/thread/worker_thread.cc | 1 - util/thread/worker_thread.h | 11 ++- util/util.gyp | 1 + util/util_test.gyp | 1 + 17 files changed, 323 insertions(+), 32 deletions(-) create mode 100644 util/stdlib/thread_safe_vector.h create mode 100644 util/stdlib/thread_safe_vector_test.cc diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 9ba3f1f8..c13bbf4b 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -29,7 +30,6 @@ #include "snapshot/module_snapshot.h" #include "util/file/file_reader.h" #include "util/misc/metrics.h" -#include "util/misc/uuid.h" #include "util/net/http_body.h" #include "util/net/http_multipart_builder.h" #include "util/net/http_transport.h" @@ -139,15 +139,19 @@ class CallRecordUploadAttempt { CrashReportUploadThread::CrashReportUploadThread(CrashReportDatabase* database, const std::string& url, + bool watch_pending_reports, bool rate_limit, bool upload_gzip) : url_(url), - // Check for pending reports every 15 minutes, even in the absence of a - // signal from the handler thread. This allows for failed uploads to be - // retried periodically, and for pending reports written by other - // processes to be recognized. - thread_(15 * 60, this), + // When watching for pending reports, check every 15 minutes, even in the + // absence of a signal from the handler thread. This allows for failed + // uploads to be retried periodically, and for pending reports written by + // other processes to be recognized. + thread_(watch_pending_reports ? 15 * 60.0 : WorkerThread::kIndefiniteWait, + this), + known_pending_report_uuids_(), database_(database), + watch_pending_reports_(watch_pending_reports), rate_limit_(rate_limit), upload_gzip_(upload_gzip) { } @@ -156,18 +160,43 @@ CrashReportUploadThread::~CrashReportUploadThread() { } void CrashReportUploadThread::Start() { - thread_.Start(0); + thread_.Start(watch_pending_reports_ ? 0.0 : WorkerThread::kIndefiniteWait); } void CrashReportUploadThread::Stop() { thread_.Stop(); } -void CrashReportUploadThread::ReportPending() { +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) { + CrashReportDatabase::Report report; + if (database_->LookUpCrashReport(report_uuid, &report) != + CrashReportDatabase::kNoError) { + continue; + } + + ProcessPendingReport(report); + + // Respect Stop() being called after at least one attempt to process a + // report. + if (!thread_.is_running()) { + return; + } + } + + // Known pending reports are always processed (above). The rest of this + // function is concerned with scanning for pending reports not already known + // to this thread. + if (!watch_pending_reports_) { + return; + } + std::vector reports; if (database_->GetPendingReports(&reports) != CrashReportDatabase::kNoError) { // The database is sick. It might be prudent to stop trying to poke it from @@ -178,6 +207,15 @@ void CrashReportUploadThread::ProcessPendingReports() { } for (const CrashReportDatabase::Report& report : reports) { + if (std::find(known_report_uuids.begin(), + known_report_uuids.end(), + report.uuid) != known_report_uuids.end()) { + // An attempt to process the report already occurred above. The report is + // still pending, so upload must have failed. Don’t retry it immediately, + // it can wait until at least the next pass through this method. + continue; + } + ProcessPendingReport(report); // Respect Stop() being called after at least one attempt to process a diff --git a/handler/crash_report_upload_thread.h b/handler/crash_report_upload_thread.h index 14debacd..c769efed 100644 --- a/handler/crash_report_upload_thread.h +++ b/handler/crash_report_upload_thread.h @@ -20,6 +20,8 @@ #include "base/macros.h" #include "client/crash_report_database.h" +#include "util/misc/uuid.h" +#include "util/stdlib/thread_safe_vector.h" #include "util/thread/worker_thread.h" namespace crashpad { @@ -32,22 +34,28 @@ namespace crashpad { //! report has been added to the database by calling ReportPending(). //! //! Independently of being triggered by ReportPending(), objects of this class -//! periodically examine the database for pending reports. This allows failed -//! upload attempts for reports left in the pending state to be retried. 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. +//! can periodically examine the database for pending reports. This allows +//! failed upload attempts for reports left in the pending state to be retried. +//! 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 { public: //! \brief Constructs a new object. //! //! \param[in] database The database to upload crash reports from. //! \param[in] url The URL of the server to upload crash reports to. + //! \param[in] watch_pending_reports Whether to periodically check for new + //! pending reports not already known to exist. When `false`, only an + //! initial upload attempt will be made for reports known to exist by + //! having been added by the ReportPending() method. No scans for new + //! pending reports will be conducted. //! \param[in] rate_limit Whether uploads should be throttled to a (currently //! hardcoded) rate. //! \param[in] upload_gzip Whether uploads should use `gzip` compression. CrashReportUploadThread(CrashReportDatabase* database, const std::string& url, + bool watch_pending_reports, bool rate_limit, bool upload_gzip); ~CrashReportUploadThread(); @@ -75,8 +83,11 @@ class CrashReportUploadThread : public WorkerThread::Delegate { //! \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(); + void ReportPending(const UUID& report_uuid); private: //! \brief The result code from UploadReport(). @@ -99,8 +110,13 @@ class CrashReportUploadThread : public WorkerThread::Delegate { kRetry, }; - //! \brief Obtains all pending reports from the database, and calls - //! ProcessPendingReport() to process each one. + //! \brief Calls ProcessPendingReport() on pending reports. + //! + //! Assuming Stop() has not been called, this will process reports that the + //! object has been made aware of in ReportPending(). Additionally, if the + //! object was constructed with \a watch_pending_reports, it will also scan + //! the crash report database for other pending reports, and process those as + //! well. void ProcessPendingReports(); //! \brief Processes a single pending report from the database. @@ -137,11 +153,13 @@ class CrashReportUploadThread : public WorkerThread::Delegate { //! been called on any thread, as well as periodically on a timer. void DoWork(const WorkerThread* thread) override; - std::string url_; + const std::string url_; WorkerThread thread_; + ThreadSafeVector known_pending_report_uuids_; CrashReportDatabase* database_; // weak - bool rate_limit_; - bool upload_gzip_; + const bool watch_pending_reports_; + const bool rate_limit_; + const bool upload_gzip_; DISALLOW_COPY_AND_ASSIGN(CrashReportUploadThread); }; diff --git a/handler/crashpad_handler.md b/handler/crashpad_handler.md index e76fed76..9cb29e43 100644 --- a/handler/crashpad_handler.md +++ b/handler/crashpad_handler.md @@ -151,7 +151,8 @@ establish the Crashpad client environment before running a program. become a client of the second one. The second instance will be started with the same **--annotation**, **--database**, **--monitor-self-annotation**, **--no-rate-limit**, **--no-upload-gzip**, and **--url** arguments as the - original one. The second instance will not be started with a + original one. The second instance will always be started with a + **--no-periodic-tasks** argument, and will not be started with a **--metrics-dir** argument even if the original instance was. Where supported by the underlying operating system, the second instance will @@ -183,6 +184,20 @@ establish the Crashpad client environment before running a program. To prevent excessive accumulation of handler processes, _ARGUMENT_ must not be `--monitor-self`. + * **--no-periodic-tasks** + + Do not scan for new pending crash reports or prune the crash report database. + Only crash reports recorded by this instance of the Crashpad handler will + become eligible for upload in this instance, and only a single initial upload + attempt will be made. + + This option is not intended for general use. It is provided to prevent + multiple instances of the Crashpad handler from duplicating the effort of + performing the same periodic tasks. In normal use, the first instance of the + Crashpad handler will assume the responsibility for performing these tasks, + and will provide this argument to any second instance. See + **--monitor-self**. + * **--no-rate-limit** Do not rate limit the upload of crash reports. By default uploads are diff --git a/handler/handler_main.cc b/handler/handler_main.cc index 641bf0f6..b4a08c51 100644 --- a/handler/handler_main.cc +++ b/handler/handler_main.cc @@ -109,6 +109,7 @@ void Usage(const base::FilePath& me) { " set a module annotation in the handler\n" " --monitor-self-argument=ARGUMENT\n" " provide additional arguments to the second handler\n" +" --no-periodic-tasks don't scan for new reports or prune the database\n" " --no-rate-limit don't rate limit crash uploads\n" " --no-upload-gzip don't use gzip compression when uploading\n" #if defined(OS_WIN) @@ -142,6 +143,7 @@ struct Options { InitialClientData initial_client_data; #endif // OS_MACOSX bool monitor_self; + bool periodic_tasks; bool rate_limit; bool upload_gzip; }; @@ -353,6 +355,7 @@ void MonitorSelf(const Options& options) { return; } std::vector extra_arguments(options.monitor_self_arguments); + extra_arguments.push_back("--no-periodic-tasks"); if (!options.rate_limit) { extra_arguments.push_back("--no-rate-limit"); } @@ -416,6 +419,7 @@ int HandlerMain(int argc, kOptionMonitorSelf, kOptionMonitorSelfAnnotation, kOptionMonitorSelfArgument, + kOptionNoPeriodicTasks, kOptionNoRateLimit, kOptionNoUploadGzip, #if defined(OS_WIN) @@ -456,6 +460,7 @@ int HandlerMain(int argc, required_argument, nullptr, kOptionMonitorSelfArgument}, + {"no-periodic-tasks", no_argument, nullptr, kOptionNoPeriodicTasks}, {"no-rate-limit", no_argument, nullptr, kOptionNoRateLimit}, {"no-upload-gzip", no_argument, nullptr, kOptionNoUploadGzip}, #if defined(OS_WIN) @@ -477,6 +482,7 @@ int HandlerMain(int argc, #if defined(OS_MACOSX) options.handshake_fd = -1; #endif + options.periodic_tasks = true; options.rate_limit = true; options.upload_gzip = true; @@ -540,6 +546,10 @@ int HandlerMain(int argc, options.monitor_self_arguments.push_back(optarg); break; } + case kOptionNoPeriodicTasks: { + options.periodic_tasks = false; + break; + } case kOptionNoRateLimit: { options.rate_limit = false; break; @@ -721,13 +731,19 @@ int HandlerMain(int argc, // 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 upload_thread( - database.get(), options.url, options.rate_limit, options.upload_gzip); + CrashReportUploadThread upload_thread(database.get(), + options.url, + options.periodic_tasks, + options.rate_limit, + options.upload_gzip); upload_thread.Start(); - PruneCrashReportThread prune_thread(database.get(), - PruneCondition::GetDefault()); - prune_thread.Start(); + std::unique_ptr prune_thread; + if (options.periodic_tasks) { + prune_thread.reset(new PruneCrashReportThread( + database.get(), PruneCondition::GetDefault())); + prune_thread->Start(); + } CrashReportExceptionHandler exception_handler(database.get(), &upload_thread, @@ -744,7 +760,9 @@ int HandlerMain(int argc, exception_handler_server.Run(&exception_handler); upload_thread.Stop(); - prune_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 a96131cd..7d3d6297 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -190,7 +190,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( return KERN_FAILURE; } - upload_thread_->ReportPending(); + upload_thread_->ReportPending(uuid); } if (client_options.system_crash_reporter_forwarding != TriState::kDisabled && diff --git a/handler/win/crash_report_exception_handler.cc b/handler/win/crash_report_exception_handler.cc index 7828aac6..0ab206c1 100644 --- a/handler/win/crash_report_exception_handler.cc +++ b/handler/win/crash_report_exception_handler.cc @@ -130,7 +130,7 @@ unsigned int CrashReportExceptionHandler::ExceptionHandlerServerException( return termination_code; } - upload_thread_->ReportPending(); + upload_thread_->ReportPending(uuid); } Metrics::ExceptionCaptureResult(Metrics::CaptureResult::kSuccess); diff --git a/util/stdlib/thread_safe_vector.h b/util/stdlib/thread_safe_vector.h new file mode 100644 index 00000000..f97024d0 --- /dev/null +++ b/util/stdlib/thread_safe_vector.h @@ -0,0 +1,63 @@ +// Copyright 2017 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_STDLIB_THREAD_SAFE_VECTOR_H_ +#define CRASHPAD_UTIL_STDLIB_THREAD_SAFE_VECTOR_H_ + +#include +#include + +#include "base/macros.h" +#include "base/synchronization/lock.h" + +namespace crashpad { + +//! \brief A wrapper for a `std::vector<>` that can be accessed safely from +//! multiple threads. +//! +//! This is not a drop-in replacement for `std::vector<>`. Only necessary +//! operations are defined. +template +class ThreadSafeVector { + public: + ThreadSafeVector() : vector_(), lock_() {} + ~ThreadSafeVector() {} + + //! \brief Wraps `std::vector<>::%push_back()`. + void PushBack(const T& element) { + base::AutoLock lock_owner(lock_); + vector_.push_back(element); + } + + //! \brief Atomically clears the underlying vector and returns its previous + //! contents. + std::vector Drain() { + std::vector contents; + { + base::AutoLock lock_owner(lock_); + std::swap(vector_, contents); + } + return contents; + } + + private: + std::vector vector_; + base::Lock lock_; + + DISALLOW_COPY_AND_ASSIGN(ThreadSafeVector); +}; + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_STDLIB_THREAD_SAFE_VECTOR_H_ diff --git a/util/stdlib/thread_safe_vector_test.cc b/util/stdlib/thread_safe_vector_test.cc new file mode 100644 index 00000000..805360f4 --- /dev/null +++ b/util/stdlib/thread_safe_vector_test.cc @@ -0,0 +1,91 @@ +// Copyright 2017 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. + +#include "util/stdlib/thread_safe_vector.h" + +#include "gtest/gtest.h" +#include "util/thread/thread.h" + +namespace crashpad { +namespace test { +namespace { + +constexpr int kElementsPerThread = 100; + +class ThreadSafeVectorTestThread : public Thread { + public: + ThreadSafeVectorTestThread() : thread_safe_vector_(nullptr), start_(0) {} + ~ThreadSafeVectorTestThread() {} + + void SetTestParameters(ThreadSafeVector* thread_safe_vector, int start) { + thread_safe_vector_ = thread_safe_vector; + start_ = start; + } + + // Thread: + void ThreadMain() override { + for (int i = start_; i < start_ + kElementsPerThread; ++i) { + thread_safe_vector_->PushBack(i); + } + } + + private: + ThreadSafeVector* thread_safe_vector_; + int start_; + + DISALLOW_COPY_AND_ASSIGN(ThreadSafeVectorTestThread); +}; + +TEST(ThreadSafeVector, ThreadSafeVector) { + ThreadSafeVector thread_safe_vector; + std::vector vector = thread_safe_vector.Drain(); + EXPECT_TRUE(vector.empty()); + + ThreadSafeVectorTestThread threads[100]; + for (size_t index = 0; index < arraysize(threads); ++index) { + threads[index].SetTestParameters( + &thread_safe_vector, static_cast(index * kElementsPerThread)); + } + + for (size_t index = 0; index < arraysize(threads); ++index) { + threads[index].Start(); + + if (index % 10 == 0) { + // Drain the vector periodically to test that simultaneous Drain() and + // PushBack() operations work properly. + std::vector drained = thread_safe_vector.Drain(); + vector.insert(vector.end(), drained.begin(), drained.end()); + } + } + + for (ThreadSafeVectorTestThread& thread : threads) { + thread.Join(); + } + + std::vector drained = thread_safe_vector.Drain(); + vector.insert(vector.end(), drained.begin(), drained.end()); + bool found[arraysize(threads) * kElementsPerThread] = {}; + EXPECT_EQ(vector.size(), arraysize(found)); + for (int element : vector) { + EXPECT_FALSE(found[element]) << element; + found[element] = true; + } + + vector = thread_safe_vector.Drain(); + EXPECT_TRUE(vector.empty()); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/synchronization/semaphore.h b/util/synchronization/semaphore.h index ac77cb7c..49479639 100644 --- a/util/synchronization/semaphore.h +++ b/util/synchronization/semaphore.h @@ -15,6 +15,8 @@ #ifndef CRASHPAD_UTIL_SYNCHRONIZATION_SEMAPHORE_H_ #define CRASHPAD_UTIL_SYNCHRONIZATION_SEMAPHORE_H_ +#include + #include "build/build_config.h" #if defined(OS_MACOSX) @@ -30,6 +32,10 @@ namespace crashpad { //! \brief An anonymous in-process counting sempahore. class Semaphore { public: + //! \brief A TimedWait() argument that causes an indefinite wait. + static constexpr double kIndefiniteWait = + std::numeric_limits::infinity(); + //! \brief Initializes the semaphore. //! //! \param[in] value The initial value of the semaphore. @@ -51,7 +57,8 @@ class Semaphore { //! \brief Performs a timed wait (or “procure”) operation on the semaphore. //! //! \param[in] seconds The maximum number of seconds to wait for the operation - //! to complete. + //! to complete. If \a seconds is #kIndefiniteWait, this method behaves as + //! Wait(), and will not time out. //! //! \return `false` if the wait timed out, `true` otherwise. //! diff --git a/util/synchronization/semaphore_mac.cc b/util/synchronization/semaphore_mac.cc index e8a79ab4..4f3bf00a 100644 --- a/util/synchronization/semaphore_mac.cc +++ b/util/synchronization/semaphore_mac.cc @@ -14,6 +14,8 @@ #include "util/synchronization/semaphore.h" +#include + #include "base/logging.h" namespace crashpad { @@ -33,6 +35,12 @@ void Semaphore::Wait() { bool Semaphore::TimedWait(double seconds) { DCHECK_GE(seconds, 0.0); + + if (std::isinf(seconds)) { + Wait(); + return true; + } + const dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, seconds * NSEC_PER_SEC); return dispatch_semaphore_wait(semaphore_, timeout) == 0; diff --git a/util/synchronization/semaphore_posix.cc b/util/synchronization/semaphore_posix.cc index 973f0a5d..c781e4c6 100644 --- a/util/synchronization/semaphore_posix.cc +++ b/util/synchronization/semaphore_posix.cc @@ -39,6 +39,12 @@ void Semaphore::Wait() { bool Semaphore::TimedWait(double seconds) { DCHECK_GE(seconds, 0.0); + + if (std::isinf(seconds)) { + Wait(); + return true; + } + timespec timeout; timeout.tv_sec = seconds; timeout.tv_nsec = (seconds - trunc(seconds)) * 1E9; diff --git a/util/synchronization/semaphore_test.cc b/util/synchronization/semaphore_test.cc index fb4338c0..ccaf742d 100644 --- a/util/synchronization/semaphore_test.cc +++ b/util/synchronization/semaphore_test.cc @@ -43,6 +43,18 @@ TEST(Semaphore, TimedWaitTimeout) { EXPECT_FALSE(semaphore.TimedWait(0.01)); // 10ms } +TEST(Semaphore, TimedWaitInfinite_0) { + Semaphore semaphore(0); + semaphore.Signal(); + EXPECT_TRUE(semaphore.TimedWait(std::numeric_limits::infinity())); +} + +TEST(Semaphore, TimedWaitInfinite_1) { + Semaphore semaphore(1); + EXPECT_TRUE(semaphore.TimedWait(std::numeric_limits::infinity())); + semaphore.Signal(); +} + struct ThreadMainInfo { #if defined(OS_POSIX) pthread_t pthread; diff --git a/util/synchronization/semaphore_win.cc b/util/synchronization/semaphore_win.cc index 962c7bae..2f26c237 100644 --- a/util/synchronization/semaphore_win.cc +++ b/util/synchronization/semaphore_win.cc @@ -14,6 +14,7 @@ #include "util/synchronization/semaphore.h" +#include #include #include "base/logging.h" @@ -38,6 +39,12 @@ void Semaphore::Wait() { bool Semaphore::TimedWait(double seconds) { DCHECK_GE(seconds, 0.0); + + if (std::isinf(seconds)) { + Wait(); + return true; + } + DWORD rv = WaitForSingleObject(semaphore_, static_cast(seconds * 1E3)); PCHECK(rv == WAIT_OBJECT_0 || rv == WAIT_TIMEOUT) << "WaitForSingleObject"; return rv == WAIT_OBJECT_0; diff --git a/util/thread/worker_thread.cc b/util/thread/worker_thread.cc index 595ef8f4..3cf48970 100644 --- a/util/thread/worker_thread.cc +++ b/util/thread/worker_thread.cc @@ -15,7 +15,6 @@ #include "util/thread/worker_thread.h" #include "base/logging.h" -#include "util/synchronization/semaphore.h" #include "util/thread/thread.h" namespace crashpad { diff --git a/util/thread/worker_thread.h b/util/thread/worker_thread.h index 321d9184..97fb6ecd 100644 --- a/util/thread/worker_thread.h +++ b/util/thread/worker_thread.h @@ -18,6 +18,7 @@ #include #include "base/macros.h" +#include "util/synchronization/semaphore.h" namespace crashpad { @@ -40,11 +41,16 @@ class WorkerThread { virtual ~Delegate() {} }; + //! \brief A delay or interval argument that causes an indefinite wait. + static constexpr double kIndefiniteWait = Semaphore::kIndefiniteWait; + //! \brief Creates a new WorkerThread that is not yet running. //! //! \param[in] work_interval The time interval in seconds at which the \a //! delegate runs. The interval counts from the completion of - //! Delegate::DoWork() to the next invocation. + //! Delegate::DoWork() to the next invocation. This can be + //! #kIndefiniteWait if work should only be done when DoWorkNow() is + //! called. //! \param[in] delegate The work delegate to invoke every interval. WorkerThread(double work_interval, Delegate* delegate); ~WorkerThread(); @@ -55,7 +61,8 @@ class WorkerThread { //! //! \param[in] initial_work_delay The amount of time in seconds to wait //! before invoking the \a delegate for the first time. Pass `0` for - //! no delay. + //! no delay. This can be #kIndefiniteWait if work should not be done + //! until DoWorkNow() is called. void Start(double initial_work_delay); //! \brief Stops the worker thread from running. diff --git a/util/util.gyp b/util/util.gyp index 7ef7a0ca..5bc4de6f 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -168,6 +168,7 @@ 'stdlib/strlcpy.h', 'stdlib/strnlen.cc', 'stdlib/strnlen.h', + 'stdlib/thread_safe_vector.h', 'string/split_string.cc', 'string/split_string.h', 'synchronization/semaphore_mac.cc', diff --git a/util/util_test.gyp b/util/util_test.gyp index 31323c0b..95dac082 100644 --- a/util/util_test.gyp +++ b/util/util_test.gyp @@ -87,6 +87,7 @@ 'stdlib/string_number_conversion_test.cc', 'stdlib/strlcpy_test.cc', 'stdlib/strnlen_test.cc', + 'stdlib/thread_safe_vector_test.cc', 'string/split_string_test.cc', 'synchronization/semaphore_test.cc', 'thread/thread_log_messages_test.cc', From c64fd3f9b4d54fe88341edca1a830c4f18809dc3 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Fri, 14 Apr 2017 17:02:10 -0400 Subject: [PATCH 2/4] Update mini_chromium to dc3d480305b27a5a1fb57f51a997529e00fed00b MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2c1b54080cb2 android: Recognize Bionic’s semantics for strerror_r() 9a7a549b414d Terminate from CHECK(), LOG(FATAL) consistently dc3d480305b2 android: Don’t set _FILE_OFFSET_BITS, it never worked Bug: crashpad:30 Change-Id: I93f810efa17047b797491a9b1089ea8c80f81e41 Reviewed-on: https://chromium-review.googlesource.com/478252 Reviewed-by: Joshua Peraza --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index aefedbc9..5c35e6d6 100644 --- a/DEPS +++ b/DEPS @@ -38,7 +38,7 @@ deps = { 'crashpad/third_party/mini_chromium/mini_chromium': Var('chromium_git') + '/chromium/mini_chromium@' + - '62e6015f633dd4acb1610db15a064889315cadaa', + 'dc3d480305b27a5a1fb57f51a997529e00fed00b', 'crashpad/third_party/zlib/zlib': Var('chromium_git') + '/chromium/src/third_party/zlib@' + '13dc246a58e4b72104d35f9b1809af95221ebda7', From ddcc74f08f4f11acd1b025cb99b792666a63f0c4 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Mon, 17 Apr 2017 17:03:57 -0400 Subject: [PATCH 3/4] mac: Tolerate dead names for reply ports in the exception handler server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-monitoring revealed this CHECK was being hit in the wild: base::debug::BreakDebugger() debugger_posix.cc:260 logging::LogMessage::~LogMessage() logging.cc:759 logging::MachLogMessage::~MachLogMessage() mach_logging.cc:45 crashpad::ExceptionHandlerServer::Run() exception_handler_server.cc:108 crashpad::HandlerMain() handler_main.cc:744 The MACH_CHECK() was: 108 MACH_CHECK(mr == MACH_MSG_SUCCESS, mr) << "MachMessageServer::Run"; Crash reports captured the full message, including the value of mr: [0418/015158.777231:FATAL:exception_handler_server.cc(108)] Check failed: mr == MACH_MSG_SUCCESS. MachMessageServer::Run: (ipc/send) invalid destination port (0x10000003) 0x10000003 = MACH_SEND_INVALID_DEST. This can happen when attempting to send a Mach message to a dead name. Send (and send-once) rights become dead names when the corresponding receive right dies. This would not normally happen for exception requests originating in the kernel. It can happen for requests originating from a user task: when the user task dies, the receive right dies with it. All it takes to trigger this CHECK() in crashpad_handler is for a Crashpad client to die (or be killed) while the handler is processing a SimulateCrash() that the client originated. Accept MACH_SEND_INVALID_DEST as a valid return value for MachMessageServer::Run(). Note that MachMessageServer’s test coverage was already aware of this behavior. MachMessageServer::Run()’s documentation is updated to reflect it too. Change-Id: I483c065d3c5f9a7da410ef3ad54db45ee53aa3c2 Reviewed-on: https://chromium-review.googlesource.com/479093 Commit-Queue: Mark Mentovai Reviewed-by: Robert Sesek --- handler/mac/exception_handler_server.cc | 9 ++++++++- util/mach/mach_message_server.h | 6 +++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/handler/mac/exception_handler_server.cc b/handler/mac/exception_handler_server.cc index 38a016e9..3f30e03a 100644 --- a/handler/mac/exception_handler_server.cc +++ b/handler/mac/exception_handler_server.cc @@ -105,7 +105,14 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, MachMessageServer::kOneShot, MachMessageServer::kReceiveLargeIgnore, kMachMessageTimeoutWaitIndefinitely); - MACH_CHECK(mr == MACH_MSG_SUCCESS, mr) << "MachMessageServer::Run"; + + // MACH_SEND_INVALID_DEST occurs when attempting to reply to a dead name. + // This can happen if a mach_exc or exc client disappears before a reply + // can be sent to it. That’s unusal for kernel-generated requests, but can + // easily happen if a task sends its own exception request (as + // SimulateCrash() does) and dies before the reply is sent. + MACH_CHECK(mr == MACH_MSG_SUCCESS || mr == MACH_SEND_INVALID_DEST, mr) + << "MachMessageServer::Run"; } } diff --git a/util/mach/mach_message_server.h b/util/mach/mach_message_server.h index 9793560e..5e96c4ce 100644 --- a/util/mach/mach_message_server.h +++ b/util/mach/mach_message_server.h @@ -162,7 +162,11 @@ class MachMessageServer { //! timeout_ms is not #kMachMessageTimeoutWaitIndefinitely). This function //! has no successful return value when \a persistent is #kPersistent and //! \a timeout_ms is #kMachMessageTimeoutWaitIndefinitely. On failure, - //! returns a value identifying the nature of the error. + //! returns a value identifying the nature of the error. A request + //! received with a reply port that is (or becomes) a dead name before the + //! reply is sent will result in `MACH_SEND_INVALID_DEST` as a return + //! value, which may or may not be considered an error from the caller’s + //! perspective. static mach_msg_return_t Run(Interface* interface, mach_port_t receive_port, mach_msg_options_t options, From b8aaa22905308cc400f880006a84dddac834bd6b Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 18 Apr 2017 11:50:39 -0400 Subject: [PATCH 4/4] mac handler: Record a file-limits annotation (temporarily) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "file-limit" annotation will be used to confirm the theory that certain crashes are caused by systems at or near their file descriptor table size limits. The annotation records the system-wide kern.num_files and kern.maxfiles values, and the process-specific current and maximum file descriptor limits. The annotation will be set on crashpad_handler startup, and will be refreshed every time an exception is handled and every time the upload thread processes a pending report. It’s expected that this annotation will be removed after enough data has been collected to confirm the theory. However, the principle is useful enough that we may want to provide this feature more generally under bugs 19 or 21. Bug: crashpad:180 Change-Id: I3bb78fae60e0567bc4ac2625716e0abe0ddae08c Reviewed-on: https://chromium-review.googlesource.com/479914 Reviewed-by: Robert Sesek --- handler/crash_report_upload_thread.cc | 8 ++ handler/handler.gyp | 2 + handler/handler_main.cc | 3 + handler/mac/crash_report_exception_handler.cc | 2 + handler/mac/file_limit_annotation.cc | 93 +++++++++++++++++++ handler/mac/file_limit_annotation.h | 38 ++++++++ 6 files changed, 146 insertions(+) create mode 100644 handler/mac/file_limit_annotation.cc create mode 100644 handler/mac/file_limit_annotation.h diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index c13bbf4b..d77cdbad 100644 --- a/handler/crash_report_upload_thread.cc +++ b/handler/crash_report_upload_thread.cc @@ -35,6 +35,10 @@ #include "util/net/http_transport.h" #include "util/stdlib/map_insert.h" +#if defined(OS_MACOSX) +#include "handler/mac/file_limit_annotation.h" +#endif // OS_MACOSX + namespace crashpad { namespace { @@ -228,6 +232,10 @@ void CrashReportUploadThread::ProcessPendingReports() { void CrashReportUploadThread::ProcessPendingReport( const CrashReportDatabase::Report& report) { +#if defined(OS_MACOSX) + RecordFileLimitAnnotation(); +#endif // OS_MACOSX + Settings* const settings = database_->GetSettings(); bool uploads_enabled; diff --git a/handler/handler.gyp b/handler/handler.gyp index d6e4c271..50479828 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -43,6 +43,8 @@ 'mac/crash_report_exception_handler.h', 'mac/exception_handler_server.cc', 'mac/exception_handler_server.h', + 'mac/file_limit_annotation.cc', + 'mac/file_limit_annotation.h', 'prune_crash_reports_thread.cc', 'prune_crash_reports_thread.h', 'user_stream_data_source.cc', diff --git a/handler/handler_main.cc b/handler/handler_main.cc index b4a08c51..e6d05e22 100644 --- a/handler/handler_main.cc +++ b/handler/handler_main.cc @@ -61,6 +61,7 @@ #include "base/mac/scoped_mach_port.h" #include "handler/mac/crash_report_exception_handler.h" #include "handler/mac/exception_handler_server.h" +#include "handler/mac/file_limit_annotation.h" #include "util/mach/child_port_handshake.h" #include "util/mach/mach_extensions.h" #include "util/posix/close_stdio.h" @@ -697,6 +698,8 @@ int HandlerMain(int argc, reset_sigterm.reset(&old_sigterm_action); } } + + RecordFileLimitAnnotation(); #elif defined(OS_WIN) // Shut down as late as possible relative to programs we're watching. if (!SetProcessShutdownParameters(0x100, SHUTDOWN_NORETRY)) diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index 7d3d6297..6f9cdbe6 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -21,6 +21,7 @@ #include "base/mac/scoped_mach_port.h" #include "base/strings/stringprintf.h" #include "client/settings.h" +#include "handler/mac/file_limit_annotation.h" #include "minidump/minidump_file_writer.h" #include "minidump/minidump_user_extension_stream_data_source.h" #include "snapshot/crashpad_info_client_options.h" @@ -67,6 +68,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( mach_msg_type_number_t* new_state_count, const mach_msg_trailer_t* trailer, bool* destroy_complex_request) { + RecordFileLimitAnnotation(); Metrics::ExceptionEncountered(); Metrics::ExceptionCode(ExceptionCodeForMetrics(exception, code[0])); *destroy_complex_request = true; diff --git a/handler/mac/file_limit_annotation.cc b/handler/mac/file_limit_annotation.cc new file mode 100644 index 00000000..359ed8e0 --- /dev/null +++ b/handler/mac/file_limit_annotation.cc @@ -0,0 +1,93 @@ +// Copyright 2017 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. + +#include "handler/mac/file_limit_annotation.h" + +#include +#include +#include +#include + +#include + +#include "base/format_macros.h" +#include "base/macros.h" +#include "base/strings/stringprintf.h" +#include "client/crashpad_info.h" +#include "client/simple_string_dictionary.h" + +namespace { + +// rv is the return value from sysctl() or sysctlbyname(), and value and size +// are the pointers passed as oldp and oldlenp. If sysctl() failed, the returned +// string will be "E" followed by the error number. If there was a size +// mismatch, the returned string will be "Z" followed by the size indicated by +// sysctl(). Normally, a string representation of *value will be returned. +std::string FormatFromSysctl(int rv, const int* value, const size_t* size) { + if (rv != 0) { + return base::StringPrintf("E%d", errno); + } + if (*size != sizeof(*value)) { + return base::StringPrintf("Z%zu", *size); + } + return base::StringPrintf("%d", *value); +} + +// Returns a string for |limit|, or "inf" if |limit| is RLIM_INFINITY. +std::string StringForRLim(rlim_t limit) { + if (limit == RLIM_INFINITY) { + return std::string("inf"); + } + + return base::StringPrintf("%" PRIu64, limit); +} + +} // namespace + +namespace crashpad { + +void RecordFileLimitAnnotation() { + CrashpadInfo* crashpad_info = CrashpadInfo::GetCrashpadInfo(); + SimpleStringDictionary* simple_annotations = + crashpad_info->simple_annotations(); + if (!simple_annotations) { + simple_annotations = new SimpleStringDictionary(); + crashpad_info->set_simple_annotations(simple_annotations); + } + + int value; + size_t size = sizeof(value); + std::string num_files = FormatFromSysctl( + sysctlbyname("kern.num_files", &value, &size, nullptr, 0), &value, &size); + + int mib[] = {CTL_KERN, KERN_MAXFILES}; + size = sizeof(value); + std::string max_files = FormatFromSysctl( + sysctl(mib, arraysize(mib), &value, &size, nullptr, 0), &value, &size); + + rlimit limit; + std::string nofile; + if (getrlimit(RLIMIT_NOFILE, &limit) != 0) { + nofile = base::StringPrintf("E%d,E%d", errno, errno); + } else { + nofile = + StringForRLim(limit.rlim_cur) + "," + StringForRLim(limit.rlim_max); + } + + std::string annotation = base::StringPrintf( + "%s,%s,%s", num_files.c_str(), max_files.c_str(), nofile.c_str()); + simple_annotations->SetKeyValue("file-limits", annotation.c_str()); +} + +} // namespace crashpad diff --git a/handler/mac/file_limit_annotation.h b/handler/mac/file_limit_annotation.h new file mode 100644 index 00000000..1131986e --- /dev/null +++ b/handler/mac/file_limit_annotation.h @@ -0,0 +1,38 @@ +// Copyright 2017 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_HANDLER_MAC_FILE_LIMIT_ANNOTATION_H_ +#define CRASHPAD_HANDLER_MAC_FILE_LIMIT_ANNOTATION_H_ + +namespace crashpad { + +//! \brief Records a `"file-limits"` simple annotation for the process. +//! +//! This annotation will be used to confirm the theory that certain crashes are +//! caused by systems at or near their file descriptor table size limits. +//! +//! The format of the annotation is four comma-separated values: the system-wide +//! `kern.num_files` and `kern.maxfiles` values from `sysctl()`, and the +//! process-specific current and maximum file descriptor limits from +//! `getrlimit(RLIMIT_NOFILE, …)`. +//! +//! See https://crashpad.chromium.org/bug/180. +//! +//! TODO(mark): Remove this annotation after sufficient data has been collected +//! for analysis. +void RecordFileLimitAnnotation(); + +} // namespace crashpad + +#endif // CRASHPAD_HANDLER_MAC_FILE_LIMIT_ANNOTATION_H_