admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Attempting to fix lingering issues with BUNDLE negotiation.

Browse Source 
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}
This commit is contained in:
Taylor Brandstetter 2018-04-16 16:42:14 -07:00 committed by Commit Bot
parent f82644c9c7
commit cbaa254641
5 changed files with 40 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

58
pc/jseptransportcontroller.cc
View File

@ -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();

14
pc/jseptransportcontroller.h
View File

@ -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,

11
pc/jseptransportcontroller_unittest.cc
View File

@ -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.

18
pc/peerconnection.cc
View File

@ -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() {

11
pc/peerconnection.h
View File

@ -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_;