From dffead8835d18ef5c76ba90631ebdc26a8ca4644 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Tue, 6 Feb 2018 10:31:29 -0800 Subject: [PATCH] Fail CreateAnswer if signaling state is not correct This changes CreateAnswer to become compliant with the WebRTC 1.0 specification which details that createAnswer should fail if the PeerConnection is in a state other than 'have-remote-offer' or 'have-local-pranswer'. Bug: webrtc:8813 Change-Id: I7ca41bdebda1ea163aec8815267c1bbfd7d6d11e Reviewed-on: https://webrtc-review.googlesource.com/47581 Commit-Queue: Steve Anton Reviewed-by: Taylor Brandstetter Cr-Commit-Position: refs/heads/master@{#21923} --- pc/peerconnection.cc | 37 +++++++++----------- pc/peerconnection_ice_unittest.cc | 2 +- pc/peerconnection_signaling_unittest.cc | 46 +++++++------------------ 3 files changed, 29 insertions(+), 56 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 40f3ff473b..4ec26fa3a6 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1679,20 +1679,18 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer, return; } - if (IsClosed()) { - std::string error = "CreateAnswer called when PeerConnection is closed."; + if (!(signaling_state_ == kHaveRemoteOffer || + signaling_state_ == kHaveLocalPrAnswer)) { + std::string error = + "PeerConnection cannot create an answer in a state other than " + "have-remote-offer or have-local-pranswer."; RTC_LOG(LS_ERROR) << error; PostCreateSessionDescriptionFailure(observer, error); return; } - if (remote_description() && - remote_description()->GetType() != SdpType::kOffer) { - std::string error = "CreateAnswer called without remote offer."; - RTC_LOG(LS_ERROR) << error; - PostCreateSessionDescriptionFailure(observer, error); - return; - } + // The remote description should be set if we're in the right state. + RTC_DCHECK(remote_description()); if (IsUnifiedPlan()) { if (options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) { @@ -3499,18 +3497,15 @@ void PeerConnection::GetOptionsForPlanBAnswer( rtc::Optional audio_index; rtc::Optional video_index; rtc::Optional data_index; - if (remote_description()) { - // The pending remote description should be an offer. - RTC_DCHECK(remote_description()->GetType() == SdpType::kOffer); - // Generate m= sections that match those in the offer. - // Note that mediasession.cc will handle intersection our preferred - // direction with the offered direction. - GenerateMediaDescriptionOptions( - remote_description(), - RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio), - RtpTransceiverDirectionFromSendRecv(send_video, recv_video), - &audio_index, &video_index, &data_index, session_options); - } + + // Generate m= sections that match those in the offer. + // Note that mediasession.cc will handle intersection our preferred + // direction with the offered direction. + GenerateMediaDescriptionOptions( + remote_description(), + RtpTransceiverDirectionFromSendRecv(send_audio, recv_audio), + RtpTransceiverDirectionFromSendRecv(send_video, recv_video), &audio_index, + &video_index, &data_index, session_options); cricket::MediaDescriptionOptions* audio_media_description_options = !audio_index ? nullptr diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index b6a8124b2c..6f6bd73871 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -303,7 +303,7 @@ TEST_P(PeerConnectionIceTest, AnswerContainsGatheredCandidates) { EXPECT_TRUE_WAIT(callee->IsIceGatheringDone(), kIceCandidatesTimeout); - auto answer = callee->CreateAnswer(); + auto* answer = callee->pc()->local_description(); EXPECT_LT(0u, caller->observer()->GetCandidatesByMline(0).size()); EXPECT_EQ(callee->observer()->GetCandidatesByMline(0).size(), answer->candidates(0)->count()); diff --git a/pc/peerconnection_signaling_unittest.cc b/pc/peerconnection_signaling_unittest.cc index cd26f8ba51..8f1792b717 100644 --- a/pc/peerconnection_signaling_unittest.cc +++ b/pc/peerconnection_signaling_unittest.cc @@ -229,18 +229,9 @@ TEST_P(PeerConnectionSignalingStateTest, CreateAnswer) { } else { std::string error; ASSERT_FALSE(wrapper->CreateAnswer(RTCOfferAnswerOptions(), &error)); - if (wrapper->signaling_state() == SignalingState::kClosed) { - EXPECT_PRED_FORMAT2(AssertStartsWith, error, - "CreateAnswer called when PeerConnection is closed."); - } else if (wrapper->signaling_state() == - SignalingState::kHaveRemotePrAnswer) { - EXPECT_PRED_FORMAT2(AssertStartsWith, error, - "CreateAnswer called without remote offer."); - } else { - EXPECT_PRED_FORMAT2( - AssertStartsWith, error, - "CreateAnswer can't be called before SetRemoteDescription."); - } + EXPECT_EQ(error, + "PeerConnection cannot create an answer in a state other than " + "have-remote-offer or have-local-pranswer."); } } @@ -369,32 +360,19 @@ INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest, SignalingState::kHaveRemotePrAnswer), Bool())); -TEST_F(PeerConnectionSignalingTest, - CreateAnswerSucceedsIfStableAndRemoteDescriptionIsOffer) { +// Test that CreateAnswer fails if a round of offer/answer has been done and +// the PeerConnection is in the stable state. +TEST_F(PeerConnectionSignalingTest, CreateAnswerFailsIfStable) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_TRUE( - caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); - - ASSERT_EQ(SignalingState::kStable, callee->signaling_state()); - EXPECT_TRUE(callee->CreateAnswer()); -} - -TEST_F(PeerConnectionSignalingTest, - CreateAnswerFailsIfStableButRemoteDescriptionIsAnswer) { - auto caller = CreatePeerConnection(); - auto callee = CreatePeerConnection(); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_TRUE( - caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); ASSERT_EQ(SignalingState::kStable, caller->signaling_state()); - std::string error; - ASSERT_FALSE(caller->CreateAnswer(RTCOfferAnswerOptions(), &error)); - EXPECT_EQ("CreateAnswer called without remote offer.", error); + EXPECT_FALSE(caller->CreateAnswer()); + + ASSERT_EQ(SignalingState::kStable, callee->signaling_state()); + EXPECT_FALSE(callee->CreateAnswer()); } // According to https://tools.ietf.org/html/rfc3264#section-8, the session id @@ -429,7 +407,7 @@ TEST_F(PeerConnectionSignalingTest, ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - auto original_answer = callee->CreateAnswerAndSetAsLocal(); + auto original_answer = callee->CreateAnswer(); const std::string original_id = original_answer->session_id(); const std::string original_version = original_answer->session_version();