Reland "Set session error if SetLocal/RemoteDescription ever fails"
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}
Bug: chromium:800775
Change-Id: I0016108264e013452e9d34239c012baf23240e99
Reviewed-on: https://webrtc-review.googlesource.com/54720
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22067}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index f04b6e5..73f79c8 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -561,6 +561,17 @@
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
@@ -1733,9 +1744,15 @@
void PeerConnection::SetLocalDescription(
SetSessionDescriptionObserver* observer,
- SessionDescriptionInterface* desc) {
+ SessionDescriptionInterface* desc_ptr) {
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;
@@ -1746,17 +1763,39 @@
return;
}
- SdpType type = desc->GetType();
+ // 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;
+ }
- RTCError error = ApplyLocalDescription(rtc::WrapUnique(desc));
+ 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));
// |desc| may be destroyed at this point.
if (!error.ok()) {
- 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() << ")";
+ // 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;
PostSetSessionDescriptionFailure(observer, std::move(error_message));
return;
}
@@ -1788,11 +1827,6 @@
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);
@@ -1867,7 +1901,7 @@
RemoveUnusedChannels(local_description()->description());
}
- error = UpdateSessionState(type, cricket::CS_LOCAL);
+ RTCError error = UpdateSessionState(type, cricket::CS_LOCAL);
if (!error.ok()) {
return error;
}
@@ -1976,23 +2010,48 @@
return;
}
- const SdpType type = desc->GetType();
+ // 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 = ApplyRemoteDescription(std::move(desc));
- // |desc| may be destroyed at this point.
-
+ RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE);
if (!error.ok()) {
- 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() << ")";
+ 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;
}
- if (remote_description()->GetType() == SdpType::kAnswer) {
+ // 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));
+ // |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;
+ observer->OnSetRemoteDescriptionComplete(
+ RTCError(error.type(), std::move(error_message)));
+ return;
+ }
+ RTC_DCHECK(remote_description());
+
+ if (type == SdpType::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
// MaybeStartGathering...
network_thread()->Invoke<void>(
@@ -2036,11 +2095,6 @@
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);
@@ -2093,7 +2147,7 @@
// NOTE: Candidates allocation will be initiated only when SetLocalDescription
// is called.
- error = UpdateSessionState(type, cricket::CS_REMOTE);
+ RTCError error = UpdateSessionState(type, cricket::CS_REMOTE);
if (!error.ok()) {
return error;
}
@@ -4499,10 +4553,7 @@
// descriptions.
error = PushdownMediaDescription(type, source);
if (!error.ok()) {
- SetSessionError(SessionError::kContent, error.message());
- }
- if (session_error() != SessionError::kNone) {
- LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, GetSessionErrorMsg());
+ return error;
}
return RTCError::OK();
