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}diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc
index b8234fa..d05339e 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -574,10 +574,7 @@
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 @@
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 @@
bundle_group_.reset();
}
}
- if (ret) {
- MaybeDestroyJsepTransport(content_info.name);
- }
- return ret;
+ MaybeDestroyJsepTransport(content_info.name);
}
bool JsepTransportController::HandleBundledContent(
@@ -779,8 +769,7 @@
// 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::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 @@
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 c3c7919..ebe105b 100644
--- a/pc/jseptransportcontroller.h
+++ b/pc/jseptransportcontroller.h
@@ -53,11 +53,9 @@
// 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 @@
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 b8ec316..f3bf2af 100644
--- a/pc/jseptransportcontroller_unittest.cc
+++ b/pc/jseptransportcontroller_unittest.cc
@@ -261,16 +261,13 @@
}
// 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 092b69a..c352960 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -6121,23 +6121,19 @@
}
}
-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 3545479..1b00ef5 100644
--- a/pc/peerconnection.h
+++ b/pc/peerconnection.h
@@ -881,11 +881,14 @@
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_;
