Thread::Stop() must be called before any subclass's destructor completes.

Update Thread documentation, fix all subclasses that had a problem.

This is to avoid a data racing between the destructor modifying the vtable, and
Thread::PreRun calling virtual method Run at the same time.

For example:
[ RUN      ] FileMediaEngineTest.TestGetCapabilities
==================
WARNING: ThreadSanitizer: data race on vptr (ctor/dtor vs virtual call) (pid=2967)
  Read of size 8 at 0x7d480000bd00 by thread T1:
    #0 talk_base::Thread::PreRun(void*) /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/base/thread.cc:353 (libjingle_media_unittest+0x000000234da8)

  Previous write of size 8 at 0x7d480000bd00 by main thread:
    #0 talk_base::Thread::~Thread() /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/base/thread.cc:158 (libjingle_media_unittest+0x00000023478c)
    #1 ~RtpSenderReceiver /mnt/data/b/build/slave/Linux_Tsan_v2/build/src/out/Release/../../talk/media/base/filemediaengine.cc:122 (libjingle_media_unittest+0x0000001b551f)
    ...

RISK=P2
TESTED=try bots and tsan
BUG=2078,2080
R=fischman@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/2428004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@4999 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/base/cpumonitor_unittest.cc

@@ -54,6 +54,9 @@ class BusyThread : public talk_base::Thread {
   BusyThread(double load, double duration, double interval) :
     load_(load), duration_(duration), interval_(interval) {
   }
+  virtual ~BusyThread() {
+    Stop();
+  }
   void Run() {
     Timing time;
     double busy_time = interval_ * load_ / 100.0;

talk/base/dbus.cc

@@ -158,6 +158,10 @@ class DBusMonitor::DBusMonitoringThread : public talk_base::Thread {
     ASSERT(filter_list_);
   }
 
+  virtual ~DBusMonitoringThread() {
+    Stop();
+  }
+
   // Override virtual method of Thread. Context: worker-thread.
   virtual void Run() {
     ASSERT(NULL == connection_);

talk/base/logging_unittest.cc

@@ -87,6 +87,12 @@ TEST(LogTest, MultipleStreams) {
 // Ensure we don't crash when adding/removing streams while threads are going.
 // We should restore the correct global state at the end.
 class LogThread : public Thread {
+ public:
+  virtual ~LogThread() {
+    Stop();
+  }
+
+ private:
   void Run() {
     // LS_SENSITIVE to avoid cluttering up any real logging going on
     LOG(LS_SENSITIVE) << "LOG";

talk/base/maccocoasocketserver_unittest.mm

@@ -36,6 +36,9 @@ class WakeThread : public Thread {
  public:
   WakeThread(SocketServer* ss) : ss_(ss) {
   }
+  virtual ~WakeThread() {
+    Stop();
+  }
   void Run() {
     ss_->WakeUp();
   }

talk/base/macsocketserver_unittest.cc

@@ -37,6 +37,9 @@ class WakeThread : public Thread {
  public:
   WakeThread(SocketServer* ss) : ss_(ss) {
   }
+  virtual ~WakeThread() {
+    Stop();
+  }
   void Run() {
     ss_->WakeUp();
   }

talk/base/signalthread.h

@@ -123,6 +123,7 @@ class SignalThread
   class Worker : public Thread {
    public:
     explicit Worker(SignalThread* parent) : parent_(parent) {}
+    virtual ~Worker() { Stop(); }
     virtual void Run() { parent_->Run(); }
 
    private:

talk/base/signalthread_unittest.cc

@@ -120,6 +120,10 @@ class OwnerThread : public Thread, public sigslot::has_slots<> {
         has_run_(false) {
   }
 
+  virtual ~OwnerThread() {
+    Stop();
+  }
+
   virtual void Run() {
     SignalThreadTest::SlowSignalThread* signal_thread =
         new SignalThreadTest::SlowSignalThread(harness_);

talk/base/thread.cc

@@ -557,6 +557,7 @@ AutoThread::AutoThread(SocketServer* ss) : Thread(ss) {
 }
 
 AutoThread::~AutoThread() {
+  Stop();
   if (ThreadManager::Instance()->CurrentThread() == this) {
     ThreadManager::Instance()->SetCurrentThread(NULL);
   }

talk/base/thread.h

@@ -111,9 +111,15 @@ class Runnable {
   DISALLOW_COPY_AND_ASSIGN(Runnable);
 };
 
+// WARNING! SUBCLASSES MUST CALL Stop() IN THEIR DESTRUCTORS!  See ~Thread().
+
 class Thread : public MessageQueue {
  public:
   explicit Thread(SocketServer* ss = NULL);
+  // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or
+  // guarantee Stop() is explicitly called before the subclass is destroyed).
+  // This is required to avoid a data race between the destructor modifying the
+  // vtable, and the Thread::PreRun calling the virtual method Run().
   virtual ~Thread();
 
   static Thread* Current();
@@ -291,6 +297,7 @@ class AutoThread : public Thread {
 class ComThread : public Thread {
  public:
   ComThread() {}
+  virtual ~ComThread() { Stop(); }
 
  protected:
   virtual void Run();

talk/base/thread_unittest.cc

@@ -122,7 +122,7 @@ class MessageClient : public MessageHandler, public TestGenerator {
 class CustomThread : public talk_base::Thread {
  public:
   CustomThread() {}
-  virtual ~CustomThread() {}
+  virtual ~CustomThread() { Stop(); }
   bool Start() { return false; }
 };
 
@@ -136,6 +136,7 @@ class SignalWhenDestroyedThread : public Thread {
   }
 
   virtual ~SignalWhenDestroyedThread() {
+    Stop();
     event_->Set();
   }
 

talk/base/win32socketserver.h

@@ -156,6 +156,7 @@ class Win32Thread : public Thread {
     set_socketserver(&ss_);
   }
   virtual ~Win32Thread() {
+    Stop();
     set_socketserver(NULL);
   }
   virtual void Run() {

talk/media/base/filemediaengine.cc

@@ -125,6 +125,7 @@ class RtpSenderReceiver
   RtpSenderReceiver(MediaChannel* channel,
                     talk_base::StreamInterface* input_file_stream,
                     talk_base::StreamInterface* output_file_stream);
+  virtual ~RtpSenderReceiver();
 
   // Called by media channel. Context: media channel thread.
   bool SetSend(bool send);
@@ -183,6 +184,10 @@ RtpSenderReceiver::RtpSenderReceiver(
   }
 }
 
+RtpSenderReceiver::~RtpSenderReceiver() {
+  Stop();
+}
+
 bool RtpSenderReceiver::SetSend(bool send) {
   bool was_sending = sending_;
   sending_ = send;

talk/media/devices/filevideocapturer.cc

@@ -108,6 +108,10 @@ class FileVideoCapturer::FileReadThread
         finished_(false) {
   }
 
+  virtual ~FileReadThread() {
+    Stop();
+  }
+
   // Override virtual method of parent Thread. Context: Worker Thread.
   virtual void Run() {
     // Read the first frame and start the message pump. The pump runs until

talk/media/devices/gdivideorenderer.cc

@@ -69,6 +69,10 @@ class GdiVideoRenderer::VideoWindow : public talk_base::Win32Window {
    public:
     explicit WindowThread(VideoWindow* window) : window_(window) {}
 
+    virtual ~WindowThread() {
+      Stop();
+    }
+
     // Override virtual method of talk_base::Thread. Context: worker Thread.
     virtual void Run() {
       // Initialize the window

talk/xmllite/xmlelement_unittest.cc

@@ -235,6 +235,10 @@ class XmlElementCreatorThread : public talk_base::Thread {
   XmlElementCreatorThread(int count, buzz::QName qname) :
       count_(count), qname_(qname) {}
 
+  virtual ~XmlElementCreatorThread() {
+    Stop();
+  }
+
   virtual void Run() {
     std::vector<buzz::XmlElement*> elems;
     for (int i = 0; i < count_; i++) {

talk/xmpp/xmppthread.cc

@@ -50,6 +50,7 @@ XmppThread::XmppThread() {
 }
 
 XmppThread::~XmppThread() {
+  Stop();
   delete pump_;
 }
 

tools/valgrind-webrtc/tsan/suppressions.txt

@@ -315,15 +315,6 @@
   fun:webrtc::Bitrate::BitrateLast
   ...
 }
-{
-  bug_2078_1
-  ThreadSanitizer:Race
-  fun:talk_base::Thread::~Thread
-  fun:cricket::RtpSenderReceiver::~RtpSenderReceiver
-  fun:cricket::RtpSenderReceiver::~RtpSenderReceiver
-  fun:talk_base::scoped_ptr::~scoped_ptr
-  ...
-}
 {
   bug_2078_2
   ThreadSanitizer:Race
@@ -656,13 +647,6 @@
   fun:talk_base::LogMessage::AddLogToStream
   ...
 }
-{
-  bug_2080_4
-  ThreadSanitizer:Race
-  fun:talk_base::Thread::~Thread
-  fun:talk_base::LogThread::~LogThread
-  ...
-}
 {
   bug_2080_6
   ThreadSanitizer:Race
@@ -848,16 +832,6 @@
   fun:talk_base::scoped_ptr::~scoped_ptr
   ...
 }
-{
-  bug_2080_28
-  ThreadSanitizer:Race
-  fun:talk_base::Thread::~Thread
-  fun:OwnerThread::~OwnerThread
-  fun:OwnerThread::~OwnerThread
-  fun:talk_base::scoped_ptr::~scoped_ptr
-  fun:SignalThreadTest_OwnerThreadGoesAway_Test::TestBody
-  fun:testing::internal::HandleSehExceptionsInMethodIfSupported
-}
 {
   bug_2080_29
   ThreadSanitizer:Race