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', diff --git a/handler/crash_report_upload_thread.cc b/handler/crash_report_upload_thread.cc index 9ba3f1f8..d77cdbad 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,12 +30,15 @@ #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" #include "util/stdlib/map_insert.h" +#if defined(OS_MACOSX) +#include "handler/mac/file_limit_annotation.h" +#endif // OS_MACOSX + namespace crashpad { namespace { @@ -139,15 +143,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 +164,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 +211,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 @@ -190,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/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.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 641bf0f6..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" @@ -109,6 +110,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 +144,7 @@ struct Options { InitialClientData initial_client_data; #endif // OS_MACOSX bool monitor_self; + bool periodic_tasks; bool rate_limit; bool upload_gzip; }; @@ -353,6 +356,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 +420,7 @@ int HandlerMain(int argc, kOptionMonitorSelf, kOptionMonitorSelfAnnotation, kOptionMonitorSelfArgument, + kOptionNoPeriodicTasks, kOptionNoRateLimit, kOptionNoUploadGzip, #if defined(OS_WIN) @@ -456,6 +461,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 +483,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 +547,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; @@ -687,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)) @@ -721,13 +734,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 +763,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..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; @@ -190,7 +192,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/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/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_ 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/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, 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',