Fix deadlock issue in WebRtcMediaStreamTrackAdapterMap.

The fix is to reduce the scope of the |lock_| during track adapter
creation because Create[Local/Remote]TrackAdapter() may synchronize with
another thread that is busy waiting for the |lock_|. There is no reason
to hold the |lock_| while creating the adapter; we are ensured no other
adapters of the same kind (=local/remote) are created concurrently
because they may only be created on the appropriate thread (local =
main thread, remote = signaling thread) which is the thread currently
running the operation. In other words, at most 1
GetOrCreateLocalTrackAdapter() on the main thread and at most 1
GetOrCreateRemoteTrackAdapter() on the signaling thread can run at the
same time.

This is the equivalent of the WebRtcMediaStreamAdapterMap fix
(https://chromium-review.googlesource.com/c/868656/) but for the track
adapter map.

The deadlock is quite rare, but due to recent unrelated changes it has
become frequent enough that it is easily reproducible on some platforms
and applications, see bug.

Bug: 813574
Change-Id: I2e7ba605db973221f80443267fd099c5973ab818
Reviewed-on: https://chromium-review.googlesource.com/980949
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546102}

content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc

@@ -108,9 +108,15 @@ WebRtcMediaStreamTrackAdapterMap::GetOrCreateLocalTrackAdapter(
     return base::WrapUnique(
         new AdapterRef(this, AdapterRef::Type::kLocal, *adapter_ptr));
   }
-  scoped_refptr<WebRtcMediaStreamTrackAdapter> new_adapter =
-      WebRtcMediaStreamTrackAdapter::CreateLocalTrackAdapter(
-          factory_, main_thread_, web_track);
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> new_adapter;
+  {
+    // Do not hold |lock_| while creating the adapter in case that operation
+    // synchronizes with the signaling thread. If we do and the signaling thread
+    // is blocked waiting for |lock_| we end up in a deadlock.
+    base::AutoUnlock scoped_unlock(lock_);
+    new_adapter = WebRtcMediaStreamTrackAdapter::CreateLocalTrackAdapter(
+        factory_, main_thread_, web_track);
+  }
   DCHECK(new_adapter->is_initialized());
   local_track_adapters_.Insert(web_track.UniqueId(), new_adapter);
   local_track_adapters_.SetSecondaryKey(web_track.UniqueId(),
@@ -161,9 +167,15 @@ WebRtcMediaStreamTrackAdapterMap::GetOrCreateRemoteTrackAdapter(
     return base::WrapUnique(
         new AdapterRef(this, AdapterRef::Type::kRemote, *adapter_ptr));
   }
-  scoped_refptr<WebRtcMediaStreamTrackAdapter> new_adapter =
-      WebRtcMediaStreamTrackAdapter::CreateRemoteTrackAdapter(
-          factory_, main_thread_, webrtc_track);
+  scoped_refptr<WebRtcMediaStreamTrackAdapter> new_adapter;
+  {
+    // Do not hold |lock_| while creating the adapter in case that operation
+    // synchronizes with the main thread. If we do and the main thread is
+    // blocked waiting for |lock_| we end up in a deadlock.
+    base::AutoUnlock scoped_unlock(lock_);
+    new_adapter = WebRtcMediaStreamTrackAdapter::CreateRemoteTrackAdapter(
+        factory_, main_thread_, webrtc_track);
+  }
   remote_track_adapters_.Insert(webrtc_track.get(), new_adapter);
   // The new adapter is initialized in a post to the main thread. As soon as it
   // is initialized we map its |webrtc_track| to the |remote_track_adapters_|

content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map_unittest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/memory/ref_counted.h"
+#include "base/rand_util.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/test/scoped_task_environment.h"
@@ -33,6 +34,10 @@ class WebRtcMediaStreamTrackAdapterMapTest : public ::testing::Test {
 
   void TearDown() override { blink::WebHeap::CollectAllGarbageForTesting(); }
 
+  scoped_refptr<base::SingleThreadTaskRunner> signaling_thread() const {
+    return dependency_factory_->GetWebRtcSignalingThread();
+  }
+
   blink::WebMediaStreamTrack CreateLocalTrack(const std::string& id) {
     blink::WebMediaStreamSource web_source;
     web_source.Initialize(
@@ -53,7 +58,7 @@ class WebRtcMediaStreamTrackAdapterMapTest : public ::testing::Test {
       webrtc::MediaStreamTrackInterface* webrtc_track) {
     DCHECK(main_thread_->BelongsToCurrentThread());
     std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef> adapter;
-    dependency_factory_->GetWebRtcSignalingThread()->PostTask(
+    signaling_thread()->PostTask(
         FROM_HERE,
         base::BindOnce(&WebRtcMediaStreamTrackAdapterMapTest::
                            GetOrCreateRemoteTrackAdapterOnSignalingThread,
@@ -66,8 +71,7 @@ class WebRtcMediaStreamTrackAdapterMapTest : public ::testing::Test {
   void GetOrCreateRemoteTrackAdapterOnSignalingThread(
       webrtc::MediaStreamTrackInterface* webrtc_track,
       std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef>* adapter) {
-    DCHECK(dependency_factory_->GetWebRtcSignalingThread()
-               ->BelongsToCurrentThread());
+    DCHECK(signaling_thread()->BelongsToCurrentThread());
     *adapter = map_->GetOrCreateRemoteTrackAdapter(webrtc_track);
   }
 
@@ -78,7 +82,7 @@ class WebRtcMediaStreamTrackAdapterMapTest : public ::testing::Test {
     base::WaitableEvent waitable_event(
         base::WaitableEvent::ResetPolicy::MANUAL,
         base::WaitableEvent::InitialState::NOT_SIGNALED);
-    dependency_factory_->GetWebRtcSignalingThread()->PostTask(
+    signaling_thread()->PostTask(
         FROM_HERE, base::BindOnce(&WebRtcMediaStreamTrackAdapterMapTest::
                                       RunMessageLoopUntilIdleOnSignalingThread,
                                   base::Unretained(this), &waitable_event));
@@ -88,8 +92,7 @@ class WebRtcMediaStreamTrackAdapterMapTest : public ::testing::Test {
 
   void RunMessageLoopUntilIdleOnSignalingThread(
       base::WaitableEvent* waitable_event) {
-    DCHECK(dependency_factory_->GetWebRtcSignalingThread()
-               ->BelongsToCurrentThread());
+    DCHECK(signaling_thread()->BelongsToCurrentThread());
     base::RunLoop().RunUntilIdle();
     waitable_event->Signal();
   }
@@ -213,4 +216,99 @@ TEST_F(WebRtcMediaStreamTrackAdapterMapTest, GetMissingRemoteTrackAdapter) {
   EXPECT_EQ(nullptr, map_->GetRemoteTrackAdapter(webrtc_track.get()));
 }
 
+// Continuously calls GetOrCreateLocalTrackAdapter() on the main thread and
+// GetOrCreateRemoteTrackAdapter() on the signaling thread hoping to hit
+// deadlocks if the operations were to synchronize with the other thread while
+// holding the lock.
+//
+// Note that this deadlock has been notoriously difficult to reproduce. This
+// test is added as an attempt to guard against this type of regression, but do
+// not trust that if this test passes there is no risk of deadlock.
+class WebRtcMediaStreamTrackAdapterMapStressTest
+    : public WebRtcMediaStreamTrackAdapterMapTest {
+ public:
+  WebRtcMediaStreamTrackAdapterMapStressTest()
+      : WebRtcMediaStreamTrackAdapterMapTest(), remaining_iterations_(0u) {}
+
+  void RunStressTest(size_t iterations) {
+    base::RunLoop run_loop;
+    remaining_iterations_ = iterations;
+    PostSignalingThreadLoop();
+    MainThreadLoop(&run_loop);
+    run_loop.Run();
+    // The run loop ensures all operations have began executing, but does not
+    // guarantee that all of them are complete, i.e. that track adapters have
+    // been fully initialized and subequently disposed. For that we need to run
+    // until idle or else we may tear down the test prematurely.
+    RunMessageLoopsUntilIdle();
+  }
+
+  void MainThreadLoop(base::RunLoop* run_loop) {
+    for (size_t i = 0u; i < 5u; ++i) {
+      map_->GetOrCreateLocalTrackAdapter(CreateLocalTrack("local_track_id"));
+    }
+    if (--remaining_iterations_ > 0) {
+      PostSignalingThreadLoop();
+      PostMainThreadLoop(run_loop);
+    } else {
+      // We are now done, but there may still be operations pending to execute
+      // on signaling thread so we perform Quit() in a post to the signaling
+      // thread. This ensures that Quit() is called after all operations have
+      // began executing (but does not guarantee that all operations have
+      // completed).
+      signaling_thread()->PostTask(
+          FROM_HERE,
+          base::BindOnce(&WebRtcMediaStreamTrackAdapterMapStressTest::
+                             QuitRunLoopOnSignalingThread,
+                         base::Unretained(this), base::Unretained(run_loop)));
+    }
+  }
+
+  void PostMainThreadLoop(base::RunLoop* run_loop) {
+    main_thread_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &WebRtcMediaStreamTrackAdapterMapStressTest::MainThreadLoop,
+            base::Unretained(this), base::Unretained(run_loop)));
+  }
+
+  void SignalingThreadLoop() {
+    std::vector<std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef>>
+        track_refs;
+    for (size_t i = 0u; i < 5u; ++i) {
+      track_refs.push_back(map_->GetOrCreateRemoteTrackAdapter(
+          MockWebRtcAudioTrack::Create("remote_track_id")));
+    }
+    main_thread_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&WebRtcMediaStreamTrackAdapterMapStressTest::
+                           DestroyAdapterRefsOnMainThread,
+                       base::Unretained(this), std::move(track_refs)));
+  }
+
+  void PostSignalingThreadLoop() {
+    signaling_thread()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &WebRtcMediaStreamTrackAdapterMapStressTest::SignalingThreadLoop,
+            base::Unretained(this)));
+  }
+
+  void DestroyAdapterRefsOnMainThread(
+      std::vector<std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef>>
+          track_refs) {}
+
+  void QuitRunLoopOnSignalingThread(base::RunLoop* run_loop) {
+    run_loop->Quit();
+  }
+
+ private:
+  size_t remaining_iterations_;
+};
+
+TEST_F(WebRtcMediaStreamTrackAdapterMapStressTest, StressTest) {
+  const size_t kNumStressTestIterations = 1000u;
+  RunStressTest(kNumStressTestIterations);
+}
+
 }  // namespace content