diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 4758f4f62c..5cccd854d7 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1674,17 +1674,20 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer, return; } - if (!(signaling_state_ == kHaveRemoteOffer || - signaling_state_ == kHaveLocalPrAnswer)) { - std::string error = - "CreateAnswer called when PeerConnection is in a wrong state."; + if (IsClosed()) { + std::string error = "CreateAnswer called when PeerConnection is closed."; 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 (remote_description() && + remote_description()->GetType() != SdpType::kOffer) { + std::string error = "CreateAnswer called without remote offer."; + RTC_LOG(LS_ERROR) << error; + PostCreateSessionDescriptionFailure(observer, error); + return; + } if (IsUnifiedPlan()) { if (options.offer_to_receive_audio != RTCOfferAnswerOptions::kUndefined) { @@ -3483,15 +3486,18 @@ void PeerConnection::GetOptionsForPlanBAnswer( rtc::Optional audio_index; rtc::Optional video_index; rtc::Optional data_index; - - // 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); + 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); + } cricket::MediaDescriptionOptions* audio_media_description_options = !audio_index ? nullptr diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index 6f6bd73871..b6a8124b2c 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->pc()->local_description(); + auto answer = callee->CreateAnswer(); 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 2cb2fc7c4b..cd26f8ba51 100644 --- a/pc/peerconnection_signaling_unittest.cc +++ b/pc/peerconnection_signaling_unittest.cc @@ -55,14 +55,12 @@ class PeerConnectionWrapperForSignalingTest : public PeerConnectionWrapper { } }; -class PeerConnectionSignalingBaseTest : public ::testing::Test { +class PeerConnectionSignalingTest : public ::testing::Test { protected: typedef std::unique_ptr WrapperPtr; - explicit PeerConnectionSignalingBaseTest(SdpSemantics sdp_semantics) - : vss_(new rtc::VirtualSocketServer()), - main_(vss_.get()), - sdp_semantics_(sdp_semantics) { + PeerConnectionSignalingTest() + : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { #ifdef WEBRTC_ANDROID InitializeAndroidObjects(); #endif @@ -78,10 +76,8 @@ class PeerConnectionSignalingBaseTest : public ::testing::Test { WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { auto observer = rtc::MakeUnique(); - RTCConfiguration modified_config = config; - modified_config.sdp_semantics = sdp_semantics_; - auto pc = pc_factory_->CreatePeerConnection(modified_config, nullptr, - nullptr, observer.get()); + auto pc = pc_factory_->CreatePeerConnection(config, nullptr, nullptr, + observer.get()); if (!pc) { return nullptr; } @@ -106,24 +102,16 @@ class PeerConnectionSignalingBaseTest : public ::testing::Test { std::unique_ptr vss_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr pc_factory_; - const SdpSemantics sdp_semantics_; }; -class PeerConnectionSignalingTest - : public PeerConnectionSignalingBaseTest, - public ::testing::WithParamInterface { - protected: - PeerConnectionSignalingTest() : PeerConnectionSignalingBaseTest(GetParam()) {} -}; - -TEST_P(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) { +TEST_F(PeerConnectionSignalingTest, SetLocalOfferTwiceWorks) { auto caller = CreatePeerConnection(); EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); EXPECT_TRUE(caller->SetLocalDescription(caller->CreateOffer())); } -TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { +TEST_F(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -131,14 +119,14 @@ TEST_P(PeerConnectionSignalingTest, SetRemoteOfferTwiceWorks) { EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); } -TEST_P(PeerConnectionSignalingTest, FailToSetNullLocalDescription) { +TEST_F(PeerConnectionSignalingTest, FailToSetNullLocalDescription) { auto caller = CreatePeerConnection(); std::string error; ASSERT_FALSE(caller->SetLocalDescription(nullptr, &error)); EXPECT_EQ("SessionDescription is NULL.", error); } -TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { +TEST_F(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { auto caller = CreatePeerConnection(); std::string error; ASSERT_FALSE(caller->SetRemoteDescription(nullptr, &error)); @@ -154,15 +142,9 @@ TEST_P(PeerConnectionSignalingTest, FailToSetNullRemoteDescription) { // state the PeerConnection was created in before it was closed. class PeerConnectionSignalingStateTest - : public PeerConnectionSignalingBaseTest, - public ::testing::WithParamInterface< - std::tuple> { + : public PeerConnectionSignalingTest, + public ::testing::WithParamInterface> { protected: - PeerConnectionSignalingStateTest() - : PeerConnectionSignalingBaseTest(std::get<0>(GetParam())), - state_under_test_(std::make_tuple(std::get<1>(GetParam()), - std::get<2>(GetParam()))) {} - RTCConfiguration GetConfig() { RTCConfiguration config; config.certificates.push_back( @@ -170,10 +152,6 @@ class PeerConnectionSignalingStateTest return config; } - WrapperPtr CreatePeerConnectionUnderTest() { - return CreatePeerConnectionInState(state_under_test_); - } - WrapperPtr CreatePeerConnectionInState(SignalingState state) { return CreatePeerConnectionInState(std::make_tuple(state, false)); } @@ -229,12 +207,10 @@ class PeerConnectionSignalingStateTest return wrapper; } - - std::tuple state_under_test_; }; TEST_P(PeerConnectionSignalingStateTest, CreateOffer) { - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() != SignalingState::kClosed) { EXPECT_TRUE(wrapper->CreateOffer()); } else { @@ -246,21 +222,30 @@ TEST_P(PeerConnectionSignalingStateTest, CreateOffer) { } TEST_P(PeerConnectionSignalingStateTest, CreateAnswer) { - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->CreateAnswer()); } else { std::string error; ASSERT_FALSE(wrapper->CreateAnswer(RTCOfferAnswerOptions(), &error)); - EXPECT_PRED_FORMAT2( - AssertStartsWith, error, - "CreateAnswer called when PeerConnection is in a wrong state."); + 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."); + } } } TEST_P(PeerConnectionSignalingStateTest, SetLocalOffer) { - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kStable || wrapper->signaling_state() == SignalingState::kHaveLocalOffer) { // Need to call CreateOffer on the PeerConnection under test, otherwise when @@ -288,7 +273,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalPrAnswer) { auto pranswer = CloneSessionDescription(wrapper_for_pranswer->pc()->local_description()); - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetLocalDescription(std::move(pranswer))); @@ -306,7 +291,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetLocalAnswer) { CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer); auto answer = wrapper_for_answer->CreateAnswer(); - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kHaveLocalPrAnswer || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetLocalDescription(std::move(answer))); @@ -325,7 +310,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteOffer) { auto offer = CloneSessionDescription(wrapper_for_offer->pc()->remote_description()); - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kStable || wrapper->signaling_state() == SignalingState::kHaveRemoteOffer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(offer))); @@ -344,7 +329,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemotePrAnswer) { auto pranswer = CloneSessionDescription(wrapper_for_pranswer->pc()->remote_description()); - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer || wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(pranswer))); @@ -362,7 +347,7 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) { CreatePeerConnectionInState(SignalingState::kHaveRemoteOffer); auto answer = wrapper_for_answer->CreateAnswer(); - auto wrapper = CreatePeerConnectionUnderTest(); + auto wrapper = CreatePeerConnectionInState(GetParam()); if (wrapper->signaling_state() == SignalingState::kHaveLocalOffer || wrapper->signaling_state() == SignalingState::kHaveRemotePrAnswer) { EXPECT_TRUE(wrapper->SetRemoteDescription(std::move(answer))); @@ -377,35 +362,46 @@ TEST_P(PeerConnectionSignalingStateTest, SetRemoteAnswer) { INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest, PeerConnectionSignalingStateTest, - Combine(Values(SdpSemantics::kPlanB, - SdpSemantics::kUnifiedPlan), - Values(SignalingState::kStable, + Combine(Values(SignalingState::kStable, SignalingState::kHaveLocalOffer, SignalingState::kHaveLocalPrAnswer, SignalingState::kHaveRemoteOffer, SignalingState::kHaveRemotePrAnswer), Bool())); -// Test that CreateAnswer fails if a round of offer/answer has been done and -// the PeerConnection is in the stable state. -TEST_P(PeerConnectionSignalingTest, CreateAnswerFailsIfStable) { +TEST_F(PeerConnectionSignalingTest, + CreateAnswerSucceedsIfStableAndRemoteDescriptionIsOffer) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); - ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); - - ASSERT_EQ(SignalingState::kStable, caller->signaling_state()); - EXPECT_FALSE(caller->CreateAnswer()); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); ASSERT_EQ(SignalingState::kStable, callee->signaling_state()); - EXPECT_FALSE(callee->CreateAnswer()); + 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_EQ(SignalingState::kStable, caller->signaling_state()); + std::string error; + ASSERT_FALSE(caller->CreateAnswer(RTCOfferAnswerOptions(), &error)); + EXPECT_EQ("CreateAnswer called without remote offer.", error); } // According to https://tools.ietf.org/html/rfc3264#section-8, the session id // stays the same but the version must be incremented if a later, different // session description is generated. These two tests verify that is the case for // both offers and answers. -TEST_P(PeerConnectionSignalingTest, +TEST_F(PeerConnectionSignalingTest, SessionVersionIncrementedInSubsequentDifferentOffer) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); @@ -426,14 +422,14 @@ TEST_P(PeerConnectionSignalingTest, EXPECT_LT(rtc::FromString(original_version), rtc::FromString(later_offer->session_version())); } -TEST_P(PeerConnectionSignalingTest, +TEST_F(PeerConnectionSignalingTest, SessionVersionIncrementedInSubsequentDifferentAnswer) { auto caller = CreatePeerConnection(); auto callee = CreatePeerConnection(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - auto original_answer = callee->CreateAnswer(); + auto original_answer = callee->CreateAnswerAndSetAsLocal(); const std::string original_id = original_answer->session_id(); const std::string original_version = original_answer->session_version(); @@ -447,7 +443,7 @@ TEST_P(PeerConnectionSignalingTest, rtc::FromString(later_answer->session_version())); } -TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { +TEST_F(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); @@ -470,7 +466,7 @@ TEST_P(PeerConnectionSignalingTest, InitiatorFlagSetOnCallerAndNotOnCallee) { // PeerConnection and make sure we get success/failure callbacks for all of the // requests. // Background: crbug.com/507307 -TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) { +TEST_F(PeerConnectionSignalingTest, CreateOffersAndShutdown) { auto caller = CreatePeerConnection(); RTCOfferAnswerOptions options; @@ -495,9 +491,4 @@ TEST_P(PeerConnectionSignalingTest, CreateOffersAndShutdown) { } } -INSTANTIATE_TEST_CASE_P(PeerConnectionSignalingTest, - PeerConnectionSignalingTest, - Values(SdpSemantics::kPlanB, - SdpSemantics::kUnifiedPlan)); - } // namespace webrtc