Fix recursive locking in SignalThread.

By requiring Release to be called with lock held. This is a bit of a
kludge, but I think this works, because all known users of this
deprecated class call Release either from OnWorkStop or
SignalWorkDone, and both are called with the lock already held.

Bug: webrtc:11567
Change-Id: Idf0007188e45a465aefcb8f13fea93a68930fe1c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/204483
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Markus Handell <handellm@webrtc.org>
Commit-Queue: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33188}

rtc_base/deprecated/signal_thread.cc

diff --git a/rtc_base/deprecated/signal_thread.cc b/rtc_base/deprecated/signal_thread.cc
index 96bdd65..318c8ca 100644
--- a/rtc_base/deprecated/signal_thread.cc
+++ b/rtc_base/deprecated/signal_thread.cc
@@ -31,7 +31,7 @@
 }
 
 DEPRECATED_SignalThread::~DEPRECATED_SignalThread() {
-  rtc::CritScope lock(&cs_);
+  webrtc::MutexLock lock(&mutex_);
   RTC_DCHECK(refcount_ == 0);
 }
 
@@ -74,9 +74,9 @@
     OnWorkStop();
     if (wait) {
       // Release the thread's lock so that it can return from ::Run.
-      cs_.Leave();
+      mutex_.Unlock();
       worker_.Stop();
-      cs_.Enter();
+      mutex_.Lock();
       refcount_--;
     }
   } else {
@@ -84,8 +84,11 @@
   }
 }
 
-void DEPRECATED_SignalThread::Release() {
-  EnterExit ee(this);
+// Disable analysis, to allow calls via SignalWorkDone (which already holds the
+// lock).
+// TODO(bugs.webrtc.org/11567): Add a Mutex::AssertHeld, to reenable analysis
+// and get a runtime check.
+void DEPRECATED_SignalThread::Release() RTC_NO_THREAD_SAFETY_ANALYSIS {
   RTC_DCHECK(!destroy_called_);
   RTC_DCHECK(main_->IsCurrent());
   if (kComplete == state_) {

rtc_base/deprecated/signal_thread.h

diff --git a/rtc_base/deprecated/signal_thread.h b/rtc_base/deprecated/signal_thread.h
index 10805ad..fb3a902 100644
--- a/rtc_base/deprecated/signal_thread.h
+++ b/rtc_base/deprecated/signal_thread.h
@@ -15,9 +15,9 @@
 
 #include "rtc_base/checks.h"
 #include "rtc_base/constructor_magic.h"
-#include "rtc_base/deprecated/recursive_critical_section.h"
 #include "rtc_base/deprecation.h"
 #include "rtc_base/message_handler.h"
+#include "rtc_base/synchronization/mutex.h"
 #include "rtc_base/third_party/sigslot/sigslot.h"
 #include "rtc_base/thread.h"
 #include "rtc_base/thread_annotations.h"
@@ -65,6 +65,12 @@
   // Context: Main Thread.  If the worker thread is complete, deletes the
   // object immediately.  Otherwise, schedules the object to be deleted once
   // the worker thread completes.  SignalWorkDone will be signalled.
+
+  // BEWARE: This method must be called with the object's internal lock held, as
+  // if annotated RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_). Callbacks via OnWorkStop
+  // and SignalWorkDone are already holding the needed lock. It's not annotated,
+  // because it's hard to tell the compiler that functions called via
+  // SignalWorkDone already hold the lock.
   void Release();
 
   // Context: Main Thread.  Signalled when work is complete.
@@ -126,9 +132,9 @@
   class RTC_SCOPED_LOCKABLE EnterExit {
    public:
     explicit EnterExit(DEPRECATED_SignalThread* t)
-        RTC_EXCLUSIVE_LOCK_FUNCTION(t->cs_)
+        RTC_EXCLUSIVE_LOCK_FUNCTION(t->mutex_)
         : t_(t) {
-      t_->cs_.Enter();
+      t_->mutex_.Lock();
       // If refcount_ is zero then the object has already been deleted and we
       // will be double-deleting it in ~EnterExit()! (shouldn't happen)
       RTC_DCHECK_NE(0, t_->refcount_);
@@ -141,7 +147,7 @@
 
     ~EnterExit() RTC_UNLOCK_FUNCTION() {
       bool d = (0 == --t_->refcount_);
-      t_->cs_.Leave();
+      t_->mutex_.Unlock();
       if (d)
         delete t_;
     }
@@ -155,10 +161,10 @@
 
   Thread* main_;
   Worker worker_;
-  RecursiveCriticalSection cs_;
-  State state_ RTC_GUARDED_BY(cs_);
-  int refcount_ RTC_GUARDED_BY(cs_);
-  bool destroy_called_ RTC_GUARDED_BY(cs_) = false;
+  webrtc::Mutex mutex_;
+  State state_ RTC_GUARDED_BY(mutex_);
+  int refcount_ RTC_GUARDED_BY(mutex_);
+  bool destroy_called_ RTC_GUARDED_BY(mutex_) = false;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(DEPRECATED_SignalThread);
 };