From 0ab56511f14ce50b23341fd0c548575d024df2a5 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Thu, 12 Apr 2018 10:30:48 -0700 Subject: [PATCH] Fix handling of empty BUNDLE groups. This CL fixes issues when applying a description with an empty BUNDLE group (previously it would fail, after recent refactoring it started crashing). This CL also will cause an empty BUNDLE group to be generated when it should be. Namely, when responding to an offer that had a BUNDLE group, rejecting everything in it. Bug: chromium:831996 Change-Id: I4e705a328daef4e81f8f1ace6aa73ddfa13c0107 Reviewed-on: https://webrtc-review.googlesource.com/69720 Reviewed-by: Zhi Huang Commit-Queue: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#22844} --- pc/jseptransportcontroller.cc | 7 +++--- pc/jseptransportcontroller.h | 4 +-- pc/mediasession.cc | 11 +++++++-- pc/peerconnection_bundle_unittest.cc | 37 ++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index 31d27cae4d..b5ce5fe0a7 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -589,7 +589,7 @@ RTCError JsepTransportController::ApplyDescription_n( } std::vector extension_ids; - if (bundle_group_ && content_info.name == *bundled_mid()) { + if (bundled_mid() && content_info.name == *bundled_mid()) { extension_ids = merged_encrypted_extension_ids; } else { extension_ids = GetEncryptedHeaderExtensionIds(content_info); @@ -685,8 +685,9 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( } if (ShouldUpdateBundleGroup(type, description)) { - std::string new_bundled_mid = *(new_bundle_group->FirstContentName()); - if (bundled_mid() && *bundled_mid() != new_bundled_mid) { + const std::string* new_bundled_mid = new_bundle_group->FirstContentName(); + if (bundled_mid() && new_bundled_mid && + *bundled_mid() != *new_bundled_mid) { return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Changing the negotiated BUNDLE-tag is not supported."); } diff --git a/pc/jseptransportcontroller.h b/pc/jseptransportcontroller.h index 794175a6e2..6e44534f08 100644 --- a/pc/jseptransportcontroller.h +++ b/pc/jseptransportcontroller.h @@ -201,8 +201,8 @@ class JsepTransportController : public sigslot::has_slots<>, rtc::Optional bundled_mid() const { rtc::Optional bundled_mid; - if (bundle_group_) { - bundled_mid = std::move(*(bundle_group_->FirstContentName())); + if (bundle_group_ && bundle_group_->FirstContentName()) { + bundled_mid = *(bundle_group_->FirstContentName()); } return bundled_mid; } diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 3bf931c613..eba3249d03 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -1494,10 +1494,17 @@ SessionDescription* MediaSessionDescriptionFactory::CreateAnswer( } } - // Only put BUNDLE group in answer if nonempty. - if (answer_bundle.FirstContentName()) { + // If a BUNDLE group was offered, put a BUNDLE group in the answer even if + // it's empty. RFC5888 says: + // + // A SIP entity that receives an offer that contains an "a=group" line + // with semantics that are understood MUST return an answer that + // contains an "a=group" line with the same semantics. + if (offer_bundle) { answer->AddGroup(answer_bundle); + } + if (answer_bundle.FirstContentName()) { // Share the same ICE credentials and crypto params across all contents, // as BUNDLE requires. if (!UpdateTransportInfoForBundle(answer_bundle, answer.get())) { diff --git a/pc/peerconnection_bundle_unittest.cc b/pc/peerconnection_bundle_unittest.cc index c71662bda0..a1b69d8ae1 100644 --- a/pc/peerconnection_bundle_unittest.cc +++ b/pc/peerconnection_bundle_unittest.cc @@ -248,6 +248,13 @@ class PeerConnectionBundleTest PeerConnectionBundleTest() : PeerConnectionBundleBaseTest(GetParam()) {} }; +class PeerConnectionBundleTestUnifiedPlan + : public PeerConnectionBundleBaseTest { + protected: + PeerConnectionBundleTestUnifiedPlan() + : PeerConnectionBundleBaseTest(SdpSemantics::kUnifiedPlan) {} +}; + SdpContentMutator RemoveRtcpMux() { return [](cricket::ContentInfo* content, cricket::TransportInfo* transport) { content->media_description()->set_rtcp_mux(false); @@ -804,4 +811,34 @@ INSTANTIATE_TEST_CASE_P(PeerConnectionBundleTest, Values(SdpSemantics::kPlanB, SdpSemantics::kUnifiedPlan)); +// According to RFC5888, if an endpoint understands the semantics of an +// "a=group", it MUST return an answer with that group. So, an empty BUNDLE +// group is valid when the answerer rejects all m= sections (by stopping all +// transceivers), meaning there's nothing to bundle. +// +// Only writing this test for Unified Plan mode, since there's no way to reject +// m= sections in answers for Plan B without SDP munging. +TEST_F(PeerConnectionBundleTestUnifiedPlan, + EmptyBundleGroupCreatedInAnswerWhenAppropriate) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnection(); + + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + // Stop all transceivers, causing all m= sections to be rejected. + for (const auto& transceiver : callee->pc()->GetTransceivers()) { + transceiver->Stop(); + } + EXPECT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + + // Verify that the answer actually contained an empty bundle group. + const SessionDescriptionInterface* desc = callee->pc()->local_description(); + ASSERT_NE(nullptr, desc); + const cricket::ContentGroup* bundle_group = + desc->description()->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + ASSERT_NE(nullptr, bundle_group); + EXPECT_TRUE(bundle_group->content_names().empty()); +} + } // namespace webrtc