Revert "Set session error if SetLocal/RemoteDescription ever fails"
This reverts commit 71439a60e7915179be96dd42dc732dc51c279884.
Reason for revert: https://ci.chromium.org/buildbot/chromium.webrtc.fyi/Mac%20Tester/47796
Original change's description:
> Set session error if SetLocal/RemoteDescription ever fails
>
> This changes SetLocalDescription/SetRemoteDescription to set a
> session error which will cause any future calls to fail early if
> there is an error when applying a session description.
>
> This is needed since until better error recovery is implemented
> failing a call to SetLocalDescription or SetRemoteDescription
> could leave the PeerConnection in an inconsistent state.
>
> Bug: chromium:800775
> Change-Id: If06fd73d6e902af15d072dc562bbe830d3b11ad5
> Reviewed-on: https://webrtc-review.googlesource.com/54061
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Commit-Queue: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#22061}
TBR=steveanton@webrtc.org,deadbeef@webrtc.org
Change-Id: I8af271f2b6dd6a896e390a6fe736e809329b4f4a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:800775
Reviewed-on: https://webrtc-review.googlesource.com/54700
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22063}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 15ec950..4f5bd8e 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -561,17 +561,6 @@
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::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<SessionDescriptionInterface> desc(desc_ptr);
-
if (!observer) {
RTC_LOG(LS_ERROR) << "SetLocalDescription - observer is NULL.";
return;
@@ -1765,39 +1748,17 @@
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 @@
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 @@
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 @@
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<void>(
@@ -2097,6 +2038,11 @@
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 @@
// 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 @@
// 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 b2f2469..c6cfb90 100644
--- a/pc/peerconnection_crypto_unittest.cc
+++ b/pc/peerconnection_crypto_unittest.cc
@@ -672,49 +672,6 @@
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 eaccdaf..06dd876 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -482,22 +482,23 @@
// 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 8d1dd76..3a88e80 100644
--- a/pc/peerconnection_media_unittest.cc
+++ b/pc/peerconnection_media_unittest.cc
@@ -918,8 +918,9 @@
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 0dd8f12..c4631cd 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 1d3019b..ee62d35 100644
--- a/rtc_base/gunit.h
+++ b/rtc_base/gunit.h
@@ -163,10 +163,4 @@
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_
