From 103f3289b5a8590bc6c08f0de348f83d3d3e0261 Mon Sep 17 00:00:00 2001 From: "tommi@webrtc.org" Date: Sun, 8 Feb 2015 00:48:10 +0000 Subject: [PATCH] Fix the binary layout of ProcessThreadImpl. We apparently hit an obscure problem on mac where seemingly an unaligned mutex causes memory corruption. The effect was that the |modules_| list became corrupt and we crashed. At this point I'm not exactly sure what the alignment requirements are but for now, I've fixed up the layout in a way that doesn't cause these same issues. I'm also changing auto->proper type at the request of drive by reviewers from my previous cl in the same file. TBR=pbos@webrtc.org Review URL: https://webrtc-codereview.appspot.com/38989004 Cr-Commit-Position: refs/heads/master@{#8286} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8286 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../utility/source/process_thread_impl.cc | 27 +++++++++-------- .../utility/source/process_thread_impl.h | 29 ++++++++++++++----- 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/webrtc/modules/utility/source/process_thread_impl.cc b/webrtc/modules/utility/source/process_thread_impl.cc index 9b13daa2b..1e498d470 100644 --- a/webrtc/modules/utility/source/process_thread_impl.cc +++ b/webrtc/modules/utility/source/process_thread_impl.cc @@ -88,24 +88,26 @@ void ProcessThreadImpl::WakeUp(Module* module) { // Allowed to be called on any thread. { rtc::CritScope lock(&lock_); - ModuleCallback cb(module); - const auto& found = std::find(modules_.begin(), modules_.end(), cb); - DCHECK(found != modules_.end()) << "programmer error?"; - (*found).next_callback = 0; + for (ModuleCallback& m : modules_) { + if (m.module == module) + m.next_callback = 0; + } } wake_up_->Set(); } int32_t ProcessThreadImpl::RegisterModule(Module* module) { // Allowed to be called on any thread. + DCHECK(module); { rtc::CritScope lock(&lock_); - // Only allow module to be registered once. - ModuleCallback cb(module); - if (std::find(modules_.begin(), modules_.end(), cb) != modules_.end()) - return -1; - modules_.push_front(cb); + for (const ModuleCallback& mc : modules_) { + if (mc.module == module) + return -1; + } + + modules_.push_back(ModuleCallback(module)); } // Wake the thread calling ProcessThreadImpl::Process() to update the @@ -118,6 +120,7 @@ int32_t ProcessThreadImpl::RegisterModule(Module* module) { int32_t ProcessThreadImpl::DeRegisterModule(const Module* module) { // Allowed to be called on any thread. + DCHECK(module); rtc::CritScope lock(&lock_); modules_.remove_if([&module](const ModuleCallback& m) { return m.module == module; @@ -137,7 +140,7 @@ bool ProcessThreadImpl::Process() { rtc::CritScope lock(&lock_); if (stop_) return false; - for (auto& m : modules_) { + for (ModuleCallback& m : modules_) { // TODO(tommi): Would be good to measure the time TimeUntilNextProcess // takes and dcheck if it takes too long (e.g. >=10ms). Ideally this // operation should not require taking a lock, so querying all modules @@ -150,7 +153,7 @@ bool ProcessThreadImpl::Process() { // Use a new 'now' reference to calculate when the next callback // should occur. We'll continue to use 'now' above for the baseline // of calculating how long we should wait, to reduce variance. - auto new_now = TickTime::MillisecondTimestamp(); + int64_t new_now = TickTime::MillisecondTimestamp(); m.next_callback = GetNextCallbackTime(m.module, new_now); } @@ -159,7 +162,7 @@ bool ProcessThreadImpl::Process() { } } - auto time_to_wait = next_checkpoint - TickTime::MillisecondTimestamp(); + int64_t time_to_wait = next_checkpoint - TickTime::MillisecondTimestamp(); if (time_to_wait > 0) wake_up_->Wait(static_cast(time_to_wait)); diff --git a/webrtc/modules/utility/source/process_thread_impl.h b/webrtc/modules/utility/source/process_thread_impl.h index 5a42140fa..7e01ae5c6 100644 --- a/webrtc/modules/utility/source/process_thread_impl.h +++ b/webrtc/modules/utility/source/process_thread_impl.h @@ -32,29 +32,44 @@ class ProcessThreadImpl : public ProcessThread { void WakeUp(Module* module) override; - int32_t RegisterModule(Module* module); - int32_t DeRegisterModule(const Module* module); + int32_t RegisterModule(Module* module) override; + int32_t DeRegisterModule(const Module* module) override; protected: static bool Run(void* obj); bool Process(); private: - rtc::ThreadChecker thread_checker_; - const rtc::scoped_ptr wake_up_; - rtc::scoped_ptr thread_; - struct ModuleCallback { + ModuleCallback() : module(nullptr), next_callback(0) {} + ModuleCallback(const ModuleCallback& cb) + : module(cb.module), next_callback(cb.next_callback) {} ModuleCallback(Module* module) : module(module), next_callback(0) {} bool operator==(const ModuleCallback& cb) const { return cb.module == module; } + Module* const module; int64_t next_callback; // Absolute timestamp. + + private: + ModuleCallback& operator=(ModuleCallback&); }; - rtc::CriticalSection lock_; // Used to guard modules_ and stop_. typedef std::list ModuleList; + + // Warning: For some reason, if |lock_| comes immediately before |modules_| + // with the current class layout, we will start to have mysterious crashes + // on Mac 10.9 debug. I (Tommi) suspect we're hitting some obscure alignemnt + // issues, but I haven't figured out what they are, if there are alignment + // requirements for mutexes on Mac or if there's something else to it. + // So be careful with changing the layout. + rtc::CriticalSection lock_; // Used to guard modules_ and stop_. + + rtc::ThreadChecker thread_checker_; + const rtc::scoped_ptr wake_up_; + rtc::scoped_ptr thread_; + ModuleList modules_; bool stop_; };