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

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);
+    for (ModuleCallback& m : modules_) {
-    const auto& found = std::find(modules_.begin(), modules_.end(), cb);
+      if (m.module == module)
-    DCHECK(found != modules_.end()) << "programmer error?";
+        m.next_callback = 0;
-    (*found).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);
+    for (const ModuleCallback& mc : modules_) {
-    if (std::find(modules_.begin(), modules_.end(), cb) != modules_.end())
+      if (mc.module == module)
-      return -1;
+        return -1;
-    modules_.push_front(cb);
+    }
+
+    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<unsigned long>(time_to_wait));
 

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 RegisterModule(Module* module) override;
-  int32_t DeRegisterModule(const Module* module);
+  int32_t DeRegisterModule(const Module* module) override;
 
  protected:
   static bool Run(void* obj);
   bool Process();
 
  private:
-  rtc::ThreadChecker thread_checker_;
-  const rtc::scoped_ptr<EventWrapper> wake_up_;
-  rtc::scoped_ptr<ThreadWrapper> 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<ModuleCallback> 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<EventWrapper> wake_up_;
+  rtc::scoped_ptr<ThreadWrapper> thread_;
+
   ModuleList modules_;
   bool stop_;
 };