RTCPeerConnectionHandler: Keep track of senders.

https://chromium-review.googlesource.com/c/566806 showed that the
WebRtcRtpBrowserTest.AddAndRemoveTracks* tests would reliably crash
if the garbage collector was invoked between addTrack and getSenders.
Tracks that were added using addTrack would have their track adapters
(glue between blink and webrtc) kept alive only by the blink layer
sender holding on to a reference, since these were not kept alive by
any "local streams" holding a reference.

The blink::RTCPeerConnection now holds a strong reference to senders
and receivers to prevent GC while in-use, with TODOs to remove ones
no longer used when addStream/removeStream is implemented using
addTrack/removeTrack.

Furthermore the content::RTCPeerConnectionHandler holds on to the
content layer representation of senders so that their associated set
of streams are not forgotten between addTrack and getSenders.

Having the handler keep track of senders is good practice, this gets
rid of the assumption that blink layer senders have to be kept alive.
A TODO was added to do the same for receivers (not yet required because
blink layer receivers are kept alive).

With this change, the AddAndRemoveTracks* tests pass and are
re-enabled.

Bug: 740650
Change-Id: Iec3a39a994bef31904f2969d791125867ec1e398
Reviewed-on: https://chromium-review.googlesource.com/567184Reviewed-by: Henrik Boström <hbos@chromium.org>
Reviewed-by: Taylor Brandstetter <deadbeef@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487430}

chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc

@@ -69,8 +69,7 @@ IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest, GetReceivers) {
   VerifyRtpReceivers(right_tab_, 6);
 }
 
-IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
-                       DISABLED_AddAndRemoveTracksWithoutStream) {
+IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest, AddAndRemoveTracksWithoutStream) {
   StartServerAndOpenTabs();
 
   SetupPeerconnectionWithoutLocalStream(left_tab_);
@@ -151,7 +150,7 @@ IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
-                       DISABLED_AddAndRemoveTracksWithSharedStream) {
+                       AddAndRemoveTracksWithSharedStream) {
   StartServerAndOpenTabs();
 
   SetupPeerconnectionWithoutLocalStream(left_tab_);
@@ -232,7 +231,7 @@ IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
 }
 
 IN_PROC_BROWSER_TEST_F(WebRtcRtpBrowserTest,
-                       DISABLED_AddAndRemoveTracksWithIndividualStreams) {
+                       AddAndRemoveTracksWithIndividualStreams) {
   StartServerAndOpenTabs();
 
   SetupPeerconnectionWithoutLocalStream(left_tab_);

content/renderer/media/rtc_peer_connection_handler.cc

@@ -30,7 +30,6 @@
 #include "content/renderer/media/rtc_dtmf_sender_handler.h"
 #include "content/renderer/media/webrtc/peer_connection_dependency_factory.h"
 #include "content/renderer/media/webrtc/rtc_rtp_receiver.h"
-#include "content/renderer/media/webrtc/rtc_rtp_sender.h"
 #include "content/renderer/media/webrtc/rtc_stats.h"
 #include "content/renderer/media/webrtc/webrtc_media_stream_adapter.h"
 #include "content/renderer/media/webrtc_audio_device_impl.h"
@@ -1586,6 +1585,10 @@ void RTCPeerConnectionHandler::RemoveStream(
   PerSessionWebRTCAPIMetrics::GetInstance()->DecrementStreamCounter();
   track_metrics_.RemoveStream(MediaStreamTrackMetrics::SENT_STREAM,
                               webrtc_stream.get());
+  // Make sure |rtp_senders_| is up-to-date. When |AddStream| and |RemoveStream|
+  // is implemented using |AddTrack| and |RemoveTrack|, add and remove
+  // |rtp_senders_| there. https://crbug.com/738929
+  GetSenders();
 }
 
 void RTCPeerConnectionHandler::GetStats(
@@ -1650,18 +1653,42 @@ RTCPeerConnectionHandler::GetSenders() {
   blink::WebVector<std::unique_ptr<blink::WebRTCRtpSender>> web_senders(
       webrtc_senders.size());
   for (size_t i = 0; i < web_senders.size(); ++i) {
+    uintptr_t id = RTCRtpSender::getId(webrtc_senders[i]);
+    auto it = rtp_senders_.find(id);
+    if (it == rtp_senders_.end()) {
       rtc::scoped_refptr<webrtc::MediaStreamTrackInterface> webrtc_track =
           webrtc_senders[i]->track();
-    std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef> track_adapter;
+      std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef>
+          track_adapter;
       if (webrtc_track) {
         track_adapter =
             track_adapter_map_->GetLocalTrackAdapter(webrtc_track->id());
         DCHECK(track_adapter);
       }
-    // Create a reference to the sender. Multiple |RTCRtpSender|s can reference
-    // the same webrtc track, see |id|.
-    web_senders[i] = base::MakeUnique<RTCRtpSender>(webrtc_senders[i].get(),
-                                                    std::move(track_adapter));
+      it = rtp_senders_
+               .insert(std::make_pair(
+                   id, base::MakeUnique<RTCRtpSender>(
+                           webrtc_senders[i].get(), std::move(track_adapter))))
+               .first;
+    }
+    web_senders[i] = it->second->ShallowCopy();
+  }
+  // TODO(hbos): When |AddStream| and |RemoveStream| are implemented using
+  // |AddTrack| and |RemoveTrack|, add and remove |rtp_senders_| there instead
+  // of figuring out which ones are no longer used here.
+  // https://crbug.com/738929
+  for (auto it = rtp_senders_.begin(); it != rtp_senders_.end();) {
+    bool sender_exists = false;
+    for (auto webrtc_sender : webrtc_senders) {
+      if (RTCRtpSender::getId(webrtc_sender) == it->first) {
+        sender_exists = true;
+        break;
+      }
+    }
+    if (!sender_exists)
+      it = rtp_senders_.erase(it);
+    else
+      ++it;
   }
   return web_senders;
 }
@@ -1720,9 +1747,16 @@ std::unique_ptr<blink::WebRTCRtpSender> RTCPeerConnectionHandler::AddTrack(
                                         webrtc_streams);
   if (!webrtc_sender)
     return nullptr;
-  return base::MakeUnique<RTCRtpSender>(std::move(webrtc_sender),
+  DCHECK(rtp_senders_.find(RTCRtpSender::getId(webrtc_sender)) ==
+         rtp_senders_.end());
+  auto it = rtp_senders_
+                .insert(std::make_pair(
+                    RTCRtpSender::getId(webrtc_sender),
+                    base::MakeUnique<RTCRtpSender>(std::move(webrtc_sender),
                                                    std::move(track_adapter),
-                                        std::move(stream_adapters));
+                                                   std::move(stream_adapters))))
+                .first;
+  return it->second->ShallowCopy();
 }
 
 bool RTCPeerConnectionHandler::RemoveTrack(blink::WebRTCRtpSender* web_sender) {
@@ -1732,6 +1766,10 @@ bool RTCPeerConnectionHandler::RemoveTrack(blink::WebRTCRtpSender* web_sender) {
     return false;
   }
   sender->OnRemoved();
+  // Make sure |rtp_senders_| is up-to-date. When |AddStream| and |RemoveStream|
+  // is implemented using |AddTrack| and |RemoveTrack|, add and remove
+  // |rtp_senders_| there. https://crbug.com/738929
+  GetSenders();
   return true;
 }
 

content/renderer/media/rtc_peer_connection_handler.h

@@ -21,6 +21,7 @@
 #include "base/threading/thread_checker.h"
 #include "content/common/content_export.h"
 #include "content/renderer/media/webrtc/media_stream_track_metrics.h"
+#include "content/renderer/media/webrtc/rtc_rtp_sender.h"
 #include "content/renderer/media/webrtc/webrtc_media_stream_adapter_map.h"
 #include "content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h"
 #include "ipc/ipc_platform_file.h"
@@ -284,6 +285,16 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
   // peer connection using |AddStream| and |RemoveStream|.
   std::vector<std::unique_ptr<WebRtcMediaStreamAdapterMap::AdapterRef>>
       local_streams_;
+  // Maps |RTCRtpSender::getId|s of |webrtc::RtpSenderInterface| to the
+  // corresponding content layer sender. This is needed to retain the senders'
+  // associated set of streams for senders created by |AddTrack|.
+  std::map<uintptr_t, std::unique_ptr<RTCRtpSender>> rtp_senders_;
+  // We don't need an "rtp_receivers_" map as long as the blink layer receivers
+  // are not GC'd (protecting the relevant adapters from destruction). These can
+  // be constructed anew on every |GetReceivers| call.
+  // TODO(hbos): When receivers can be created separately from remote streams we
+  // should add an "rtp_receivers_" map too to get rid of the requirement for
+  // the blink layer to keep a receiver reference. https://crbug.com/741619
 
   base::WeakPtr<PeerConnectionTracker> peer_connection_tracker_;
 

content/renderer/media/webrtc/rtc_rtp_sender.cc

@@ -54,6 +54,17 @@ RTCRtpSender::RTCRtpSender(
 
 RTCRtpSender::~RTCRtpSender() {}
 
+std::unique_ptr<RTCRtpSender> RTCRtpSender::ShallowCopy() const {
+  std::vector<std::unique_ptr<WebRtcMediaStreamAdapterMap::AdapterRef>>
+      stream_adapter_copies(stream_adapters_.size());
+  for (size_t i = 0; i < stream_adapters_.size(); ++i) {
+    stream_adapter_copies[i] = stream_adapters_[i]->Copy();
+  }
+  return base::MakeUnique<RTCRtpSender>(webrtc_rtp_sender_,
+                                        track_adapter_->Copy(),
+                                        std::move(stream_adapter_copies));
+}
+
 uintptr_t RTCRtpSender::Id() const {
   return getId(webrtc_rtp_sender_.get());
 }

content/renderer/media/webrtc/rtc_rtp_sender.h

@@ -19,7 +19,7 @@
 namespace content {
 
 // Used to surface |webrtc::RtpSenderInterface| to blink. Multiple
-// |RTCRtpSender|s could reference the same webrtc receiver; |id| is the value
+// |RTCRtpSender|s could reference the same webrtc sender; |id| is the value
 // of the pointer to the webrtc sender.
 class CONTENT_EXPORT RTCRtpSender : public blink::WebRTCRtpSender {
  public:
@@ -36,6 +36,10 @@ class CONTENT_EXPORT RTCRtpSender : public blink::WebRTCRtpSender {
           stream_adapters);
   ~RTCRtpSender() override;
 
+  // Creates a shallow copy of the sender, representing the same underlying
+  // webrtc sender as the original.
+  std::unique_ptr<RTCRtpSender> ShallowCopy() const;
+
   uintptr_t Id() const override;
   const blink::WebMediaStreamTrack* Track() const override;
 

content/renderer/media/webrtc/webrtc_media_stream_adapter_map.cc

@@ -39,6 +39,11 @@ WebRtcMediaStreamAdapterMap::AdapterRef::~AdapterRef() {
   }
 }
 
+std::unique_ptr<WebRtcMediaStreamAdapterMap::AdapterRef>
+WebRtcMediaStreamAdapterMap::AdapterRef::Copy() const {
+  return base::WrapUnique(new AdapterRef(map_, it_));
+}
+
 WebRtcMediaStreamAdapterMap::WebRtcMediaStreamAdapterMap(
     PeerConnectionDependencyFactory* const factory,
     scoped_refptr<WebRtcMediaStreamTrackAdapterMap> track_adapter_map)

content/renderer/media/webrtc/webrtc_media_stream_adapter_map.h

@@ -48,6 +48,7 @@ class CONTENT_EXPORT WebRtcMediaStreamAdapterMap
     // adapter it will be disposed and removed from the map.
     ~AdapterRef();
 
+    std::unique_ptr<AdapterRef> Copy() const;
     const WebRtcMediaStreamAdapter& adapter() const {
       return *it_->second.adapter;
     }

content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.cc

@@ -54,6 +54,12 @@ WebRtcMediaStreamTrackAdapterMap::AdapterRef::~AdapterRef() {
   }
 }
 
+std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef>
+WebRtcMediaStreamTrackAdapterMap::AdapterRef::Copy() const {
+  base::AutoLock scoped_lock(map_->lock_);
+  return base::WrapUnique(new AdapterRef(map_, type_, it_));
+}
+
 WebRtcMediaStreamTrackAdapterMap::WebRtcMediaStreamTrackAdapterMap(
     PeerConnectionDependencyFactory* const factory)
     : factory_(factory), main_thread_(base::ThreadTaskRunnerHandle::Get()) {

content/renderer/media/webrtc/webrtc_media_stream_track_adapter_map.h

@@ -50,6 +50,7 @@ class CONTENT_EXPORT WebRtcMediaStreamTrackAdapterMap
     // adapter it will be disposed and removed from the map.
     ~AdapterRef();
 
+    std::unique_ptr<AdapterRef> Copy() const;
     bool is_initialized() const { return adapter_->is_initialized(); }
     const blink::WebMediaStreamTrack& web_track() const {
       return adapter_->web_track();

third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp

@@ -1110,7 +1110,10 @@ void RTCPeerConnection::addStream(ScriptState* script_state,
   if (!valid)
     exception_state.ThrowDOMException(kSyntaxError,
                                       "Unable to add the provided stream.");
-  // Ensure |rtp_senders_| is up-to-date.
+  // Ensure |rtp_senders_| is up-to-date so that |addTrack| knows if a sender
+  // already exists for a track. When |addStream| and |removeStream| are
+  // are implemented using |addTrack| and |removeTrack| we can simply add and
+  // remove senders there instead. https://crbug.com/738929
   getSenders();
 }
 
@@ -1137,10 +1140,16 @@ void RTCPeerConnection::removeStream(MediaStream* stream,
 }
 
 MediaStreamVector RTCPeerConnection::getLocalStreams() const {
+  // TODO(hbos): We should define this as "the streams of all senders" instead
+  // of a set that we add to and subtract from on |addStream| and
+  // |removeStream|. https://crbug.com/738918
   return local_streams_;
 }
 
 MediaStreamVector RTCPeerConnection::getRemoteStreams() const {
+  // TODO(hbos): We should define this as "the streams of all receivers" instead
+  // of a set that we add to and subtract from on a remote stream being added or
+  // removed. https://crbug.com/741618
   return remote_streams_;
 }
 
@@ -1305,6 +1314,9 @@ void RTCPeerConnection::removeTrack(RTCRtpSender* sender,
   // being nulled.
   DCHECK(!sender->web_rtp_sender()->Track());
   sender->SetTrack(nullptr);
+  // TODO(hbos): When |addStream| and |removeStream| are implemented using
+  // |addTrack| and |removeTrack|, we should remove |sender| from |rtp_senders_|
+  // here. https://crbug.com/738929
 }
 
 RTCDataChannel* RTCPeerConnection::createDataChannel(
@@ -1481,6 +1493,9 @@ void RTCPeerConnection::DidRemoveRemoteStream(
   DCHECK(pos != kNotFound);
   remote_streams_.erase(pos);
   stream->UnregisterObserver(this);
+  // TODO(hbos): When we listen to receivers/tracks being added and removed
+  // instead of streams we should remove the receiver(s) from |rtp_receivers_|
+  // here. https://crbug.com/741619
 
   ScheduleDispatchEvent(
       MediaStreamEvent::Create(EventTypeNames::removestream, stream));

third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h

@@ -247,7 +247,7 @@ class MODULES_EXPORT RTCPeerConnection final
   void ScheduleDispatchEvent(Event*);
   void ScheduleDispatchEvent(Event*, std::unique_ptr<BoolFunction>);
   void DispatchScheduledEvent();
-  MediaStreamTrack* GetTrack(const WebMediaStreamTrack& web_track) const;
+  MediaStreamTrack* GetTrack(const WebMediaStreamTrack&) const;
 
   void ChangeSignalingState(WebRTCPeerConnectionHandlerClient::SignalingState);
   void ChangeIceGatheringState(
@@ -276,8 +276,8 @@ class MODULES_EXPORT RTCPeerConnection final
   // |rtp_receivers_|.
   HeapHashMap<WeakMember<MediaStreamComponent>, WeakMember<MediaStreamTrack>>
       tracks_;
-  HeapHashMap<uintptr_t, WeakMember<RTCRtpSender>> rtp_senders_;
-  HeapHashMap<uintptr_t, WeakMember<RTCRtpReceiver>> rtp_receivers_;
+  HeapHashMap<uintptr_t, Member<RTCRtpSender>> rtp_senders_;
+  HeapHashMap<uintptr_t, Member<RTCRtpReceiver>> rtp_receivers_;
 
   std::unique_ptr<WebRTCPeerConnectionHandler> peer_handler_;