admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "Set session error if SetLocal/RemoteDescription ever fails"

Browse Source 
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}
This commit is contained in:
Steve Anton 2018-02-16 22:27:00 +00:00 committed by Commit Bot
parent a23c69314d
commit b953245311
6 changed files with 43 additions and 154 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

113
pc/peerconnection.cc
View File

@ -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<SessionDescriptionInterface> 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<void>(
@ -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();

43
pc/peerconnection_crypto_unittest.cc
View File

@ -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,

17
pc/peerconnection_ice_unittest.cc
View File

@ -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));

5
pc/peerconnection_media_unittest.cc
View File

@ -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);
}

13
rtc_base/gunit.cc
View File

@ -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 << "\"";
}
}

6
rtc_base/gunit.h
View File

@ -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_