WebRtcMediaStreamTrackAdapter: Ensure destruction on main thread.

Adds destructor traits to ensure WebRtcMediaStreamTrackAdapter, which is
reference counted, is destroyed on the main thread. Due to references in
base::Bind-ings, possibly from third_party/webrtc or elsewhere,
destruction did not always occur on the main thread. The mix of
reference counting and usage on multiple threads is just inherently
risky. This change allows us to add a DCHECK in the destructor and fixes
one of the most common crashes currently in Canary.

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

content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc

@@ -72,6 +72,21 @@ WebRtcMediaStreamTrackAdapter::WebRtcMediaStreamTrackAdapter(
 WebRtcMediaStreamTrackAdapter::~WebRtcMediaStreamTrackAdapter() {
   DCHECK(!remote_track_can_complete_initialization_.IsSignaled());
   DCHECK(is_disposed_);
+  // Ensured by destructor traits.
+  DCHECK(main_thread_->BelongsToCurrentThread());
+}
+
+// static
+void WebRtcMediaStreamTrackAdapterTraits::Destruct(
+    const WebRtcMediaStreamTrackAdapter* adapter) {
+  if (!adapter->main_thread_->BelongsToCurrentThread()) {
+    adapter->main_thread_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&WebRtcMediaStreamTrackAdapterTraits::Destruct,
+                       base::Unretained(adapter)));
+    return;
+  }
+  delete adapter;
 }
 
 void WebRtcMediaStreamTrackAdapter::Dispose() {

content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h

@@ -21,6 +21,7 @@
 namespace content {
 
 class PeerConnectionDependencyFactory;
+struct WebRtcMediaStreamTrackAdapterTraits;
 
 // This is a mapping between a webrtc and blink media stream track. It takes
 // care of creation, initialization and disposing of tracks independently of
@@ -29,7 +30,8 @@ class PeerConnectionDependencyFactory;
 // and whether it is an audio or video track; this adapter hides that fact and
 // lets you use a single class for any type of track.
 class CONTENT_EXPORT WebRtcMediaStreamTrackAdapter
-    : public base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter> {
+    : public base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter,
+                                        WebRtcMediaStreamTrackAdapterTraits> {
  public:
   // Invoke on the main thread. The returned adapter is fully initialized, see
   // |is_initialized|. The adapter will keep a reference to the |main_thread|.
@@ -76,7 +78,9 @@ class CONTENT_EXPORT WebRtcMediaStreamTrackAdapter
   }
 
  protected:
-  friend class base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter>;
+  friend class base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter,
+                                          WebRtcMediaStreamTrackAdapterTraits>;
+  friend struct WebRtcMediaStreamTrackAdapterTraits;
 
   WebRtcMediaStreamTrackAdapter(
       PeerConnectionDependencyFactory* factory,
@@ -132,6 +136,16 @@ class CONTENT_EXPORT WebRtcMediaStreamTrackAdapter
   DISALLOW_COPY_AND_ASSIGN(WebRtcMediaStreamTrackAdapter);
 };
 
+struct CONTENT_EXPORT WebRtcMediaStreamTrackAdapterTraits {
+ private:
+  friend class base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter,
+                                          WebRtcMediaStreamTrackAdapterTraits>;
+
+  // Ensure destruction occurs on main thread so that "Web" and other resources
+  // are destroyed on the correct thread.
+  static void Destruct(const WebRtcMediaStreamTrackAdapter* adapter);
+};
+
 }  // namespace content
 
 #endif  // CONTENT_RENDERER_MEDIA_WEBRTC_WEBRTC_MEDIA_STREAM_TRACK_ADAPTER_H_

content/renderer/media/webrtc/webrtc_media_stream_track_adapter_unittest.cc

@@ -79,6 +79,12 @@ class WebRtcMediaStreamTrackAdapterTest : public ::testing::Test {
         dependency_factory_.get(), main_thread_, webrtc_track);
   }
 
+  void HoldOntoAdapterReference(
+      base::WaitableEvent* waitable_event,
+      scoped_refptr<WebRtcMediaStreamTrackAdapter> adapter) {
+    waitable_event->Wait();
+  }
+
   // Runs message loops on the webrtc signaling thread and optionally the main
   // thread until idle.
   void RunMessageLoopsUntilIdle(bool run_loop_on_main_thread = true) {
@@ -228,4 +234,33 @@ TEST_F(WebRtcMediaStreamTrackAdapterTest, RemoteTrackExplicitlyInitialized) {
       track_adapter_->GetRemoteAudioTrackAdapterForTesting()->initialized());
 }
 
+TEST_F(WebRtcMediaStreamTrackAdapterTest, LastReferenceOnSignalingThread) {
+  scoped_refptr<MockWebRtcAudioTrack> webrtc_track =
+      MockWebRtcAudioTrack::Create("remote_audio_track");
+  dependency_factory_->GetWebRtcSignalingThread()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &WebRtcMediaStreamTrackAdapterTest::CreateRemoteTrackAdapter,
+          base::Unretained(this), base::Unretained(webrtc_track.get())));
+  // The adapter is initialized implicitly in a PostTask, allow it to run.
+  RunMessageLoopsUntilIdle();
+  DCHECK(track_adapter_);
+  EXPECT_TRUE(track_adapter_->is_initialized());
+
+  base::WaitableEvent waitable_event(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  dependency_factory_->GetWebRtcSignalingThread()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &WebRtcMediaStreamTrackAdapterTest::HoldOntoAdapterReference,
+          base::Unretained(this), base::Unretained(&waitable_event),
+          track_adapter_));
+  // Clear last reference on main thread.
+  track_adapter_->Dispose();
+  track_adapter_ = nullptr;
+  waitable_event.Signal();
+  RunMessageLoopsUntilIdle();
+}
+
 }  // namespace content