Fix crash where a remote stream is removed before the promise resolves.

setRemoteDescription() ("SRD") is executed on the main thread, does work
on the webrtc signaling thread which has callbacks that jump back to the
main thread to fire events and resolve the promise.

The callback on the main thread needs to know about the stream, tracks
and their receivers. Previously, we invoked native_peer_connection_
->GetReceivers() in the main thread callback. This was problematic in
this case:

SRD that adds stream x
SRD that removes stream x

By the time we do GetReceivers() in the main thread callback the stream
may already have been removed from the native_peer_connection_ by the
second SRD call. This lead to a crash because it couldn't find the
receivers of the added stream.

The fix is to do the GetReceivers() call while still on the webrtc
signaling thread and not do any querying of the
native_peer_connection_'s state from the main thread callback.

Calling SRD in succession without waiting for the promises to resolve
is inherently racey but this ensures the events fire without crashing
and that we end up in the correct state. The indeterminate (racey) part
is whether or not the stream fired in the first ontrack/onaddstream
events contain the first track or is empty.

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

chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc

@@ -381,3 +381,11 @@ IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest, SwitchRemoteStreamAndBackAgain) {
   EXPECT_EQ("ok",
             ExecuteJavascript("switchRemoteStreamAndBackAgain()", left_tab_));
 }
+
+IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
+                       SwitchRemoteStreamWithoutWaitingForPromisesToResolve) {
+  StartServerAndOpenTabs();
+  EXPECT_EQ("ok", ExecuteJavascript(
+                      "switchRemoteStreamWithoutWaitingForPromisesToResolve()",
+                      left_tab_));
+}

chrome/test/data/webrtc/peerconnection_rtp.js

@@ -275,6 +275,70 @@ function switchRemoteStreamAndBackAgain() {
     });
 }
 
+function switchRemoteStreamWithoutWaitingForPromisesToResolve() {
+  let pc = new RTCPeerConnection();
+  let trackEventsFired = 0;
+  let streamEventsFired = 0;
+  pc.ontrack = (e) => {
+    ++trackEventsFired;
+    if (trackEventsFired == 1) {
+      if (e.track.id != 'track0')
+        throw failTest('Unexpected track id in first track event.');
+      if (e.receiver.track != e.track)
+        throw failTest('Unexpected receiver.track in first track event.');
+      if (e.streams[0].id != 'stream0')
+        throw failTest('Unexpected stream id in first track event.');
+      // Because we did not wait for promises to resolve before calling
+      // |setRemoteDescription| a second time, it may or may not have had an
+      // effect here. This is inherently racey and we have to check if the
+      // stream contains the track.
+      if (e.streams[0].getTracks().length != 0) {
+        if (e.streams[0].getTracks()[0] != e.track)
+          throw failTest('Unexpected track in stream in first track event.');
+      }
+    } else if (trackEventsFired == 2) {
+      if (e.track.id != 'track1')
+        throw failTest('Unexpected track id in second track event.');
+      if (e.receiver.track != e.track)
+        throw failTest('Unexpected receiver.track in second track event.');
+      if (e.streams[0].id != 'stream1')
+        throw failTest('Unexpected stream id in second track event.');
+      if (e.streams[0].getTracks()[0] != e.track)
+        throw failTest('The track should belong to the stream in the second ' +
+                       'track event.');
+      if (streamEventsFired != trackEventsFired)
+        throw failTest('All stream events should already have fired.');
+      returnToTest('ok');
+    }
+  };
+  pc.onaddstream = (e) => {
+    ++streamEventsFired;
+    if (streamEventsFired == 1) {
+      if (e.stream.id != 'stream0')
+        throw failTest('Unexpected stream id in first stream event.');
+      // Because we did not wait for promises to resolve before calling
+      // |setRemoteDescription| a second time, it may or may not have had an
+      // effect here. This is inherently racey and we have to check if the
+      // stream contains the track.
+      if (e.stream.getTracks().length != 0) {
+        if (e.stream.getTracks()[0].id != 'track0')
+          throw failTest('Unexpected track id in first stream event.');
+      }
+    } else if (streamEventsFired == 2) {
+      if (e.stream.id != 'stream1')
+        throw failTest('Unexpected stream id in second stream event.');
+      if (e.stream.getTracks()[0].id != 'track1')
+        throw failTest('Unexpected track id in second stream event.');
+    }
+  };
+  pc.setRemoteDescription(createOffer('stream0', 'track0'));
+  pc.setRemoteDescription(createOffer('stream1', 'track1'));
+}
+
+// TODO(hbos): Also try switching back again for the case where IDs do match
+// but the objects are in fact different. Verify that they are different objects
+// like in the |switchRemoteStreamAndBackAgain| test.
+
 /**
  * Invokes the GC and returns "ok-gc".
  */

content/renderer/media/rtc_peer_connection_handler.cc

@@ -925,23 +925,29 @@ std::set<RTCPeerConnectionHandler*>* GetPeerConnectionHandlers() {
   return handlers;
 }
 
-bool IsReceiverForStream(
-    const rtc::scoped_refptr<webrtc::RtpReceiverInterface>& webrtc_receiver,
-    const scoped_refptr<webrtc::MediaStreamInterface>& webrtc_stream) {
-  rtc::scoped_refptr<webrtc::MediaStreamTrackInterface> webrtc_track =
-      webrtc_receiver->track();
-  if (webrtc_track->kind() == webrtc::MediaStreamTrackInterface::kAudioKind) {
-    for (const auto& track : webrtc_stream->GetAudioTracks()) {
-      if (webrtc_track == track)
-        return true;
-    }
-  } else {
-    for (const auto& track : webrtc_stream->GetVideoTracks()) {
-      if (webrtc_track == track)
-        return true;
-    }
-  }
-  return false;
+rtc::scoped_refptr<webrtc::RtpReceiverInterface> FindReceiverForTrack(
+    const std::string& track_id,
+    const std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>&
+        webrtc_receivers) {
+  for (const auto& webrtc_receiver : webrtc_receivers) {
+    if (webrtc_receiver->track()->id() == track_id)
+      return webrtc_receiver;
+  }
+  return nullptr;
+}
+
+std::unique_ptr<RTCRtpReceiver> CreateRTCRtpReceiverForTrack(
+    webrtc::MediaStreamTrackInterface* webrtc_track,
+    const std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>&
+        webrtc_receivers,
+    scoped_refptr<WebRtcMediaStreamTrackAdapterMap> track_adapter_map) {
+  auto webrtc_receiver =
+      FindReceiverForTrack(webrtc_track->id(), webrtc_receivers);
+  DCHECK(webrtc_receiver);
+  auto track_adapter = track_adapter_map->GetRemoteTrackAdapter(webrtc_track);
+  DCHECK(track_adapter);
+  return base::MakeUnique<RTCRtpReceiver>(std::move(webrtc_receiver),
+                                          std::move(track_adapter));
 }
 
 }  // namespace
@@ -995,10 +1001,15 @@ class RTCPeerConnectionHandler::Observer
   Observer(const base::WeakPtr<RTCPeerConnectionHandler>& handler)
       : handler_(handler),
         main_thread_(base::ThreadTaskRunnerHandle::Get()),
-        track_adapter_map_(handler_->track_adapter_map_) {
+        track_adapter_map_(handler_->track_adapter_map_),
+        native_peer_connection_(nullptr) {
     DCHECK(track_adapter_map_);
   }
 
+  void Initialize(webrtc::PeerConnectionInterface* native_peer_connection) {
+    native_peer_connection_ = native_peer_connection;
+  }
+
  protected:
   friend class base::RefCountedThreadSafe<RTCPeerConnectionHandler::Observer>;
   virtual ~Observer() {}
@@ -1015,18 +1026,41 @@ class RTCPeerConnectionHandler::Observer
     }
   }
 
-  void OnAddStream(rtc::scoped_refptr<MediaStreamInterface> stream) override {
-    DCHECK(stream);
+  void OnAddStream(
+      rtc::scoped_refptr<MediaStreamInterface> webrtc_stream) override {
+    DCHECK(webrtc_stream);
+    DCHECK(native_peer_connection_);
+    DCHECK(!main_thread_->BelongsToCurrentThread());
     std::unique_ptr<RemoteMediaStreamImpl> remote_stream(
-        new RemoteMediaStreamImpl(main_thread_, track_adapter_map_, stream));
+        new RemoteMediaStreamImpl(main_thread_, track_adapter_map_,
+                                  webrtc_stream));
+
+    // Because |SetRemoteDescription| is asynchronous it is unsafe to query the
+    // |native_peer_connection_|'s state after the callback has jumped back to
+    // the main thread - something could happen in-between this callback on the
+    // signaling thread and |OnAddStreamImpl|. Every state that the callback on
+    // the main thread needs to know about has to be passed as arguments.
+    // Get all receivers that belong to the stream's track.
+    std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>
+        webrtc_receivers = native_peer_connection_->GetReceivers();
+    std::vector<std::unique_ptr<blink::WebRTCRtpReceiver>> stream_web_receivers;
+    for (const auto& webrtc_audio_track : webrtc_stream->GetAudioTracks()) {
+      stream_web_receivers.push_back(CreateRTCRtpReceiverForTrack(
+          webrtc_audio_track.get(), webrtc_receivers, track_adapter_map_));
+    }
+    for (const auto& webrtc_video_track : webrtc_stream->GetVideoTracks()) {
+      stream_web_receivers.push_back(CreateRTCRtpReceiverForTrack(
+          webrtc_video_track.get(), webrtc_receivers, track_adapter_map_));
+    }
 
-    // The webkit object owned by RemoteMediaStreamImpl, will be initialized
-    // asynchronously and the posted task will execude after that initialization
+    // The webkit object owned by |RemoteMediaStreamImpl|, will be initialized
+    // asynchronously and the posted task will execute after that initialization
     // is done.
     main_thread_->PostTask(
         FROM_HERE,
         base::BindOnce(&RTCPeerConnectionHandler::Observer::OnAddStreamImpl,
-                       this, base::Passed(&remote_stream)));
+                       this, base::Passed(&remote_stream),
+                       base::Passed(&stream_web_receivers)));
   }
 
   void OnRemoveStream(
@@ -1101,9 +1135,13 @@ class RTCPeerConnectionHandler::Observer
                        candidate->candidate().address().family()));
   }
 
-  void OnAddStreamImpl(std::unique_ptr<RemoteMediaStreamImpl> stream) {
-    if (handler_)
-      handler_->OnAddStream(std::move(stream));
+  void OnAddStreamImpl(std::unique_ptr<RemoteMediaStreamImpl> stream,
+                       std::vector<std::unique_ptr<blink::WebRTCRtpReceiver>>
+                           stream_webrtc_receivers) {
+    if (handler_) {
+      handler_->OnAddStream(std::move(stream),
+                            std::move(stream_webrtc_receivers));
+    }
   }
 
   void OnRemoveStreamImpl(const scoped_refptr<MediaStreamInterface>& stream) {
@@ -1128,6 +1166,9 @@ class RTCPeerConnectionHandler::Observer
   const base::WeakPtr<RTCPeerConnectionHandler> handler_;
   const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
   const scoped_refptr<WebRtcMediaStreamTrackAdapterMap> track_adapter_map_;
+  // Raw pointer. Only guaranteed to be valid within within callbacks on the
+  // signaling thread since these are triggered by |native_peer_connection_|.
+  webrtc::PeerConnectionInterface* native_peer_connection_;
 };
 
 RTCPeerConnectionHandler::RTCPeerConnectionHandler(
@@ -1197,6 +1238,7 @@ bool RTCPeerConnectionHandler::Initialize(
   peer_connection_observer_ = new Observer(weak_factory_.GetWeakPtr());
   native_peer_connection_ = dependency_factory_->CreatePeerConnection(
       configuration_, frame_, peer_connection_observer_.get());
+  peer_connection_observer_->Initialize(native_peer_connection_.get());
 
   if (!native_peer_connection_.get()) {
     LOG(ERROR) << "Failed to initialize native PeerConnection.";
@@ -1225,6 +1267,7 @@ bool RTCPeerConnectionHandler::InitializeForTest(
 
   native_peer_connection_ = dependency_factory_->CreatePeerConnection(
       configuration_, nullptr, peer_connection_observer_.get());
+  peer_connection_observer_->Initialize(native_peer_connection_.get());
   if (!native_peer_connection_.get()) {
     LOG(ERROR) << "Failed to initialize native PeerConnection.";
     return false;
@@ -1991,7 +2034,9 @@ void RTCPeerConnectionHandler::OnRenegotiationNeeded() {
 }
 
 void RTCPeerConnectionHandler::OnAddStream(
-    std::unique_ptr<RemoteMediaStreamImpl> stream) {
+    std::unique_ptr<RemoteMediaStreamImpl> stream,
+    std::vector<std::unique_ptr<blink::WebRTCRtpReceiver>>
+        stream_webrtc_receivers) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(remote_streams_.find(stream->webrtc_stream().get()) ==
          remote_streams_.end());
@@ -2011,27 +2056,14 @@ void RTCPeerConnectionHandler::OnAddStream(
   track_metrics_.AddStream(MediaStreamTrackMetrics::RECEIVED_STREAM,
                            stream_ptr->webrtc_stream().get());
   if (!is_closed_) {
-    // Get receivers for the tracks in this stream. We need to filter out the
-    // webrtc layer receivers of interest before creating content layer
-    // receivers so that we don't end up with content layer receivers for
-    // receivers of remote streams that have not been processed yet (this could
-    // yield track adapters that have not completed initialization).
-    // Note: This performs a synchronous call to the webrtc signaling thread.
-    // Once we switch over to |OnAddTrack| we'll get rid of these thread hops
-    // since it will contain the receiver. https://crbug.com/741619
-    std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>
-        webrtc_receivers = native_peer_connection_->GetReceivers();
-    std::vector<std::unique_ptr<blink::WebRTCRtpReceiver>> stream_web_receivers;
-    for (const auto& webrtc_receiver : webrtc_receivers) {
-      if (IsReceiverForStream(webrtc_receiver, stream_ptr->webrtc_stream()))
-        stream_web_receivers.push_back(GetWebRTCRtpReceiver(webrtc_receiver));
-    }
-    blink::WebVector<std::unique_ptr<blink::WebRTCRtpReceiver>> result(
-        stream_web_receivers.size());
-    for (size_t i = 0; i < stream_web_receivers.size(); ++i) {
-      result[i] = std::move(stream_web_receivers[i]);
+    blink::WebVector<std::unique_ptr<blink::WebRTCRtpReceiver>>
+        web_vector_stream_webrtc_receivers(stream_webrtc_receivers.size());
+    for (size_t i = 0; i < stream_webrtc_receivers.size(); ++i) {
+      web_vector_stream_webrtc_receivers[i] =
+          std::move(stream_webrtc_receivers[i]);
     }
-    client_->DidAddRemoteStream(stream_ptr->webkit_stream(), &result);
+    client_->DidAddRemoteStream(stream_ptr->webkit_stream(),
+                                &web_vector_stream_webrtc_receivers);
   }
 }
 

content/renderer/media/rtc_peer_connection_handler.h

@@ -199,7 +199,9 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
   void OnIceGatheringChange(
       webrtc::PeerConnectionInterface::IceGatheringState new_state);
   void OnRenegotiationNeeded();
-  void OnAddStream(std::unique_ptr<RemoteMediaStreamImpl> stream);
+  void OnAddStream(
+      std::unique_ptr<RemoteMediaStreamImpl> stream,
+      std::vector<std::unique_ptr<blink::WebRTCRtpReceiver>> web_receivers);
   void OnRemoveStream(
       const scoped_refptr<webrtc::MediaStreamInterface>& stream);
   void OnDataChannel(std::unique_ptr<RtcDataChannelHandler> handler);

content/renderer/media/webrtc/rtc_rtp_receiver.cc

@@ -23,7 +23,6 @@ RTCRtpReceiver::RTCRtpReceiver(
       track_adapter_(std::move(track_adapter)) {
   DCHECK(webrtc_rtp_receiver_);
   DCHECK(track_adapter_);
-  DCHECK_EQ(track_adapter_->webrtc_track(), webrtc_rtp_receiver_->track());
 }
 
 RTCRtpReceiver::~RTCRtpReceiver() {}

content/renderer/media/webrtc/webrtc_media_stream_track_adapter.cc

@@ -37,20 +37,20 @@ scoped_refptr<WebRtcMediaStreamTrackAdapter>
 WebRtcMediaStreamTrackAdapter::CreateRemoteTrackAdapter(
     PeerConnectionDependencyFactory* factory,
     const scoped_refptr<base::SingleThreadTaskRunner>& main_thread,
-    webrtc::MediaStreamTrackInterface* webrtc_track) {
+    const scoped_refptr<webrtc::MediaStreamTrackInterface>& webrtc_track) {
   DCHECK(factory);
   DCHECK(!main_thread->BelongsToCurrentThread());
   DCHECK(webrtc_track);
   scoped_refptr<WebRtcMediaStreamTrackAdapter> remote_track_adapter(
       new WebRtcMediaStreamTrackAdapter(factory, main_thread));
   if (webrtc_track->kind() == webrtc::MediaStreamTrackInterface::kAudioKind) {
-    remote_track_adapter->InitializeRemoteAudioTrack(
-        static_cast<webrtc::AudioTrackInterface*>(webrtc_track));
+    remote_track_adapter->InitializeRemoteAudioTrack(make_scoped_refptr(
+        static_cast<webrtc::AudioTrackInterface*>(webrtc_track.get())));
   } else {
     DCHECK_EQ(webrtc_track->kind(),
               webrtc::MediaStreamTrackInterface::kVideoKind);
-    remote_track_adapter->InitializeRemoteVideoTrack(
-        static_cast<webrtc::VideoTrackInterface*>(webrtc_track));
+    remote_track_adapter->InitializeRemoteVideoTrack(make_scoped_refptr(
+        static_cast<webrtc::VideoTrackInterface*>(webrtc_track.get())));
   }
   return remote_track_adapter;
 }
@@ -174,15 +174,15 @@ void WebRtcMediaStreamTrackAdapter::InitializeLocalVideoTrack(
 }
 
 void WebRtcMediaStreamTrackAdapter::InitializeRemoteAudioTrack(
-    webrtc::AudioTrackInterface* webrtc_audio_track) {
+    const scoped_refptr<webrtc::AudioTrackInterface>& webrtc_audio_track) {
   DCHECK(!main_thread_->BelongsToCurrentThread());
   DCHECK(!is_initialized_);
   DCHECK(!remote_track_can_complete_initialization_.IsSignaled());
   DCHECK(webrtc_audio_track);
   DCHECK_EQ(webrtc_audio_track->kind(),
             webrtc::MediaStreamTrackInterface::kAudioKind);
   remote_audio_track_adapter_ =
-      new RemoteAudioTrackAdapter(main_thread_, webrtc_audio_track);
+      new RemoteAudioTrackAdapter(main_thread_, webrtc_audio_track.get());
   webrtc_track_ = webrtc_audio_track;
   remote_track_can_complete_initialization_.Signal();
   main_thread_->PostTask(
@@ -193,13 +193,13 @@ void WebRtcMediaStreamTrackAdapter::InitializeRemoteAudioTrack(
 }
 
 void WebRtcMediaStreamTrackAdapter::InitializeRemoteVideoTrack(
-    webrtc::VideoTrackInterface* webrtc_video_track) {
+    const scoped_refptr<webrtc::VideoTrackInterface>& webrtc_video_track) {
   DCHECK(!main_thread_->BelongsToCurrentThread());
   DCHECK(webrtc_video_track);
   DCHECK_EQ(webrtc_video_track->kind(),
             webrtc::MediaStreamTrackInterface::kVideoKind);
   remote_video_track_adapter_ =
-      new RemoteVideoTrackAdapter(main_thread_, webrtc_video_track);
+      new RemoteVideoTrackAdapter(main_thread_, webrtc_video_track.get());
   webrtc_track_ = webrtc_video_track;
   remote_track_can_complete_initialization_.Signal();
   main_thread_->PostTask(

content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h

@@ -32,18 +32,19 @@ class CONTENT_EXPORT WebRtcMediaStreamTrackAdapter
     : public base::RefCountedThreadSafe<WebRtcMediaStreamTrackAdapter> {
  public:
   // Invoke on the main thread. The returned adapter is fully initialized, see
-  // |is_initialized|.
+  // |is_initialized|. The adapter will keep a reference to the |main_thread|.
   static scoped_refptr<WebRtcMediaStreamTrackAdapter> CreateLocalTrackAdapter(
       PeerConnectionDependencyFactory* factory,
       const scoped_refptr<base::SingleThreadTaskRunner>& main_thread,
       const blink::WebMediaStreamTrack& web_track);
   // Invoke on the webrtc signaling thread. Initialization finishes on the main
   // thread in a post, meaning returned adapters are ensured to be initialized
-  // in posts to the main thread, see |is_initialized|.
+  // in posts to the main thread, see |is_initialized|. The adapter will keep
+  // references to the |main_thread| and |webrtc_track|.
   static scoped_refptr<WebRtcMediaStreamTrackAdapter> CreateRemoteTrackAdapter(
       PeerConnectionDependencyFactory* factory,
       const scoped_refptr<base::SingleThreadTaskRunner>& main_thread,
-      webrtc::MediaStreamTrackInterface* webrtc_track);
+      const scoped_refptr<webrtc::MediaStreamTrackInterface>& webrtc_track);
   // Must be called before all external references are released (i.e. before
   // destruction). Invoke on the main thread. Disposing may finish
   // asynchronously using the webrtc signaling thread and the main thread. After
@@ -88,9 +89,9 @@ class CONTENT_EXPORT WebRtcMediaStreamTrackAdapter
   // Initialization of remote tracks starts on the webrtc signaling thread and
   // finishes on the main thread.
   void InitializeRemoteAudioTrack(
-      webrtc::AudioTrackInterface* webrtc_audio_track);
+      const scoped_refptr<webrtc::AudioTrackInterface>& webrtc_audio_track);
   void InitializeRemoteVideoTrack(
-      webrtc::VideoTrackInterface* webrtc_video_track);
+      const scoped_refptr<webrtc::VideoTrackInterface>& webrtc_video_track);
   void FinalizeRemoteTrackInitializationOnMainThread();
   void EnsureTrackIsInitialized();