diff options
author | Moonchild <moonchild@palemoon.org> | 2022-09-09 08:10:36 +0000 |
---|---|---|
committer | Moonchild <moonchild@palemoon.org> | 2022-09-09 08:10:36 +0000 |
commit | c623c471573c1a14b5b540c8505341445d2cae64 (patch) | |
tree | 3c7c6b66dd799aace0a979dedb68e1de2a04ade7 | |
parent | da835adb4862038fe15163da5c1e9ed85d746870 (diff) | |
parent | 77bca114d1b6efc29ab98f6a95feacd701cfcbe7 (diff) | |
download | uxp-c623c471573c1a14b5b540c8505341445d2cae64.tar.gz |
Merge pull request 'Update IPC locking code.' (#2004) from athenian200/UXP:ipc-locking-update into master
Reviewed-on: https://repo.palemoon.org/MoonchildProductions/UXP/pulls/2004
-rw-r--r-- | ipc/chromium/moz.build | 1 | ||||
-rw-r--r-- | ipc/chromium/src/base/condition_variable.h | 93 | ||||
-rw-r--r-- | ipc/chromium/src/base/condition_variable_posix.cc | 50 | ||||
-rw-r--r-- | ipc/chromium/src/base/condition_variable_win.cc | 436 | ||||
-rw-r--r-- | ipc/chromium/src/base/lock.cc | 8 | ||||
-rw-r--r-- | ipc/chromium/src/base/lock.h | 56 | ||||
-rw-r--r-- | ipc/chromium/src/base/lock_impl.h | 44 | ||||
-rw-r--r-- | ipc/chromium/src/base/lock_impl_posix.cc | 80 | ||||
-rw-r--r-- | ipc/chromium/src/base/lock_impl_win.cc | 67 |
9 files changed, 190 insertions, 645 deletions
diff --git a/ipc/chromium/moz.build b/ipc/chromium/moz.build index 4050307fe9..e79c7f3753 100644 --- a/ipc/chromium/moz.build +++ b/ipc/chromium/moz.build @@ -12,7 +12,6 @@ UNIFIED_SOURCES += [ 'src/base/file_path.cc', 'src/base/file_util.cc', 'src/base/histogram.cc', - 'src/base/lock.cc', 'src/base/logging.cc', 'src/base/message_loop.cc', 'src/base/message_pump_default.cc', diff --git a/ipc/chromium/src/base/condition_variable.h b/ipc/chromium/src/base/condition_variable.h index 63610e0e3b..f274c8eabe 100644 --- a/ipc/chromium/src/base/condition_variable.h +++ b/ipc/chromium/src/base/condition_variable.h @@ -1,5 +1,4 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -66,7 +65,17 @@ #ifndef BASE_CONDITION_VARIABLE_H_ #define BASE_CONDITION_VARIABLE_H_ +#include "base/basictypes.h" #include "base/lock.h" +#include "build/build_config.h" + +#if defined(OS_POSIX) +#include <pthread.h> +#endif + +#if defined(OS_WIN) +#include <windows.h> +#endif namespace base { class TimeDelta; @@ -80,11 +89,13 @@ class ConditionVariable { ~ConditionVariable(); // Wait() releases the caller's critical section atomically as it starts to - // sleep, and the reacquires it when it is signaled. + // sleep, and the reacquires it when it is signaled. The wait functions are + // susceptible to spurious wakeups. (See usage note 1 for more details.) void Wait(); void TimedWait(const base::TimeDelta& max_time); - // Broadcast() revives all waiting threads. + // Broadcast() revives all waiting threads. (See usage note 2 for more + // details.) void Broadcast(); // Signal() revives one waiting thread. void Signal(); @@ -92,81 +103,11 @@ class ConditionVariable { private: #if defined(OS_WIN) - - // Define Event class that is used to form circularly linked lists. - // The list container is an element with NULL as its handle_ value. - // The actual list elements have a non-zero handle_ value. - // All calls to methods MUST be done under protection of a lock so that links - // can be validated. Without the lock, some links might asynchronously - // change, and the assertions would fail (as would list change operations). - class Event { - public: - // Default constructor with no arguments creates a list container. - Event(); - ~Event(); - - // InitListElement transitions an instance from a container, to an element. - void InitListElement(); - - // Methods for use on lists. - bool IsEmpty() const; - void PushBack(Event* other); - Event* PopFront(); - Event* PopBack(); - - // Methods for use on list elements. - // Accessor method. - HANDLE handle() const; - // Pull an element from a list (if it's in one). - Event* Extract(); - - // Method for use on a list element or on a list. - bool IsSingleton() const; - - private: - // Provide pre/post conditions to validate correct manipulations. - bool ValidateAsDistinct(Event* other) const; - bool ValidateAsItem() const; - bool ValidateAsList() const; - bool ValidateLinks() const; - - HANDLE handle_; - Event* next_; - Event* prev_; - DISALLOW_COPY_AND_ASSIGN(Event); - }; - - // Note that RUNNING is an unlikely number to have in RAM by accident. - // This helps with defensive destructor coding in the face of user error. - enum RunState { SHUTDOWN = 0, RUNNING = 64213 }; - - // Internal implementation methods supporting Wait(). - Event* GetEventForWaiting(); - void RecycleEvent(Event* used_event); - - RunState run_state_; - - // Private critical section for access to member data. - Lock internal_lock_; - - // Lock that is acquired before calling Wait(). - Lock& user_lock_; - - // Events that threads are blocked on. - Event waiting_list_; - - // Free list for old events. - Event recycling_list_; - int recycling_list_size_; - - // The number of allocated, but not yet deleted events. - int allocation_counter_; - + CONDITION_VARIABLE cv_; + SRWLOCK* const srwlock_; #elif defined(OS_POSIX) - pthread_cond_t condition_; pthread_mutex_t* user_mutex_; - #endif DISALLOW_COPY_AND_ASSIGN(ConditionVariable); diff --git a/ipc/chromium/src/base/condition_variable_posix.cc b/ipc/chromium/src/base/condition_variable_posix.cc index 4a8024c2bc..8b689f5e5d 100644 --- a/ipc/chromium/src/base/condition_variable_posix.cc +++ b/ipc/chromium/src/base/condition_variable_posix.cc @@ -1,23 +1,19 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "base/condition_variable.h" #include <errno.h> +#include <stdint.h> #include <sys/time.h> #include "base/lock.h" -#include "base/lock_impl.h" -#include "base/logging.h" #include "base/time.h" - -using base::Time; -using base::TimeDelta; +#include "build/build_config.h" ConditionVariable::ConditionVariable(Lock* user_lock) - : user_mutex_(user_lock->lock_impl()->os_lock()) { + : user_mutex_(user_lock->lock_.native_handle()) { int rv = 0; #if !defined(OS_MACOSX) pthread_condattr_t attrs; @@ -33,52 +29,66 @@ ConditionVariable::ConditionVariable(Lock* user_lock) } ConditionVariable::~ConditionVariable() { +#if defined(OS_MACOSX) + // This hack is necessary to avoid a fatal pthreads subsystem bug in the + // Darwin kernel. http://crbug.com/517681. + { + Lock lock; + AutoLock l(lock); + struct timespec ts; + ts.tv_sec = 0; + ts.tv_nsec = 1; + pthread_cond_timedwait_relative_np(&condition_, lock.lock_.native_handle(), + &ts); + } +#endif int rv = pthread_cond_destroy(&condition_); - DCHECK(rv == 0); + DCHECK_EQ(0, rv); } void ConditionVariable::Wait() { int rv = pthread_cond_wait(&condition_, user_mutex_); - DCHECK(rv == 0); + DCHECK_EQ(0, rv); } -void ConditionVariable::TimedWait(const TimeDelta& max_time) { +void ConditionVariable::TimedWait(const base::TimeDelta& max_time) { int64_t usecs = max_time.InMicroseconds(); - struct timespec relative_time; - relative_time.tv_sec = usecs / Time::kMicrosecondsPerSecond; + relative_time.tv_sec = usecs / base::Time::kMicrosecondsPerSecond; relative_time.tv_nsec = - (usecs % Time::kMicrosecondsPerSecond) * Time::kNanosecondsPerMicrosecond; + (usecs % base::Time::kMicrosecondsPerSecond) * base::Time::kNanosecondsPerMicrosecond; #if defined(OS_MACOSX) int rv = pthread_cond_timedwait_relative_np( &condition_, user_mutex_, &relative_time); #else // The timeout argument to pthread_cond_timedwait is in absolute time. + struct timespec absolute_time; struct timespec now; clock_gettime(CLOCK_MONOTONIC, &now); - - struct timespec absolute_time; absolute_time.tv_sec = now.tv_sec; absolute_time.tv_nsec = now.tv_nsec; absolute_time.tv_sec += relative_time.tv_sec; absolute_time.tv_nsec += relative_time.tv_nsec; - absolute_time.tv_sec += absolute_time.tv_nsec / Time::kNanosecondsPerSecond; - absolute_time.tv_nsec %= Time::kNanosecondsPerSecond; + absolute_time.tv_sec += absolute_time.tv_nsec / base::Time::kNanosecondsPerSecond; + absolute_time.tv_nsec %= base::Time::kNanosecondsPerSecond; DCHECK_GE(absolute_time.tv_sec, now.tv_sec); // Overflow paranoia int rv = pthread_cond_timedwait(&condition_, user_mutex_, &absolute_time); #endif // OS_MACOSX + + // On failure, we only expect the CV to timeout. Any other error value means + // that we've unexpectedly woken up. DCHECK(rv == 0 || rv == ETIMEDOUT); } void ConditionVariable::Broadcast() { int rv = pthread_cond_broadcast(&condition_); - DCHECK(rv == 0); + DCHECK_EQ(0, rv); } void ConditionVariable::Signal() { int rv = pthread_cond_signal(&condition_); - DCHECK(rv == 0); + DCHECK_EQ(0, rv); } diff --git a/ipc/chromium/src/base/condition_variable_win.cc b/ipc/chromium/src/base/condition_variable_win.cc index 8737781375..468fe8951d 100644 --- a/ipc/chromium/src/base/condition_variable_win.cc +++ b/ipc/chromium/src/base/condition_variable_win.cc @@ -1,447 +1,47 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "base/condition_variable.h" -#include <stack> - #include "base/lock.h" #include "base/logging.h" #include "base/time.h" -using base::TimeDelta; - ConditionVariable::ConditionVariable(Lock* user_lock) - : user_lock_(*user_lock), - run_state_(RUNNING), - allocation_counter_(0), - recycling_list_size_(0) { + : srwlock_(user_lock->lock_.native_handle()) +{ DCHECK(user_lock); + InitializeConditionVariable(&cv_); } -ConditionVariable::~ConditionVariable() { - AutoLock auto_lock(internal_lock_); - run_state_ = SHUTDOWN; // Prevent any more waiting. - DCHECK_EQ(recycling_list_size_, allocation_counter_); - if (recycling_list_size_ != allocation_counter_) { // Rare shutdown problem. - // There are threads of execution still in this->TimedWait() and yet the - // caller has instigated the destruction of this instance :-/. - // A common reason for such "overly hasty" destruction is that the caller - // was not willing to wait for all the threads to terminate. Such hasty - // actions are a violation of our usage contract, but we'll give the - // waiting thread(s) one last chance to exit gracefully (prior to our - // destruction). - // Note: waiting_list_ *might* be empty, but recycling is still pending. - AutoUnlock auto_unlock(internal_lock_); - Broadcast(); // Make sure all waiting threads have been signaled. - Sleep(10); // Give threads a chance to grab internal_lock_. - // All contained threads should be blocked on user_lock_ by now :-). - } // Reacquire internal_lock_. - - DCHECK_EQ(recycling_list_size_, allocation_counter_); -} +ConditionVariable::~ConditionVariable() = default; void ConditionVariable::Wait() { // Default to "wait forever" timing, which means have to get a Signal() // or Broadcast() to come out of this wait state. - TimedWait(TimeDelta::FromMilliseconds(INFINITE)); + TimedWait(base::TimeDelta::FromMilliseconds(INFINITE)); } -void ConditionVariable::TimedWait(const TimeDelta& max_time) { - Event* waiting_event; - HANDLE handle; - { - AutoLock auto_lock(internal_lock_); - if (RUNNING != run_state_) return; // Destruction in progress. - waiting_event = GetEventForWaiting(); - handle = waiting_event->handle(); - DCHECK(handle); - } // Release internal_lock. +void ConditionVariable::TimedWait(const base::TimeDelta& max_time) { + DWORD timeout = static_cast<DWORD>(max_time.InMilliseconds()); - { - AutoUnlock unlock(user_lock_); // Release caller's lock - WaitForSingleObject(handle, static_cast<DWORD>(max_time.InMilliseconds())); - // Minimize spurious signal creation window by recycling asap. - AutoLock auto_lock(internal_lock_); - RecycleEvent(waiting_event); - // Release internal_lock_ - } // Reacquire callers lock to depth at entry. + if (!SleepConditionVariableSRW(&cv_, srwlock_, timeout, 0)) { + // On failure, we only expect the CV to timeout. Any other error value means + // that we've unexpectedly woken up. + // Note that WAIT_TIMEOUT != ERROR_TIMEOUT. WAIT_TIMEOUT is used with the + // WaitFor* family of functions as a direct return value. ERROR_TIMEOUT is + // used with GetLastError(). + DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError()); + } } -// Broadcast() is guaranteed to signal all threads that were waiting (i.e., had -// a cv_event internally allocated for them) before Broadcast() was called. void ConditionVariable::Broadcast() { - std::stack<HANDLE> handles; // See FAQ-question-10. - { - AutoLock auto_lock(internal_lock_); - if (waiting_list_.IsEmpty()) - return; - while (!waiting_list_.IsEmpty()) - // This is not a leak from waiting_list_. See FAQ-question 12. - handles.push(waiting_list_.PopBack()->handle()); - } // Release internal_lock_. - while (!handles.empty()) { - SetEvent(handles.top()); - handles.pop(); - } + WakeAllConditionVariable(&cv_); } -// Signal() will select one of the waiting threads, and signal it (signal its -// cv_event). For better performance we signal the thread that went to sleep -// most recently (LIFO). If we want fairness, then we wake the thread that has -// been sleeping the longest (FIFO). void ConditionVariable::Signal() { - HANDLE handle; - { - AutoLock auto_lock(internal_lock_); - if (waiting_list_.IsEmpty()) - return; // No one to signal. - // Only performance option should be used. - // This is not a leak from waiting_list. See FAQ-question 12. - handle = waiting_list_.PopBack()->handle(); // LIFO. - } // Release internal_lock_. - SetEvent(handle); -} - -// GetEventForWaiting() provides a unique cv_event for any caller that needs to -// wait. This means that (worst case) we may over time create as many cv_event -// objects as there are threads simultaneously using this instance's Wait() -// functionality. -ConditionVariable::Event* ConditionVariable::GetEventForWaiting() { - // We hold internal_lock, courtesy of Wait(). - Event* cv_event; - if (0 == recycling_list_size_) { - DCHECK(recycling_list_.IsEmpty()); - cv_event = new Event(); - cv_event->InitListElement(); - allocation_counter_++; - // CHECK_NE is not defined in our codebase, so we have to use CHECK - CHECK(cv_event->handle()); - } else { - cv_event = recycling_list_.PopFront(); - recycling_list_size_--; - } - waiting_list_.PushBack(cv_event); - return cv_event; -} - -// RecycleEvent() takes a cv_event that was previously used for Wait()ing, and -// recycles it for use in future Wait() calls for this or other threads. -// Note that there is a tiny chance that the cv_event is still signaled when we -// obtain it, and that can cause spurious signals (if/when we re-use the -// cv_event), but such is quite rare (see FAQ-question-5). -void ConditionVariable::RecycleEvent(Event* used_event) { - // We hold internal_lock, courtesy of Wait(). - // If the cv_event timed out, then it is necessary to remove it from - // waiting_list_. If it was selected by Broadcast() or Signal(), then it is - // already gone. - used_event->Extract(); // Possibly redundant - recycling_list_.PushBack(used_event); - recycling_list_size_++; -} -//------------------------------------------------------------------------------ -// The next section provides the implementation for the private Event class. -//------------------------------------------------------------------------------ - -// Event provides a doubly-linked-list of events for use exclusively by the -// ConditionVariable class. - -// This custom container was crafted because no simple combination of STL -// classes appeared to support the functionality required. The specific -// unusual requirement for a linked-list-class is support for the Extract() -// method, which can remove an element from a list, potentially for insertion -// into a second list. Most critically, the Extract() method is idempotent, -// turning the indicated element into an extracted singleton whether it was -// contained in a list or not. This functionality allows one (or more) of -// threads to do the extraction. The iterator that identifies this extractable -// element (in this case, a pointer to the list element) can be used after -// arbitrary manipulation of the (possibly) enclosing list container. In -// general, STL containers do not provide iterators that can be used across -// modifications (insertions/extractions) of the enclosing containers, and -// certainly don't provide iterators that can be used if the identified -// element is *deleted* (removed) from the container. - -// It is possible to use multiple redundant containers, such as an STL list, -// and an STL map, to achieve similar container semantics. This container has -// only O(1) methods, while the corresponding (multiple) STL container approach -// would have more complex O(log(N)) methods (yeah... N isn't that large). -// Multiple containers also makes correctness more difficult to assert, as -// data is redundantly stored and maintained, which is generally evil. - -ConditionVariable::Event::Event() : handle_(0) { - next_ = prev_ = this; // Self referencing circular. -} - -ConditionVariable::Event::~Event() { - if (0 == handle_) { - // This is the list holder - while (!IsEmpty()) { - Event* cv_event = PopFront(); - DCHECK(cv_event->ValidateAsItem()); - delete cv_event; - } - } - DCHECK(IsSingleton()); - if (0 != handle_) { - int ret_val = CloseHandle(handle_); - DCHECK(ret_val); - } -} - -// Change a container instance permanently into an element of a list. -void ConditionVariable::Event::InitListElement() { - DCHECK(!handle_); - handle_ = CreateEvent(NULL, false, false, NULL); - CHECK(handle_); -} - -// Methods for use on lists. -bool ConditionVariable::Event::IsEmpty() const { - DCHECK(ValidateAsList()); - return IsSingleton(); -} - -void ConditionVariable::Event::PushBack(Event* other) { - DCHECK(ValidateAsList()); - DCHECK(other->ValidateAsItem()); - DCHECK(other->IsSingleton()); - // Prepare other for insertion. - other->prev_ = prev_; - other->next_ = this; - // Cut into list. - prev_->next_ = other; - prev_ = other; - DCHECK(ValidateAsDistinct(other)); -} - -ConditionVariable::Event* ConditionVariable::Event::PopFront() { - DCHECK(ValidateAsList()); - DCHECK(!IsSingleton()); - return next_->Extract(); -} - -ConditionVariable::Event* ConditionVariable::Event::PopBack() { - DCHECK(ValidateAsList()); - DCHECK(!IsSingleton()); - return prev_->Extract(); -} - -// Methods for use on list elements. -// Accessor method. -HANDLE ConditionVariable::Event::handle() const { - DCHECK(ValidateAsItem()); - return handle_; -} - -// Pull an element from a list (if it's in one). -ConditionVariable::Event* ConditionVariable::Event::Extract() { - DCHECK(ValidateAsItem()); - if (!IsSingleton()) { - // Stitch neighbors together. - next_->prev_ = prev_; - prev_->next_ = next_; - // Make extractee into a singleton. - prev_ = next_ = this; - } - DCHECK(IsSingleton()); - return this; -} - -// Method for use on a list element or on a list. -bool ConditionVariable::Event::IsSingleton() const { - DCHECK(ValidateLinks()); - return next_ == this; -} - -// Provide pre/post conditions to validate correct manipulations. -bool ConditionVariable::Event::ValidateAsDistinct(Event* other) const { - return ValidateLinks() && other->ValidateLinks() && (this != other); -} - -bool ConditionVariable::Event::ValidateAsItem() const { - return (0 != handle_) && ValidateLinks(); -} - -bool ConditionVariable::Event::ValidateAsList() const { - return (0 == handle_) && ValidateLinks(); + WakeConditionVariable(&cv_); } -bool ConditionVariable::Event::ValidateLinks() const { - // Make sure both of our neighbors have links that point back to us. - // We don't do the O(n) check and traverse the whole loop, and instead only - // do a local check to (and returning from) our immediate neighbors. - return (next_->prev_ == this) && (prev_->next_ == this); -} - - -/* -FAQ On subtle implementation details: - -1) What makes this problem subtle? Please take a look at "Strategies -for Implementing POSIX Condition Variables on Win32" by Douglas -C. Schmidt and Irfan Pyarali. -http://www.cs.wustl.edu/~schmidt/win32-cv-1.html It includes -discussions of numerous flawed strategies for implementing this -functionality. I'm not convinced that even the final proposed -implementation has semantics that are as nice as this implementation -(especially with regard to Broadcast() and the impact on threads that -try to Wait() after a Broadcast() has been called, but before all the -original waiting threads have been signaled). - -2) Why can't you use a single wait_event for all threads that call -Wait()? See FAQ-question-1, or consider the following: If a single -event were used, then numerous threads calling Wait() could release -their cs locks, and be preempted just before calling -WaitForSingleObject(). If a call to Broadcast() was then presented on -a second thread, it would be impossible to actually signal all -waiting(?) threads. Some number of SetEvent() calls *could* be made, -but there could be no guarantee that those led to to more than one -signaled thread (SetEvent()'s may be discarded after the first!), and -there could be no guarantee that the SetEvent() calls didn't just -awaken "other" threads that hadn't even started waiting yet (oops). -Without any limit on the number of requisite SetEvent() calls, the -system would be forced to do many such calls, allowing many new waits -to receive spurious signals. - -3) How does this implementation cause spurious signal events? The -cause in this implementation involves a race between a signal via -time-out and a signal via Signal() or Broadcast(). The series of -actions leading to this are: - -a) Timer fires, and a waiting thread exits the line of code: - - WaitForSingleObject(waiting_event, max_time.InMilliseconds()); - -b) That thread (in (a)) is randomly pre-empted after the above line, -leaving the waiting_event reset (unsignaled) and still in the -waiting_list_. - -c) A call to Signal() (or Broadcast()) on a second thread proceeds, and -selects the waiting cv_event (identified in step (b)) as the event to revive -via a call to SetEvent(). - -d) The Signal() method (step c) calls SetEvent() on waiting_event (step b). - -e) The waiting cv_event (step b) is now signaled, but no thread is -waiting on it. - -f) When that waiting_event (step b) is reused, it will immediately -be signaled (spuriously). - - -4) Why do you recycle events, and cause spurious signals? First off, -the spurious events are very rare. They can only (I think) appear -when the race described in FAQ-question-3 takes place. This should be -very rare. Most(?) uses will involve only timer expiration, or only -Signal/Broadcast() actions. When both are used, it will be rare that -the race will appear, and it would require MANY Wait() and signaling -activities. If this implementation did not recycle events, then it -would have to create and destroy events for every call to Wait(). -That allocation/deallocation and associated construction/destruction -would be costly (per wait), and would only be a rare benefit (when the -race was "lost" and a spurious signal took place). That would be bad -(IMO) optimization trade-off. Finally, such spurious events are -allowed by the specification of condition variables (such as -implemented in Vista), and hence it is better if any user accommodates -such spurious events (see usage note in condition_variable.h). - -5) Why don't you reset events when you are about to recycle them, or -about to reuse them, so that the spurious signals don't take place? -The thread described in FAQ-question-3 step c may be pre-empted for an -arbitrary length of time before proceeding to step d. As a result, -the wait_event may actually be re-used *before* step (e) is reached. -As a result, calling reset would not help significantly. - -6) How is it that the callers lock is released atomically with the -entry into a wait state? We commit to the wait activity when we -allocate the wait_event for use in a given call to Wait(). This -allocation takes place before the caller's lock is released (and -actually before our internal_lock_ is released). That allocation is -the defining moment when "the wait state has been entered," as that -thread *can* now be signaled by a call to Broadcast() or Signal(). -Hence we actually "commit to wait" before releasing the lock, making -the pair effectively atomic. - -8) Why do you need to lock your data structures during waiting, as the -caller is already in possession of a lock? We need to Acquire() and -Release() our internal lock during Signal() and Broadcast(). If we tried -to use a callers lock for this purpose, we might conflict with their -external use of the lock. For example, the caller may use to consistently -hold a lock on one thread while calling Signal() on another, and that would -block Signal(). - -9) Couldn't a more efficient implementation be provided if you -preclude using more than one external lock in conjunction with a -single ConditionVariable instance? Yes, at least it could be viewed -as a simpler API (since you don't have to reiterate the lock argument -in each Wait() call). One of the constructors now takes a specific -lock as an argument, and a there are corresponding Wait() calls that -don't specify a lock now. It turns that the resulting implmentation -can't be made more efficient, as the internal lock needs to be used by -Signal() and Broadcast(), to access internal data structures. As a -result, I was not able to utilize the user supplied lock (which is -being used by the user elsewhere presumably) to protect the private -member access. - -9) Since you have a second lock, how can be be sure that there is no -possible deadlock scenario? Our internal_lock_ is always the last -lock acquired, and the first one released, and hence a deadlock (due -to critical section problems) is impossible as a consequence of our -lock. - -10) When doing a Broadcast(), why did you copy all the events into -an STL queue, rather than making a linked-loop, and iterating over it? -The iterating during Broadcast() is done so outside the protection -of the internal lock. As a result, other threads, such as the thread -wherein a related event is waiting, could asynchronously manipulate -the links around a cv_event. As a result, the link structure cannot -be used outside a lock. Broadcast() could iterate over waiting -events by cycling in-and-out of the protection of the internal_lock, -but that appears more expensive than copying the list into an STL -stack. - -11) Why did the lock.h file need to be modified so much for this -change? Central to a Condition Variable is the atomic release of a -lock during a Wait(). This places Wait() functionality exactly -mid-way between the two classes, Lock and Condition Variable. Given -that there can be nested Acquire()'s of locks, and Wait() had to -Release() completely a held lock, it was necessary to augment the Lock -class with a recursion counter. Even more subtle is the fact that the -recursion counter (in a Lock) must be protected, as many threads can -access it asynchronously. As a positive fallout of this, there are -now some DCHECKS to be sure no one Release()s a Lock more than they -Acquire()ed it, and there is ifdef'ed functionality that can detect -nested locks (legal under windows, but not under Posix). - -12) Why is it that the cv_events removed from list in Broadcast() and Signal() -are not leaked? How are they recovered?? The cv_events that appear to leak are -taken from the waiting_list_. For each element in that list, there is currently -a thread in or around the WaitForSingleObject() call of Wait(), and those -threads have references to these otherwise leaked events. They are passed as -arguments to be recycled just aftre returning from WaitForSingleObject(). - -13) Why did you use a custom container class (the linked list), when STL has -perfectly good containers, such as an STL list? The STL list, as with any -container, does not guarantee the utility of an iterator across manipulation -(such as insertions and deletions) of the underlying container. The custom -double-linked-list container provided that assurance. I don't believe any -combination of STL containers provided the services that were needed at the same -O(1) efficiency as the custom linked list. The unusual requirement -for the container class is that a reference to an item within a container (an -iterator) needed to be maintained across an arbitrary manipulation of the -container. This requirement exposes itself in the Wait() method, where a -waiting_event must be selected prior to the WaitForSingleObject(), and then it -must be used as part of recycling to remove the related instance from the -waiting_list. A hash table (STL map) could be used, but I was embarrased to -use a complex and relatively low efficiency container when a doubly linked list -provided O(1) performance in all required operations. Since other operations -to provide performance-and/or-fairness required queue (FIFO) and list (LIFO) -containers, I would also have needed to use an STL list/queue as well as an STL -map. In the end I decided it would be "fun" to just do it right, and I -put so many assertions (DCHECKs) into the container class that it is trivial to -code review and validate its correctness. - -*/ diff --git a/ipc/chromium/src/base/lock.cc b/ipc/chromium/src/base/lock.cc deleted file mode 100644 index a34d3726e0..0000000000 --- a/ipc/chromium/src/base/lock.cc +++ /dev/null @@ -1,8 +0,0 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -// Lock class. - -// Depricated file. See lock_impl_*.cc for platform specific versions. diff --git a/ipc/chromium/src/base/lock.h b/ipc/chromium/src/base/lock.h index 66fb110002..236c2a1925 100644 --- a/ipc/chromium/src/base/lock.h +++ b/ipc/chromium/src/base/lock.h @@ -1,38 +1,60 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #ifndef BASE_LOCK_H_ #define BASE_LOCK_H_ +#include "base/basictypes.h" #include "base/lock_impl.h" +#include "base/platform_thread.h" +#include "build/build_config.h" // A convenient wrapper for an OS specific critical section. - class Lock { public: + // Optimized wrapper implementation Lock() : lock_() {} ~Lock() {} void Acquire() { lock_.Lock(); } void Release() { lock_.Unlock(); } + // If the lock is not held, take it and return true. If the lock is already - // held by another thread, immediately return false. + // held by another thread, immediately return false. This must not be called + // by a thread already holding the lock (what happens is undefined and an + // assertion may fail). bool Try() { return lock_.Try(); } - // In debug builds this method checks that the lock has been acquired by the - // calling thread. If the lock has not been acquired, then the method - // will DCHECK(). In non-debug builds, the LockImpl's implementation of - // AssertAcquired() is an empty inline method. - void AssertAcquired() const { return lock_.AssertAcquired(); } + // Null implementation if not debug. + void AssertAcquired() const {} + + // Whether Lock mitigates priority inversion when used from different thread + // priorities. + static bool HandlesMultipleThreadPriorities() { +#if defined(OS_POSIX) + // POSIX mitigates priority inversion by setting the priority of a thread + // holding a Lock to the maximum priority of any other thread waiting on it. + return base::internal::LockImpl::PriorityInheritanceAvailable(); +#elif defined(OS_WIN) + // Windows mitigates priority inversion by randomly boosting the priority of + // ready threads. + // https://msdn.microsoft.com/library/windows/desktop/ms684831.aspx + return true; +#else +#error Unsupported platform +#endif + } - // Return the underlying lock implementation. - // TODO(awalker): refactor lock and condition variables so that this is - // unnecessary. - LockImpl* lock_impl() { return &lock_; } +#if defined(OS_POSIX) || defined(OS_WIN) + // Both Windows and POSIX implementations of ConditionVariable need to be + // able to see our lock and tweak our debugging counters, as they release and + // acquire locks inside of their condition variable APIs. + friend class ConditionVariable; +#endif private: - LockImpl lock_; // Platform specific underlying lock implementation. + // Platform specific underlying lock implementation. + ::base::internal::LockImpl lock_; DISALLOW_COPY_AND_ASSIGN(Lock); }; @@ -40,10 +62,16 @@ class Lock { // A helper class that acquires the given Lock while the AutoLock is in scope. class AutoLock { public: + struct AlreadyAcquired {}; + explicit AutoLock(Lock& lock) : lock_(lock) { lock_.Acquire(); } + AutoLock(Lock& lock, const AlreadyAcquired&) : lock_(lock) { + lock_.AssertAcquired(); + } + ~AutoLock() { lock_.AssertAcquired(); lock_.Release(); diff --git a/ipc/chromium/src/base/lock_impl.h b/ipc/chromium/src/base/lock_impl.h index 67105bd57c..a9a52d4b79 100644 --- a/ipc/chromium/src/base/lock_impl.h +++ b/ipc/chromium/src/base/lock_impl.h @@ -1,11 +1,11 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #ifndef BASE_LOCK_IMPL_H_ #define BASE_LOCK_IMPL_H_ +#include "base/basictypes.h" #include "build/build_config.h" #if defined(OS_WIN) @@ -14,8 +14,8 @@ #include <pthread.h> #endif -#include "base/basictypes.h" -#include "base/platform_thread.h" +namespace base { +namespace internal { // This class implements the underlying platform-specific spin-lock mechanism // used for the Lock class. Most users should not use LockImpl directly, but @@ -23,9 +23,9 @@ class LockImpl { public: #if defined(OS_WIN) - typedef CRITICAL_SECTION OSLockType; + using NativeHandle = SRWLOCK; #elif defined(OS_POSIX) - typedef pthread_mutex_t OSLockType; + using NativeHandle = pthread_mutex_t; #endif LockImpl(); @@ -42,37 +42,23 @@ class LockImpl { // a successful call to Try, or a call to Lock. void Unlock(); - // Debug-only method that will DCHECK() if the lock is not acquired by the - // current thread. In non-debug builds, no check is performed. - // Because linux and mac condition variables modify the underlyning lock - // through the os_lock() method, runtime assertions can not be done on those - // builds. -#if defined(NDEBUG) || !defined(OS_WIN) - void AssertAcquired() const {} -#else - void AssertAcquired() const; -#endif - - // Return the native underlying lock. Not supported for Windows builds. + // Return the native underlying lock. // TODO(awalker): refactor lock and condition variables so that this is // unnecessary. -#if !defined(OS_WIN) - OSLockType* os_lock() { return &os_lock_; } + NativeHandle* native_handle() { return &native_handle_; } + +#if defined(OS_POSIX) + // Whether this lock will attempt to use priority inheritance. + static bool PriorityInheritanceAvailable(); #endif private: - OSLockType os_lock_; - -#if !defined(NDEBUG) && defined(OS_WIN) - // All private data is implicitly protected by lock_. - // Be VERY careful to only access members under that lock. - PlatformThreadId owning_thread_id_; - int32_t recursion_count_shadow_; - bool recursion_used_; // Allow debugging to continued after a DCHECK(). -#endif // NDEBUG + NativeHandle native_handle_; DISALLOW_COPY_AND_ASSIGN(LockImpl); }; +} // namespace internal +} // namespace base #endif // BASE_LOCK_IMPL_H_ diff --git a/ipc/chromium/src/base/lock_impl_posix.cc b/ipc/chromium/src/base/lock_impl_posix.cc index 0513cf0e65..b0fbfe5f04 100644 --- a/ipc/chromium/src/base/lock_impl_posix.cc +++ b/ipc/chromium/src/base/lock_impl_posix.cc @@ -1,49 +1,85 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "base/lock_impl.h" #include <errno.h> +#include <string.h> #include "base/logging.h" +#include "base/lock.h" + +namespace base { +namespace internal { + +#define PRIORITY_INHERITANCE_LOCKS_POSSIBLE() 1 LockImpl::LockImpl() { -#ifndef NDEBUG - // In debug, setup attributes for lock error checking. pthread_mutexattr_t mta; int rv = pthread_mutexattr_init(&mta); - DCHECK_EQ(rv, 0); + + DCHECK_EQ(rv, 0) << ". " << strerror(rv); +#if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() + if (PriorityInheritanceAvailable()) { + rv = pthread_mutexattr_setprotocol(&mta, PTHREAD_PRIO_INHERIT); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); + } +#endif +#ifndef NDEBUG + // In debug, setup attributes for lock error checking. rv = pthread_mutexattr_settype(&mta, PTHREAD_MUTEX_ERRORCHECK); - DCHECK_EQ(rv, 0); - rv = pthread_mutex_init(&os_lock_, &mta); - DCHECK_EQ(rv, 0); - rv = pthread_mutexattr_destroy(&mta); - DCHECK_EQ(rv, 0); -#else - // In release, go with the default lock attributes. - pthread_mutex_init(&os_lock_, NULL); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); #endif + rv = pthread_mutex_init(&native_handle_, &mta); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); + rv = pthread_mutexattr_destroy(&mta); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); } LockImpl::~LockImpl() { - int rv = pthread_mutex_destroy(&os_lock_); - DCHECK_EQ(rv, 0); + int rv = pthread_mutex_destroy(&native_handle_); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); } bool LockImpl::Try() { - int rv = pthread_mutex_trylock(&os_lock_); - DCHECK(rv == 0 || rv == EBUSY); - return rv == 0; + int rv = pthread_mutex_trylock(&native_handle_); + DCHECK(rv == 0 || rv == EBUSY) << ". " << strerror(rv); } void LockImpl::Lock() { - int rv = pthread_mutex_lock(&os_lock_); - DCHECK_EQ(rv, 0); + int rv = pthread_mutex_lock(&native_handle_); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); } void LockImpl::Unlock() { - int rv = pthread_mutex_unlock(&os_lock_); - DCHECK_EQ(rv, 0); + int rv = pthread_mutex_unlock(&native_handle_); + DCHECK_EQ(rv, 0) << ". " << strerror(rv); +} + +// static +bool LockImpl::PriorityInheritanceAvailable() { +#if PRIORITY_INHERITANCE_LOCKS_POSSIBLE() && defined(OS_MACOSX) + return true; +#else + // Security concerns prevent the use of priority inheritance mutexes on Linux. + // * CVE-2010-0622 - wake_futex_pi unlocks incorrect, possible DoS. + // https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2010-0622 + // * CVE-2012-6647 - Linux < 3.5.1, futex_wait_requeue_pi possible DoS. + // https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2012-6647 + // * CVE-2014-3153 - Linux <= 3.14.5, futex_requeue, privilege escalation. + // https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-3153 + // + // If the above were all addressed, we still need a runtime check to deal with + // the bug below. + // * glibc Bug 14652: https://sourceware.org/bugzilla/show_bug.cgi?id=14652 + // Fixed in glibc 2.17. + // Priority inheritance mutexes may deadlock with condition variables + // during recacquisition of the mutex after the condition variable is + // signalled. + return false; +#endif } + +} // namespace internal +} // namespace base diff --git a/ipc/chromium/src/base/lock_impl_win.cc b/ipc/chromium/src/base/lock_impl_win.cc index 17d72ea962..c8f2191441 100644 --- a/ipc/chromium/src/base/lock_impl_win.cc +++ b/ipc/chromium/src/base/lock_impl_win.cc @@ -1,74 +1,27 @@ -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ -// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. +// Copyright (c) 2011 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. #include "base/lock_impl.h" -#include "base/logging.h" -// NOTE: Although windows critical sections support recursive locks, we do not -// allow this, and we will commonly fire a DCHECK() if a thread attempts to -// acquire the lock a second time (while already holding it). +namespace base { +namespace internal { -LockImpl::LockImpl() { -#ifndef NDEBUG - recursion_count_shadow_ = 0; - recursion_used_ = false; - owning_thread_id_ = 0; -#endif // NDEBUG - // The second parameter is the spin count, for short-held locks it avoid the - // contending thread from going to sleep which helps performance greatly. - ::InitializeCriticalSectionAndSpinCount(&os_lock_, 2000); -} +LockImpl::LockImpl() : native_handle_(SRWLOCK_INIT) {} -LockImpl::~LockImpl() { - ::DeleteCriticalSection(&os_lock_); -} +LockImpl::~LockImpl() = default; bool LockImpl::Try() { - if (::TryEnterCriticalSection(&os_lock_) != FALSE) { -#ifndef NDEBUG - // ONLY access data after locking. - owning_thread_id_ = PlatformThread::CurrentId(); - DCHECK_NE(owning_thread_id_, 0); - recursion_count_shadow_++; - if (2 == recursion_count_shadow_ && !recursion_used_) { - recursion_used_ = true; - DCHECK(false); // Catch accidental redundant lock acquisition. - } -#endif - return true; - } - return false; + return !!::TryAcquireSRWLockExclusive(&native_handle_); } void LockImpl::Lock() { - ::EnterCriticalSection(&os_lock_); -#ifndef NDEBUG - // ONLY access data after locking. - owning_thread_id_ = PlatformThread::CurrentId(); - DCHECK_NE(owning_thread_id_, 0); - recursion_count_shadow_++; - if (2 == recursion_count_shadow_ && !recursion_used_) { - recursion_used_ = true; - DCHECK(false); // Catch accidental redundant lock acquisition. - } -#endif // NDEBUG + ::AcquireSRWLockExclusive(&native_handle_); } void LockImpl::Unlock() { -#ifndef NDEBUG - --recursion_count_shadow_; // ONLY access while lock is still held. - DCHECK(0 <= recursion_count_shadow_); - owning_thread_id_ = 0; -#endif // NDEBUG - ::LeaveCriticalSection(&os_lock_); + ::ReleaseSRWLockExclusive(&native_handle_); } -// In non-debug builds, this method is declared as an empty inline method. -#ifndef NDEBUG -void LockImpl::AssertAcquired() const { - DCHECK(recursion_count_shadow_ > 0); - DCHECK_EQ(owning_thread_id_, PlatformThread::CurrentId()); -} -#endif +} // namespace internal +} // namespace base |