diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 15ec9509a0..4f5bd8e75f 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -561,17 +561,6 @@ bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, return false; } -// Generates a string error message for SetLocalDescription/SetRemoteDescription -// from an RTCError. -std::string GetSetDescriptionErrorMessage(cricket::ContentSource source, - SdpType type, - const RTCError& error) { - std::ostringstream oss; - oss << "Failed to set " << (source == cricket::CS_LOCAL ? "local" : "remote") - << " " << SdpTypeToString(type) << " sdp: " << error.message(); - return oss.str(); -} - } // namespace // Upon completion, posts a task to execute the callback of the @@ -1746,15 +1735,9 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer, void PeerConnection::SetLocalDescription( SetSessionDescriptionObserver* observer, - SessionDescriptionInterface* desc_ptr) { + SessionDescriptionInterface* desc) { TRACE_EVENT0("webrtc", "PeerConnection::SetLocalDescription"); - // The SetLocalDescription contract is that we take ownership of the session - // description regardless of the outcome, so wrap it in a unique_ptr right - // away. Ideally, SetLocalDescription's signature will be changed to take the - // description as a unique_ptr argument to formalize this agreement. - std::unique_ptr desc(desc_ptr); - if (!observer) { RTC_LOG(LS_ERROR) << "SetLocalDescription - observer is NULL."; return; @@ -1765,39 +1748,17 @@ void PeerConnection::SetLocalDescription( return; } - // If a session error has occurred the PeerConnection is in a possibly - // inconsistent state so fail right away. - if (session_error() != SessionError::kNone) { - std::string error_message = GetSessionErrorMsg(); - RTC_LOG(LS_ERROR) << "SetLocalDescription: " << error_message; - PostSetSessionDescriptionFailure(observer, std::move(error_message)); - return; - } + SdpType type = desc->GetType(); - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL); - if (!error.ok()) { - std::string error_message = GetSetDescriptionErrorMessage( - cricket::CS_LOCAL, desc->GetType(), error); - RTC_LOG(LS_ERROR) << error_message; - PostSetSessionDescriptionFailure(observer, std::move(error_message)); - return; - } - - // Grab the description type before moving ownership to ApplyLocalDescription, - // which may destroy it before returning. - const SdpType type = desc->GetType(); - - error = ApplyLocalDescription(std::move(desc)); + RTCError error = ApplyLocalDescription(rtc::WrapUnique(desc)); // |desc| may be destroyed at this point. if (!error.ok()) { - // If ApplyLocalDescription fails, the PeerConnection could be in an - // inconsistent state, so act conservatively here and set the session error - // so that future calls to SetLocalDescription/SetRemoteDescription fail. - SetSessionError(SessionError::kContent, error.message()); - std::string error_message = - GetSetDescriptionErrorMessage(cricket::CS_LOCAL, type, error); - RTC_LOG(LS_ERROR) << error_message; + std::ostringstream oss; + oss << "Failed to set local " << SdpTypeToString(type) + << " sdp: " << error.message(); + std::string error_message = oss.str(); + RTC_LOG(LS_ERROR) << error_message << " (" << error.type() << ")"; PostSetSessionDescriptionFailure(observer, std::move(error_message)); return; } @@ -1829,6 +1790,11 @@ RTCError PeerConnection::ApplyLocalDescription( RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL); + if (!error.ok()) { + return error; + } + // Update stats here so that we have the most recent stats for tracks and // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); @@ -1903,7 +1869,7 @@ RTCError PeerConnection::ApplyLocalDescription( RemoveUnusedChannels(local_description()->description()); } - RTCError error = UpdateSessionState(type, cricket::CS_LOCAL); + error = UpdateSessionState(type, cricket::CS_LOCAL); if (!error.ok()) { return error; } @@ -2012,48 +1978,23 @@ void PeerConnection::SetRemoteDescription( return; } - // If a session error has occurred the PeerConnection is in a possibly - // inconsistent state so fail right away. - if (session_error() != SessionError::kNone) { - std::string error_message = GetSessionErrorMsg(); - RTC_LOG(LS_ERROR) << "SetRemoteDescription: " << error_message; - observer->OnSetRemoteDescriptionComplete( - RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message))); - return; - } - - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE); - if (!error.ok()) { - std::string error_message = GetSetDescriptionErrorMessage( - cricket::CS_REMOTE, desc->GetType(), error); - RTC_LOG(LS_ERROR) << error_message; - observer->OnSetRemoteDescriptionComplete( - RTCError(error.type(), std::move(error_message))); - return; - } - - // Grab the description type before moving ownership to - // ApplyRemoteDescription, which may destroy it before returning. const SdpType type = desc->GetType(); - error = ApplyRemoteDescription(std::move(desc)); + RTCError error = ApplyRemoteDescription(std::move(desc)); // |desc| may be destroyed at this point. if (!error.ok()) { - // If ApplyRemoteDescription fails, the PeerConnection could be in an - // inconsistent state, so act conservatively here and set the session error - // so that future calls to SetLocalDescription/SetRemoteDescription fail. - SetSessionError(SessionError::kContent, error.message()); - std::string error_message = - GetSetDescriptionErrorMessage(cricket::CS_REMOTE, type, error); - RTC_LOG(LS_ERROR) << error_message; + std::ostringstream oss; + oss << "Failed to set remote " << SdpTypeToString(type) + << " sdp: " << error.message(); + std::string error_message = oss.str(); + RTC_LOG(LS_ERROR) << error_message << " (" << error.type() << ")"; observer->OnSetRemoteDescriptionComplete( RTCError(error.type(), std::move(error_message))); return; } - RTC_DCHECK(remote_description()); - if (type == SdpType::kAnswer) { + if (remote_description()->GetType() == SdpType::kAnswer) { // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... network_thread()->Invoke( @@ -2097,6 +2038,11 @@ RTCError PeerConnection::ApplyRemoteDescription( RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE); + if (!error.ok()) { + return error; + } + // Update stats here so that we have the most recent stats for tracks and // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); @@ -2149,7 +2095,7 @@ RTCError PeerConnection::ApplyRemoteDescription( // NOTE: Candidates allocation will be initiated only when SetLocalDescription // is called. - RTCError error = UpdateSessionState(type, cricket::CS_REMOTE); + error = UpdateSessionState(type, cricket::CS_REMOTE); if (!error.ok()) { return error; } @@ -4557,7 +4503,10 @@ RTCError PeerConnection::UpdateSessionState(SdpType type, // descriptions. error = PushdownMediaDescription(type, source); if (!error.ok()) { - return error; + SetSessionError(SessionError::kContent, error.message()); + } + if (session_error() != SessionError::kNone) { + LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, GetSessionErrorMsg()); } return RTCError::OK(); diff --git a/pc/peerconnection_crypto_unittest.cc b/pc/peerconnection_crypto_unittest.cc index b2f24692bf..c6cfb90204 100644 --- a/pc/peerconnection_crypto_unittest.cc +++ b/pc/peerconnection_crypto_unittest.cc @@ -672,49 +672,6 @@ TEST_P(PeerConnectionCryptoTest, CreateAnswerWithDifferentSslRoles) { ASSERT_TRUE(callee->SetRemoteDescription(std::move(answer))); } -// Tests that if the DTLS fingerprint is invalid then all future calls to -// SetLocalDescription and SetRemoteDescription will fail due to a session -// error. -// This is a regression test for crbug.com/800775 -TEST_P(PeerConnectionCryptoTest, SessionErrorIfFingerprintInvalid) { - auto callee_certificate = rtc::RTCCertificate::FromPEM(kRsaPems[0]); - auto other_certificate = rtc::RTCCertificate::FromPEM(kRsaPems[1]); - - auto caller = CreatePeerConnectionWithAudioVideo(); - RTCConfiguration callee_config; - callee_config.enable_dtls_srtp.emplace(true); - callee_config.certificates.push_back(callee_certificate); - auto callee = CreatePeerConnectionWithAudioVideo(callee_config); - - ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - - // Create an invalid answer with the other certificate's fingerprint. - auto invalid_answer = callee->CreateAnswer(); - auto* audio_content = - cricket::GetFirstAudioContent(invalid_answer->description()); - ASSERT_TRUE(audio_content); - auto* audio_transport_info = - invalid_answer->description()->GetTransportInfoByName( - audio_content->name); - ASSERT_TRUE(audio_transport_info); - audio_transport_info->description.identity_fingerprint.reset( - rtc::SSLFingerprint::CreateFromCertificate(other_certificate)); - - // Set the invalid answer and expect a fingerprint error. - std::string error; - ASSERT_FALSE(callee->SetLocalDescription(std::move(invalid_answer), &error)); - EXPECT_PRED_FORMAT2(AssertStringContains, error, - "Local fingerprint does not match identity."); - - // Make sure that setting a valid remote offer or local answer also fails now. - ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer(), &error)); - EXPECT_PRED_FORMAT2(AssertStringContains, error, - "Session error code: ERROR_CONTENT."); - ASSERT_FALSE(callee->SetLocalDescription(callee->CreateAnswer(), &error)); - EXPECT_PRED_FORMAT2(AssertStringContains, error, - "Session error code: ERROR_CONTENT."); -} - INSTANTIATE_TEST_CASE_P(PeerConnectionCryptoTest, PeerConnectionCryptoTest, Values(SdpSemantics::kPlanB, diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index eaccdaf54f..06dd876dd7 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -482,22 +482,23 @@ TEST_P(PeerConnectionIceTest, // The standard (https://tools.ietf.org/html/rfc5245#section-15.4) says that // pwd must be 22-256 characters and ufrag must be 4-256 characters. TEST_P(PeerConnectionIceTest, VerifyUfragPwdLength) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + auto set_local_description_with_ufrag_pwd_length = - [this](int ufrag_len, int pwd_len) { - auto pc = CreatePeerConnectionWithAudioVideo(); - auto offer = pc->CreateOffer(); + [this, &caller](int ufrag_len, int pwd_len) { + auto offer = caller->CreateOffer(); SetIceUfragPwd(offer.get(), std::string(ufrag_len, 'x'), std::string(pwd_len, 'x')); - return pc->SetLocalDescription(std::move(offer)); + return caller->SetLocalDescription(std::move(offer)); }; auto set_remote_description_with_ufrag_pwd_length = - [this](int ufrag_len, int pwd_len) { - auto pc = CreatePeerConnectionWithAudioVideo(); - auto offer = pc->CreateOffer(); + [this, &caller, &callee](int ufrag_len, int pwd_len) { + auto offer = caller->CreateOffer(); SetIceUfragPwd(offer.get(), std::string(ufrag_len, 'x'), std::string(pwd_len, 'x')); - return pc->SetRemoteDescription(std::move(offer)); + return callee->SetRemoteDescription(std::move(offer)); }; EXPECT_FALSE(set_local_description_with_ufrag_pwd_length(3, 22)); diff --git a/pc/peerconnection_media_unittest.cc b/pc/peerconnection_media_unittest.cc index 8d1dd7694f..3a88e80007 100644 --- a/pc/peerconnection_media_unittest.cc +++ b/pc/peerconnection_media_unittest.cc @@ -918,8 +918,9 @@ TEST_P(PeerConnectionMediaTest, MediaEngineErrorPropagatedToClients) { ASSERT_FALSE(caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal(), &error)); EXPECT_EQ( - "Failed to set remote answer sdp: Failed to set remote video description " - "send parameters.", + "Failed to set remote answer sdp: Session error code: ERROR_CONTENT. " + "Session error description: Failed to set remote video description send " + "parameters..", error); } diff --git a/rtc_base/gunit.cc b/rtc_base/gunit.cc index 0dd8f1230d..c4631cdcdc 100644 --- a/rtc_base/gunit.cc +++ b/rtc_base/gunit.cc @@ -26,16 +26,3 @@ << prefix_expr << "\nwhich is\n\"" << prefix << "\""; } } - -::testing::AssertionResult AssertStringContains(const char* str_expr, - const char* substr_expr, - const std::string& str, - const std::string& substr) { - if (str.find(substr) != std::string::npos) { - return ::testing::AssertionSuccess(); - } else { - return ::testing::AssertionFailure() - << str_expr << "\nwhich is\n\"" << str << "\"\ndoes not contain\n" - << substr_expr << "\nwhich is\n\"" << substr << "\""; - } -} diff --git a/rtc_base/gunit.h b/rtc_base/gunit.h index 1d3019b58c..ee62d35dcc 100644 --- a/rtc_base/gunit.h +++ b/rtc_base/gunit.h @@ -163,10 +163,4 @@ testing::AssertionResult AssertStartsWith(const char* str_expr, const std::string& str, const std::string& prefix); -// Usage: EXPECT_PRED_FORMAT2(AssertStringContains, str, "substring"); -testing::AssertionResult AssertStringContains(const char* str_expr, - const char* substr_expr, - const std::string& str, - const std::string& substr); - #endif // RTC_BASE_GUNIT_H_