Attempting to fix lingering issues with BUNDLE negotiation.

I found one additional way a crash could occur: "OnRtpTransportChanged"
being called instead of "OnDtlsTransportChanged", due to a mixup of m=
section types. I could reproduce this by:

1. Applying description with RTP data channel m= section.
2. Applying description with both a rejected RTP data channel m=
   section and rejected SCTP m= section.

This is a very strange scenario, but maybe there are other ways to
reproduce that I haven't thought of.

The solution is to combine "OnRtpTransportChanged" and
"OnDtlsTransportChanged", and not do anything with the content type.
This simplifies the code a bit as well.

Bug: chromium:827917
Change-Id: If6818ea0c41573255831534060b30c76a6544e04
Reviewed-on: https://webrtc-review.googlesource.com/70360
Reviewed-by: Zhi Huang <zhihuang@webrtc.org>
Commit-Queue: Zhi Huang <zhihuang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22893}

5 files changed, 40 insertions, 72 deletions

diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc
index b8234fa413..d05339e1a1 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -574,10 +574,7 @@ RTCError JsepTransportController::ApplyDescription_n(
     const cricket::TransportInfo& transport_info =
         description->transport_infos()[i];
     if (content_info.rejected) {
-      if (!HandleRejectedContent(content_info, description)) {
-        return RTCError(RTCErrorType::INVALID_PARAMETER,
-                        "Failed to process the rejected m= section.");
-      }
+      HandleRejectedContent(content_info, description);
       continue;
     }
 
@@ -742,20 +739,16 @@ RTCError JsepTransportController::ValidateContent(
   return RTCError::OK();
 }
 
-bool JsepTransportController::HandleRejectedContent(
+void JsepTransportController::HandleRejectedContent(
     const cricket::ContentInfo& content_info,
     const cricket::SessionDescription* description) {
-  bool ret = true;
   // If the content is rejected, let the
   // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
   // then destroy the cricket::JsepTransport.
-  ret = RemoveTransportForMid(content_info.name, content_info.type);
+  RemoveTransportForMid(content_info.name);
   if (content_info.name == bundled_mid()) {
     for (auto content_name : bundle_group_->content_names()) {
-      const cricket::ContentInfo* content_in_group =
-          description->GetContentByName(content_name);
-      RTC_DCHECK(content_in_group);
-      ret = ret && RemoveTransportForMid(content_name, content_in_group->type);
+      RemoveTransportForMid(content_name);
     }
     bundle_group_.reset();
   } else if (IsBundled(content_info.name)) {
@@ -766,10 +759,7 @@ bool JsepTransportController::HandleRejectedContent(
       bundle_group_.reset();
     }
   }
-  if (ret) {
-    MaybeDestroyJsepTransport(content_info.name);
-  }
-  return ret;
+  MaybeDestroyJsepTransport(content_info.name);
 }
 
 bool JsepTransportController::HandleBundledContent(
@@ -779,8 +769,7 @@ bool JsepTransportController::HandleBundledContent(
   // If the content is bundled, let the
   // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
   // then destroy the cricket::JsepTransport.
-  if (SetTransportForMid(content_info.name, jsep_transport,
-                         content_info.type)) {
+  if (SetTransportForMid(content_info.name, jsep_transport)) {
     MaybeDestroyJsepTransport(content_info.name);
     return true;
   }
@@ -789,37 +778,25 @@ bool JsepTransportController::HandleBundledContent(
 
 bool JsepTransportController::SetTransportForMid(
     const std::string& mid,
-    cricket::JsepTransport* jsep_transport,
-    cricket::MediaProtocolType protocol_type) {
+    cricket::JsepTransport* jsep_transport) {
   RTC_DCHECK(jsep_transport);
   if (mid_to_transport_[mid] == jsep_transport) {
     return true;
   }
 
-  bool ret = true;
   mid_to_transport_[mid] = jsep_transport;
-  if (protocol_type == cricket::MediaProtocolType::kRtp) {
-    ret = config_.transport_observer->OnRtpTransportChanged(
-        mid, jsep_transport->rtp_transport());
-  } else {
-    config_.transport_observer->OnDtlsTransportChanged(
-        mid, jsep_transport->rtp_dtls_transport());
-  }
-  return ret;
+  return config_.transport_observer->OnTransportChanged(
+      mid, jsep_transport->rtp_transport(),
+      jsep_transport->rtp_dtls_transport());
 }
 
-bool JsepTransportController::RemoveTransportForMid(
-    const std::string& mid,
-    cricket::MediaProtocolType protocol_type) {
-  bool ret = true;
-  if (protocol_type == cricket::MediaProtocolType::kRtp) {
-    ret = config_.transport_observer->OnRtpTransportChanged(mid, nullptr);
-    RTC_DCHECK(ret);
-  } else {
-    config_.transport_observer->OnDtlsTransportChanged(mid, nullptr);
-  }
+void JsepTransportController::RemoveTransportForMid(const std::string& mid) {
+  bool ret =
+      config_.transport_observer->OnTransportChanged(mid, nullptr, nullptr);
+  // Calling OnTransportChanged with nullptr should always succeed, since it is
+  // only expected to fail when adding media to a transport (not removing).
+  RTC_DCHECK(ret);
   mid_to_transport_.erase(mid);
-  return ret;
 }
 
 cricket::JsepTransportDescription
@@ -996,8 +973,7 @@ RTCError JsepTransportController::MaybeCreateJsepTransport(
           std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport));
   jsep_transport->SignalRtcpMuxActive.connect(
       this, &JsepTransportController::UpdateAggregateStates_n);
-  SetTransportForMid(content_info.name, jsep_transport.get(),
-                     content_info.type);
+  SetTransportForMid(content_info.name, jsep_transport.get());
 
   jsep_transports_by_name_[content_info.name] = std::move(jsep_transport);
   UpdateAggregateStates_n();
diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h
index c3c7919f50..ebe105b510 100644
--- a/pc/jseptransportcontroller.h
+++ b/pc/jseptransportcontroller.h
@@ -53,11 +53,9 @@ class JsepTransportController : public sigslot::has_slots<>,
     // Returns true if media associated with |mid| was successfully set up to be
     // demultiplexed on |rtp_transport|. Could return false if two bundled m=
     // sections use the same SSRC, for example.
-    virtual bool OnRtpTransportChanged(const std::string& mid,
-                                       RtpTransportInternal* rtp_transport) = 0;
-
-    virtual void OnDtlsTransportChanged(
+    virtual bool OnTransportChanged(
         const std::string& mid,
+        RtpTransportInternal* rtp_transport,
         cricket::DtlsTransportInternal* dtls_transport) = 0;
   };
 
@@ -189,15 +187,13 @@ class JsepTransportController : public sigslot::has_slots<>,
       const cricket::SessionDescription* description);
   RTCError ValidateContent(const cricket::ContentInfo& content_info);
 
-  bool HandleRejectedContent(const cricket::ContentInfo& content_info,
+  void HandleRejectedContent(const cricket::ContentInfo& content_info,
                              const cricket::SessionDescription* description);
   bool HandleBundledContent(const cricket::ContentInfo& content_info);
 
   bool SetTransportForMid(const std::string& mid,
-                          cricket::JsepTransport* jsep_transport,
-                          cricket::MediaProtocolType protocol_type);
-  bool RemoveTransportForMid(const std::string& mid,
-                             cricket::MediaProtocolType protocol_type);
+                          cricket::JsepTransport* jsep_transport);
+  void RemoveTransportForMid(const std::string& mid);
 
   cricket::JsepTransportDescription CreateJsepTransportDescription(
       cricket::ContentInfo content_info,
diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc
index b8ec316d72..f3bf2aff05 100644
--- a/pc/jseptransportcontroller_unittest.cc
+++ b/pc/jseptransportcontroller_unittest.cc
@@ -261,16 +261,13 @@ class JsepTransportControllerTest : public JsepTransportController::Observer,
   }
 
   // JsepTransportController::Observer overrides.
-  bool OnRtpTransportChanged(const std::string& mid,
-                             RtpTransportInternal* rtp_transport) override {
-    changed_rtp_transport_by_mid_[mid] = rtp_transport;
-    return true;
-  }
-
-  void OnDtlsTransportChanged(
+  bool OnTransportChanged(
       const std::string& mid,
+      RtpTransportInternal* rtp_transport,
       cricket::DtlsTransportInternal* dtls_transport) override {
+    changed_rtp_transport_by_mid_[mid] = rtp_transport;
     changed_dtls_transport_by_mid_[mid] = dtls_transport;
+    return true;
   }
 
   // Information received from signals from transport controller.
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 092b69a2ac..c352960383 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -6121,23 +6121,19 @@ void PeerConnection::DestroyBaseChannel(cricket::BaseChannel* channel) {
   }
 }
 
-bool PeerConnection::OnRtpTransportChanged(
+bool PeerConnection::OnTransportChanged(
     const std::string& mid,
-    RtpTransportInternal* rtp_transport) {
+    RtpTransportInternal* rtp_transport,
+    cricket::DtlsTransportInternal* dtls_transport) {
+  bool ret = true;
   auto base_channel = GetChannel(mid);
   if (base_channel) {
-    return base_channel->SetRtpTransport(rtp_transport);
+    ret = base_channel->SetRtpTransport(rtp_transport);
   }
-  return true;
-}
-
-void PeerConnection::OnDtlsTransportChanged(
-    const std::string& mid,
-    cricket::DtlsTransportInternal* dtls_transport) {
-  if (sctp_transport_) {
-    RTC_DCHECK(mid == sctp_mid_);
+  if (sctp_transport_ && mid == sctp_mid_) {
     sctp_transport_->SetDtlsTransport(dtls_transport);
   }
+  return ret;
 }
 
 void PeerConnection::ClearStatsCache() {
diff --git a/pc/peerconnection.h b/pc/peerconnection.h
index 3545479d2a..1b00ef5103 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -881,11 +881,14 @@ class PeerConnection : public PeerConnectionInternal,
   void DestroyBaseChannel(cricket::BaseChannel* channel);
 
   // JsepTransportController::Observer override.
-  bool OnRtpTransportChanged(const std::string& mid,
-                             RtpTransportInternal* rtp_transport) override;
-
-  void OnDtlsTransportChanged(
+  //
+  // Called by |transport_controller_| when processing transport information
+  // from a session description, and the mapping from m= sections to transports
+  // changed (as a result of BUNDLE negotiation, or m= sections being
+  // rejected).
+  bool OnTransportChanged(
       const std::string& mid,
+      RtpTransportInternal* rtp_transport,
       cricket::DtlsTransportInternal* dtls_transport) override;
 
   sigslot::signal1<DataChannel*> SignalDataChannelCreated_;