Revert "RTCPeerConnectionHandler: Keep track of senders."

This reverts commit 4979aac4.

Reason for revert: None of these tests pass with Clang on Windows. That suggests that there are side effects that depend on order of function call argument evaluation, but I haven't been able to pin it down. http://crbug.com/746971

Original change's description:
> 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/567184
> Reviewed-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}

TBR=hbos@chromium.org,deadbeef@chromium.org,jochen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 740650
Change-Id: Ie514367178863307b3596a5a34d2fdaed0fac817
Reviewed-on: https://chromium-review.googlesource.com/580367Reviewed-by: Reid Kleckner <rnk@chromium.org>
Commit-Queue: Reid Kleckner <rnk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488415}

chrome/browser/media/webrtc/webrtc_rtp_browsertest.cc

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

content/renderer/media/rtc_peer_connection_handler.cc

@@ -30,6 +30,7 @@
 #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"
@@ -1585,10 +1586,6 @@ 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(
@@ -1653,42 +1650,18 @@ 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;
-      if (webrtc_track) {
-        track_adapter =
-            track_adapter_map_->GetLocalTrackAdapter(webrtc_track->id());
-        DCHECK(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;
-      }
+    rtc::scoped_refptr<webrtc::MediaStreamTrackInterface> webrtc_track =
+        webrtc_senders[i]->track();
+    std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef> track_adapter;
+    if (webrtc_track) {
+      track_adapter =
+          track_adapter_map_->GetLocalTrackAdapter(webrtc_track->id());
+      DCHECK(track_adapter);
     }
-    if (!sender_exists)
-      it = rtp_senders_.erase(it);
-    else
-      ++it;
+    // 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));
   }
   return web_senders;
 }
@@ -1747,16 +1720,9 @@ std::unique_ptr<blink::WebRTCRtpSender> RTCPeerConnectionHandler::AddTrack(
                                         webrtc_streams);
   if (!webrtc_sender)
     return nullptr;
-  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))))
-                .first;
-  return it->second->ShallowCopy();
+  return base::MakeUnique<RTCRtpSender>(std::move(webrtc_sender),
+                                        std::move(track_adapter),
+                                        std::move(stream_adapters));
 }
 
 bool RTCPeerConnectionHandler::RemoveTrack(blink::WebRTCRtpSender* web_sender) {
@@ -1766,10 +1732,6 @@ 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,7 +21,6 @@
 #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"
@@ -285,16 +284,6 @@ 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,17 +54,6 @@ 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 sender; |id| is the value
+// |RTCRtpSender|s could reference the same webrtc receiver; |id| is the value
 // of the pointer to the webrtc sender.
 class CONTENT_EXPORT RTCRtpSender : public blink::WebRTCRtpSender {
  public:
@@ -36,10 +36,6 @@ 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,11 +39,6 @@ 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,7 +48,6 @@ 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,12 +54,6 @@ 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,7 +50,6 @@ 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,10 +1110,7 @@ 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 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
+  // Ensure |rtp_senders_| is up-to-date.
   getSenders();
 }
 
@@ -1140,16 +1137,10 @@ 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_;
 }
 
@@ -1314,9 +1305,6 @@ 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(
@@ -1489,9 +1477,6 @@ 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&) const;
+  MediaStreamTrack* GetTrack(const WebMediaStreamTrack& web_track) const;
 
   // The "Change" methods set the state asynchronously and fire the
   // corresponding event immediately after changing the state (if it was really
@@ -296,8 +296,8 @@ class MODULES_EXPORT RTCPeerConnection final
   // |rtp_receivers_|.
   HeapHashMap<WeakMember<MediaStreamComponent>, WeakMember<MediaStreamTrack>>
       tracks_;
-  HeapHashMap<uintptr_t, Member<RTCRtpSender>> rtp_senders_;
-  HeapHashMap<uintptr_t, Member<RTCRtpReceiver>> rtp_receivers_;
+  HeapHashMap<uintptr_t, WeakMember<RTCRtpSender>> rtp_senders_;
+  HeapHashMap<uintptr_t, WeakMember<RTCRtpReceiver>> rtp_receivers_;
 
   std::unique_ptr<WebRTCPeerConnectionHandler> peer_handler_;