[Perfect Negotiation] Implement non-racy version of SetLocalDescription.
BACKGROUND
When SLD is invoked with SetSessionDescriptionObserver, the observer is
called by posting a message back to the execution thread, delaying the
call. This delay is "artificial" - it's not necessary; the operation is
already complete. It's a post from the signaling thread to the signaling
thread. The rationale for the post was to avoid the observer making
recursive calls back into the PeerConnection. The problem with this is
that by the time the observer is called, the PeerConnection could
already have executed other operations and modified its states.
This causes the referenced bug: one can have a race where SLD is
resolved "too late" (after a pending SRD is executed) and the signaling
state observed when SLD resolves doesn't make sense.
When implementing Unified Plan, we fixed similar issues for SRD by
adding a version that takes SetRemoteDescriptionObserverInterface as
argument instead of SetSessionDescriptionObserver. The new version did
not have the delay. The old version had to be kept around not to break
downstream projects that had dependencies both on he delay and on
allowing the PC to be destroyed midst-operation without informing its
observers.
THIS CL
This does the old SRD fix for SLD as well: A new observer interface is
added, SetLocalDescriptionObserverInterface, and
PeerConnection::SetLocalDescription() is overloaded. If you call it with
the old observer, you get the delay, but if you call it with the new
observer, you don't get a delay.
- SetLocalDescriptionObserverInterface is added.
- SetLocalDescription is overloaded.
- The adapter for SetSessionDescriptionObserver that causes the delay
previously only used for SRD is updated to handle both SLD and SRD.
- FakeSetLocalDescriptionObserver is added and
MockSetRemoteDescriptionObserver is renamed "Fake...".
Bug: chromium:1071733
Change-Id: I920368e648bede481058ac22f5b8794752a220b3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179100
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31798}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 0d4ba2c..4ca6c9b 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -149,6 +149,7 @@
"rtp_transceiver_interface.h",
"sctp_transport_interface.cc",
"sctp_transport_interface.h",
+ "set_local_description_observer_interface.h",
"set_remote_description_observer_interface.h",
"stats_types.cc",
"stats_types.h",
diff --git a/api/DEPS b/api/DEPS
index 220b30b..4fc132b 100644
--- a/api/DEPS
+++ b/api/DEPS
@@ -172,6 +172,9 @@
"+rtc_base/ref_count.h",
],
+ "set_local_description_observer_interface\.h": [
+ "+rtc_base/ref_count.h",
+ ],
"set_remote_description_observer_interface\.h": [
"+rtc_base/ref_count.h",
],
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index fd4d2df..52d0d87 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -97,6 +97,7 @@
#include "api/rtp_sender_interface.h"
#include "api/rtp_transceiver_interface.h"
#include "api/sctp_transport_interface.h"
+#include "api/set_local_description_observer_interface.h"
#include "api/set_remote_description_observer_interface.h"
#include "api/stats/rtc_stats_collector_callback.h"
#include "api/stats_types.h"
@@ -947,26 +948,56 @@
const RTCOfferAnswerOptions& options) = 0;
// Sets the local session description.
- // The PeerConnection takes the ownership of |desc| even if it fails.
- // The |observer| callback will be called when done.
- // TODO(deadbeef): Change |desc| to be a unique_ptr, to make it clear
- // that this method always takes ownership of it.
+ //
+ // According to spec, the local session description MUST be the same as was
+ // returned by CreateOffer() or CreateAnswer() or else the operation should
+ // fail. Our implementation however allows some amount of "SDP munging", but
+ // please note that this is HIGHLY DISCOURAGED. If you do not intent to munge
+ // SDP, the method below that doesn't take |desc| as an argument will create
+ // the offer or answer for you.
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetLocalDescription() method has exited.
+ virtual void SetLocalDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {}
+ // Creates an offer or answer (depending on current signaling state) and sets
+ // it as the local session description.
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetLocalDescription() method has exited.
+ virtual void SetLocalDescription(
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {}
+ // Like SetLocalDescription() above, but the observer is invoked with a delay
+ // after the operation completes. This helps avoid recursive calls by the
+ // observer but also makes it possible for states to change in-between the
+ // operation completing and the observer getting called. This makes them racy
+ // for synchronizing peer connection states to the application.
+ // TODO(https://crbug.com/webrtc/11798): Delete these methods in favor of the
+ // ones taking SetLocalDescriptionObserverInterface as argument.
virtual void SetLocalDescription(SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) = 0;
- // Implicitly creates an offer or answer (depending on the current signaling
- // state) and performs SetLocalDescription() with the newly generated session
- // description.
- // TODO(hbos): Make pure virtual when implemented by downstream projects.
virtual void SetLocalDescription(SetSessionDescriptionObserver* observer) {}
+
// Sets the remote session description.
- // The PeerConnection takes the ownership of |desc| even if it fails.
- // The |observer| callback will be called when done.
- // TODO(hbos): Remove when Chrome implements the new signature.
- virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
- SessionDescriptionInterface* desc) {}
+ //
+ // (Unlike "SDP munging" before SetLocalDescription(), modifying a remote
+ // offer or answer is allowed by the spec.)
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetRemoteDescription() method has exited.
virtual void SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer) = 0;
+ // Like SetRemoteDescription() above, but the observer is invoked with a delay
+ // after the operation completes. This helps avoid recursive calls by the
+ // observer but also makes it possible for states to change in-between the
+ // operation completing and the observer getting called. This makes them racy
+ // for synchronizing peer connection states to the application.
+ // TODO(https://crbug.com/webrtc/11798): Delete this method in favor of the
+ // ones taking SetRemoteDescriptionObserverInterface as argument.
+ virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+ SessionDescriptionInterface* desc) {}
virtual PeerConnectionInterface::RTCConfiguration GetConfiguration() = 0;
diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h
index 23887e5..aa72d7c 100644
--- a/api/peer_connection_proxy.h
+++ b/api/peer_connection_proxy.h
@@ -98,17 +98,24 @@
const RTCOfferAnswerOptions&)
PROXY_METHOD2(void,
SetLocalDescription,
+ std::unique_ptr<SessionDescriptionInterface>,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
+PROXY_METHOD1(void,
+ SetLocalDescription,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
+PROXY_METHOD2(void,
+ SetLocalDescription,
SetSessionDescriptionObserver*,
SessionDescriptionInterface*)
PROXY_METHOD1(void, SetLocalDescription, SetSessionDescriptionObserver*)
PROXY_METHOD2(void,
SetRemoteDescription,
- SetSessionDescriptionObserver*,
- SessionDescriptionInterface*)
-PROXY_METHOD2(void,
- SetRemoteDescription,
std::unique_ptr<SessionDescriptionInterface>,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>)
+PROXY_METHOD2(void,
+ SetRemoteDescription,
+ SetSessionDescriptionObserver*,
+ SessionDescriptionInterface*)
PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration)
PROXY_METHOD1(RTCError,
SetConfiguration,
diff --git a/api/set_local_description_observer_interface.h b/api/set_local_description_observer_interface.h
new file mode 100644
index 0000000..90d000c
--- /dev/null
+++ b/api/set_local_description_observer_interface.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2020 The WebRTC project authors. All Rights Reserved.
+ *
+ * Use of this source code is governed by a BSD-style license
+ * that can be found in the LICENSE file in the root of the source
+ * tree. An additional intellectual property rights grant can be found
+ * in the file PATENTS. All contributing project authors may
+ * be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_
+#define API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_
+
+#include "api/rtc_error.h"
+#include "rtc_base/ref_count.h"
+
+namespace webrtc {
+
+// OnSetLocalDescriptionComplete() invokes as soon as
+// PeerConnectionInterface::SetLocalDescription() operation completes, allowing
+// the observer to examine the effects of the operation without delay.
+class SetLocalDescriptionObserverInterface : public rtc::RefCountInterface {
+ public:
+ // On success, |error.ok()| is true.
+ virtual void OnSetLocalDescriptionComplete(RTCError error) = 0;
+};
+
+} // namespace webrtc
+
+#endif // API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 1e738a9..2682f83 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -708,17 +708,12 @@
// Used by parameterless SetLocalDescription() to create an offer or answer.
// Upon completion of creating the session description, SetLocalDescription() is
// invoked with the result.
-// For consistency with DoSetLocalDescription(), if the PeerConnection is
-// destroyed midst operation, we DO NOT inform the
-// |set_local_description_observer| that the operation failed.
-// TODO(hbos): If/when we process SLD messages in ~PeerConnection, the
-// consistent thing would be to inform the observer here.
class PeerConnection::ImplicitCreateSessionDescriptionObserver
: public CreateSessionDescriptionObserver {
public:
ImplicitCreateSessionDescriptionObserver(
rtc::WeakPtr<PeerConnection> pc,
- rtc::scoped_refptr<SetSessionDescriptionObserver>
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
set_local_description_observer)
: pc_(std::move(pc)),
set_local_description_observer_(
@@ -744,42 +739,27 @@
operation_complete_callback_();
return;
}
- // DoSetLocalDescription() is currently implemented as a synchronous
- // operation but where the |set_local_description_observer_|'s callbacks are
- // invoked asynchronously in a post to PeerConnection::OnMessage().
+ // DoSetLocalDescription() is a synchronous operation that invokes
+ // |set_local_description_observer_| with the result.
pc_->DoSetLocalDescription(std::move(desc),
std::move(set_local_description_observer_));
- // For backwards-compatability reasons, we declare the operation as
- // completed here (rather than in PeerConnection::OnMessage()). This ensures
- // that subsequent offer/answer operations can start immediately (without
- // waiting for OnMessage()).
operation_complete_callback_();
}
void OnFailure(RTCError error) override {
RTC_DCHECK(!was_called_);
was_called_ = true;
-
- // Abort early if |pc_| is no longer valid.
- if (!pc_) {
- operation_complete_callback_();
- return;
- }
- // DoSetLocalDescription() reports its failures in a post. We do the
- // same thing here for consistency.
- pc_->PostSetSessionDescriptionFailure(
- set_local_description_observer_,
- RTCError(error.type(),
- std::string("SetLocalDescription failed to create "
- "session description - ") +
- error.message()));
+ set_local_description_observer_->OnSetLocalDescriptionComplete(RTCError(
+ error.type(), std::string("SetLocalDescription failed to create "
+ "session description - ") +
+ error.message()));
operation_complete_callback_();
}
private:
bool was_called_ = false;
rtc::WeakPtr<PeerConnection> pc_;
- rtc::scoped_refptr<SetSessionDescriptionObserver>
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
set_local_description_observer_;
std::function<void()> operation_complete_callback_;
};
@@ -833,33 +813,45 @@
std::set<std::pair<std::string, std::string>> ice_credentials_;
};
-// Upon completion, posts a task to execute the callback of the
-// SetSessionDescriptionObserver asynchronously on the same thread. At this
-// point, the state of the peer connection might no longer reflect the effects
-// of the SetRemoteDescription operation, as the peer connection could have been
-// modified during the post.
-// TODO(hbos): Remove this class once we remove the version of
-// PeerConnectionInterface::SetRemoteDescription() that takes a
-// SetSessionDescriptionObserver as an argument.
-class PeerConnection::SetRemoteDescriptionObserverAdapter
- : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
+// Wrapper for SetSessionDescriptionObserver that invokes the success or failure
+// callback in a posted message handled by the peer connection. This introduces
+// a delay that prevents recursive API calls by the observer, but this also
+// means that the PeerConnection can be modified before the observer sees the
+// result of the operation. This is ill-advised for synchronizing states.
+//
+// Implements both the SetLocalDescriptionObserverInterface and the
+// SetRemoteDescriptionObserverInterface.
+class PeerConnection::SetSessionDescriptionObserverAdapter
+ : public SetLocalDescriptionObserverInterface,
+ public SetRemoteDescriptionObserverInterface {
public:
- SetRemoteDescriptionObserverAdapter(
- rtc::scoped_refptr<PeerConnection> pc,
- rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
- : pc_(std::move(pc)), wrapper_(std::move(wrapper)) {}
+ SetSessionDescriptionObserverAdapter(
+ rtc::WeakPtr<PeerConnection> pc,
+ rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer)
+ : pc_(std::move(pc)), inner_observer_(std::move(inner_observer)) {}
+ // SetLocalDescriptionObserverInterface implementation.
+ void OnSetLocalDescriptionComplete(RTCError error) override {
+ OnSetDescriptionComplete(std::move(error));
+ }
// SetRemoteDescriptionObserverInterface implementation.
void OnSetRemoteDescriptionComplete(RTCError error) override {
- if (error.ok())
- pc_->PostSetSessionDescriptionSuccess(wrapper_);
- else
- pc_->PostSetSessionDescriptionFailure(wrapper_, std::move(error));
+ OnSetDescriptionComplete(std::move(error));
}
private:
- rtc::scoped_refptr<PeerConnection> pc_;
- rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
+ void OnSetDescriptionComplete(RTCError error) {
+ if (!pc_)
+ return;
+ if (error.ok()) {
+ pc_->PostSetSessionDescriptionSuccess(inner_observer_);
+ } else {
+ pc_->PostSetSessionDescriptionFailure(inner_observer_, std::move(error));
+ }
+ }
+
+ rtc::WeakPtr<PeerConnection> pc_;
+ rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer_;
};
bool PeerConnectionInterface::RTCConfiguration::operator==(
@@ -2407,22 +2399,51 @@
std::function<void()> operations_chain_callback) mutable {
// Abort early if |this_weak_ptr| is no longer valid.
if (!this_weak_ptr) {
- // For consistency with DoSetLocalDescription(), we DO NOT inform the
- // |observer_refptr| that the operation failed in this case.
- // TODO(hbos): If/when we process SLD messages in ~PeerConnection,
- // the consistent thing would be to inform the observer here.
+ // For consistency with SetSessionDescriptionObserverAdapter whose
+ // posted messages doesn't get processed when the PC is destroyed, we
+ // do not inform |observer_refptr| that the operation failed.
operations_chain_callback();
return;
}
- this_weak_ptr->DoSetLocalDescription(std::move(desc),
- std::move(observer_refptr));
- // DoSetLocalDescription() is currently implemented as a synchronous
- // operation but where the |observer|'s callbacks are invoked
- // asynchronously in a post to OnMessage().
+ // SetSessionDescriptionObserverAdapter takes care of making sure the
+ // |observer_refptr| is invoked in a posted message.
+ this_weak_ptr->DoSetLocalDescription(
+ std::move(desc),
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>(
+ new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
+ this_weak_ptr, observer_refptr)));
// For backwards-compatability reasons, we declare the operation as
- // completed here (rather than in OnMessage()). This ensures that
- // subsequent offer/answer operations can start immediately (without
- // waiting for OnMessage()).
+ // completed here (rather than in a post), so that the operation chain
+ // is not blocked by this operation when the observer is invoked. This
+ // allows the observer to trigger subsequent offer/answer operations
+ // synchronously if the operation chain is now empty.
+ operations_chain_callback();
+ });
+}
+
+void PeerConnection::SetLocalDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ // Chain this operation. If asynchronous operations are pending on the chain,
+ // this operation will be queued to be invoked, otherwise the contents of the
+ // lambda will execute immediately.
+ operations_chain_->ChainOperation(
+ [this_weak_ptr = weak_ptr_factory_.GetWeakPtr(), observer,
+ desc = std::move(desc)](
+ std::function<void()> operations_chain_callback) mutable {
+ // Abort early if |this_weak_ptr| is no longer valid.
+ if (!this_weak_ptr) {
+ observer->OnSetLocalDescriptionComplete(RTCError(
+ RTCErrorType::INTERNAL_ERROR,
+ "SetLocalDescription failed because the session was shut down"));
+ operations_chain_callback();
+ return;
+ }
+ this_weak_ptr->DoSetLocalDescription(std::move(desc), observer);
+ // DoSetLocalDescription() is implemented as a synchronous operation.
+ // The |observer| will already have been informed that it completed, and
+ // we can mark this operation as complete without any loose ends.
operations_chain_callback();
});
}
@@ -2430,13 +2451,20 @@
void PeerConnection::SetLocalDescription(
SetSessionDescriptionObserver* observer) {
RTC_DCHECK_RUN_ON(signaling_thread());
+ SetLocalDescription(
+ new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
+ weak_ptr_factory_.GetWeakPtr(), observer));
+}
+
+void PeerConnection::SetLocalDescription(
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
// The |create_sdp_observer| handles performing DoSetLocalDescription() with
// the resulting description as well as completing the operation.
rtc::scoped_refptr<ImplicitCreateSessionDescriptionObserver>
create_sdp_observer(
new rtc::RefCountedObject<ImplicitCreateSessionDescriptionObserver>(
- weak_ptr_factory_.GetWeakPtr(),
- rtc::scoped_refptr<SetSessionDescriptionObserver>(observer)));
+ weak_ptr_factory_.GetWeakPtr(), observer));
// Chain this operation. If asynchronous operations are pending on the chain,
// this operation will be queued to be invoked, otherwise the contents of the
// lambda will execute immediately.
@@ -2484,7 +2512,7 @@
void PeerConnection::DoSetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetSessionDescriptionObserver> observer) {
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
RTC_DCHECK_RUN_ON(signaling_thread());
TRACE_EVENT0("webrtc", "PeerConnection::DoSetLocalDescription");
@@ -2494,8 +2522,7 @@
}
if (!desc) {
- PostSetSessionDescriptionFailure(
- observer,
+ observer->OnSetLocalDescriptionComplete(
RTCError(RTCErrorType::INTERNAL_ERROR, "SessionDescription is NULL."));
return;
}
@@ -2505,8 +2532,7 @@
if (session_error() != SessionError::kNone) {
std::string error_message = GetSessionErrorMsg();
RTC_LOG(LS_ERROR) << "SetLocalDescription: " << error_message;
- PostSetSessionDescriptionFailure(
- observer,
+ observer->OnSetLocalDescriptionComplete(
RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
return;
}
@@ -2514,16 +2540,11 @@
// For SLD we support only explicit rollback.
if (desc->GetType() == SdpType::kRollback) {
if (IsUnifiedPlan()) {
- RTCError error = Rollback(desc->GetType());
- if (error.ok()) {
- PostSetSessionDescriptionSuccess(observer);
- } else {
- PostSetSessionDescriptionFailure(observer, std::move(error));
- }
+ observer->OnSetLocalDescriptionComplete(Rollback(desc->GetType()));
} else {
- PostSetSessionDescriptionFailure(
- observer, RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
- "Rollback not supported in Plan B"));
+ observer->OnSetLocalDescriptionComplete(
+ RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+ "Rollback not supported in Plan B"));
}
return;
}
@@ -2533,8 +2554,7 @@
std::string error_message = GetSetDescriptionErrorMessage(
cricket::CS_LOCAL, desc->GetType(), error);
RTC_LOG(LS_ERROR) << error_message;
- PostSetSessionDescriptionFailure(
- observer,
+ observer->OnSetLocalDescriptionComplete(
RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
return;
}
@@ -2554,15 +2574,12 @@
std::string error_message =
GetSetDescriptionErrorMessage(cricket::CS_LOCAL, type, error);
RTC_LOG(LS_ERROR) << error_message;
- PostSetSessionDescriptionFailure(
- observer,
+ observer->OnSetLocalDescriptionComplete(
RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
return;
}
RTC_DCHECK(local_description());
- PostSetSessionDescriptionSuccess(observer);
-
// MaybeStartGathering needs to be called after posting
// MSG_SET_SESSIONDESCRIPTION_SUCCESS, so that we don't signal any candidates
// before signaling that SetLocalDescription completed.
@@ -2587,6 +2604,7 @@
}
}
+ observer->OnSetLocalDescriptionComplete(RTCError::OK());
NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
}
@@ -2881,27 +2899,24 @@
std::function<void()> operations_chain_callback) mutable {
// Abort early if |this_weak_ptr| is no longer valid.
if (!this_weak_ptr) {
- // For consistency with SetRemoteDescriptionObserverAdapter, we DO NOT
- // inform the |observer_refptr| that the operation failed in this
- // case.
- // TODO(hbos): If/when we process SRD messages in ~PeerConnection,
- // the consistent thing would be to inform the observer here.
+ // For consistency with SetSessionDescriptionObserverAdapter whose
+ // posted messages doesn't get processed when the PC is destroyed, we
+ // do not inform |observer_refptr| that the operation failed.
operations_chain_callback();
return;
}
+ // SetSessionDescriptionObserverAdapter takes care of making sure the
+ // |observer_refptr| is invoked in a posted message.
this_weak_ptr->DoSetRemoteDescription(
std::move(desc),
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>(
- new SetRemoteDescriptionObserverAdapter(
- this_weak_ptr.get(), std::move(observer_refptr))));
- // DoSetRemoteDescription() is currently implemented as a synchronous
- // operation but where SetRemoteDescriptionObserverAdapter ensures that
- // the |observer|'s callbacks are invoked asynchronously in a post to
- // OnMessage().
+ new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
+ this_weak_ptr, observer_refptr)));
// For backwards-compatability reasons, we declare the operation as
- // completed here (rather than in OnMessage()). This ensures that
- // subsequent offer/answer operations can start immediately (without
- // waiting for OnMessage()).
+ // completed here (rather than in a post), so that the operation chain
+ // is not blocked by this operation when the observer is invoked. This
+ // allows the observer to trigger subsequent offer/answer operations
+ // synchronously if the operation chain is now empty.
operations_chain_callback();
});
}
@@ -2919,21 +2934,17 @@
std::function<void()> operations_chain_callback) mutable {
// Abort early if |this_weak_ptr| is no longer valid.
if (!this_weak_ptr) {
- // For consistency with DoSetRemoteDescription(), we DO inform the
- // |observer| that the operation failed in this case.
observer->OnSetRemoteDescriptionComplete(RTCError(
- RTCErrorType::INVALID_STATE,
- "Failed to set remote offer sdp: failed because the session was "
- "shut down"));
+ RTCErrorType::INTERNAL_ERROR,
+ "SetRemoteDescription failed because the session was shut down"));
operations_chain_callback();
return;
}
this_weak_ptr->DoSetRemoteDescription(std::move(desc),
std::move(observer));
- // DoSetRemoteDescription() is currently implemented as a synchronous
- // operation. The |observer| will already have been informed that it
- // completed, and we can mark this operation as complete without any
- // loose ends.
+ // DoSetRemoteDescription() is implemented as a synchronous operation.
+ // The |observer| will already have been informed that it completed, and
+ // we can mark this operation as complete without any loose ends.
operations_chain_callback();
});
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 2591c4b..1cb5752 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -207,15 +207,29 @@
const RTCOfferAnswerOptions& options) override;
void CreateAnswer(CreateSessionDescriptionObserver* observer,
const RTCOfferAnswerOptions& options) override;
+
+ void SetLocalDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer)
+ override;
+ void SetLocalDescription(
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer)
+ override;
+ // TODO(https://crbug.com/webrtc/11798): Delete these methods in favor of the
+ // ones taking SetLocalDescriptionObserverInterface as argument.
void SetLocalDescription(SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) override;
void SetLocalDescription(SetSessionDescriptionObserver* observer) override;
- void SetRemoteDescription(SetSessionDescriptionObserver* observer,
- SessionDescriptionInterface* desc) override;
+
void SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer)
override;
+ // TODO(https://crbug.com/webrtc/11798): Delete this methods in favor of the
+ // ones taking SetRemoteDescriptionObserverInterface as argument.
+ void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+ SessionDescriptionInterface* desc) override;
+
PeerConnectionInterface::RTCConfiguration GetConfiguration() override;
RTCError SetConfiguration(
const PeerConnectionInterface::RTCConfiguration& configuration) override;
@@ -333,8 +347,8 @@
private:
class ImplicitCreateSessionDescriptionObserver;
friend class ImplicitCreateSessionDescriptionObserver;
- class SetRemoteDescriptionObserverAdapter;
- friend class SetRemoteDescriptionObserverAdapter;
+ class SetSessionDescriptionObserverAdapter;
+ friend class SetSessionDescriptionObserverAdapter;
// Represents the [[LocalIceCredentialsToReplace]] internal slot in the spec.
// It makes the next CreateOffer() produce new ICE credentials even if
@@ -428,7 +442,7 @@
rtc::scoped_refptr<CreateSessionDescriptionObserver> observer);
void DoSetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetSessionDescriptionObserver> observer);
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer);
void DoSetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer);
diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc
index 30b11ce..72cbffb 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -565,30 +565,102 @@
EXPECT_TRUE(observer->called());
}
-TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+TEST_P(PeerConnectionSignalingTest,
+ ImplicitCreateOfferAndShutdownWithOldObserver) {
auto caller = CreatePeerConnection();
auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
+ EXPECT_FALSE(observer->called());
+}
+
+TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+ auto caller = CreatePeerConnection();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->SetLocalDescription(observer);
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ CloseBeforeImplicitCreateOfferAndShutdownWithOldObserver) {
+ auto caller = CreatePeerConnection();
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->Close();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
EXPECT_FALSE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, CloseBeforeImplicitCreateOfferAndShutdown) {
auto caller = CreatePeerConnection();
- auto observer = MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->Close();
caller->pc()->SetLocalDescription(observer);
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ CloseAfterImplicitCreateOfferAndShutdownWithOldObserver) {
+ auto caller = CreatePeerConnection();
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer.get());
+ caller->pc()->Close();
+ caller.reset(nullptr);
+ // The old observer does not get invoked because posted messages are lost.
EXPECT_FALSE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, CloseAfterImplicitCreateOfferAndShutdown) {
auto caller = CreatePeerConnection();
- auto observer = MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
caller->pc()->SetLocalDescription(observer);
caller->pc()->Close();
caller.reset(nullptr);
+ // The new observer gets invoked because it is called immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ SetLocalDescriptionNewObserverIsInvokedImmediately) {
+ auto caller = CreatePeerConnection();
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+ new FakeSetLocalDescriptionObserver());
+ caller->pc()->SetLocalDescription(std::move(offer), observer);
+ // The new observer is invoked immediately.
+ EXPECT_TRUE(observer->called());
+ EXPECT_TRUE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+ SetLocalDescriptionOldObserverIsInvokedInAPostedMessage) {
+ auto caller = CreatePeerConnection();
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ auto observer = MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(observer, offer.release());
+ // The old observer is not invoked immediately.
EXPECT_FALSE(observer->called());
+ // Process all currently pending messages by waiting for a posted task to run.
+ bool checkpoint_reached = false;
+ rtc::Thread::Current()->PostTask(
+ RTC_FROM_HERE, [&checkpoint_reached] { checkpoint_reached = true; });
+ EXPECT_TRUE_WAIT(checkpoint_reached, kWaitTimeout);
+ // If resolving the observer was pending, it must now have been called.
+ EXPECT_TRUE(observer->called());
}
TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) {
@@ -601,7 +673,7 @@
// By not waiting for the observer's callback we can verify that the operation
// executed immediately.
callee->pc()->SetRemoteDescription(std::move(offer),
- new MockSetRemoteDescriptionObserver());
+ new FakeSetRemoteDescriptionObserver());
EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
}
@@ -620,7 +692,7 @@
// asynchronously, when CreateOffer() completes.
callee->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
callee->pc()->SetRemoteDescription(std::move(offer),
- new MockSetRemoteDescriptionObserver());
+ new FakeSetRemoteDescriptionObserver());
// CreateOffer() is asynchronous; without message processing this operation
// should not have completed.
EXPECT_FALSE(offer_observer->called());
@@ -639,7 +711,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
// The offer is created asynchronously; message processing is needed for it to
// complete.
@@ -665,7 +737,7 @@
EXPECT_EQ(PeerConnection::kHaveRemoteOffer, callee->signaling_state());
auto observer = MockSetSessionDescriptionObserver::Create();
- callee->pc()->SetLocalDescription(observer);
+ callee->pc()->SetLocalDescription(observer.get());
// The answer is created asynchronously; message processing is needed for it
// to complete.
@@ -687,28 +759,27 @@
auto callee = CreatePeerConnectionWithAudioVideo();
// SetLocalDescription(), implicitly creating an offer.
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- caller_set_local_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
- caller->pc()->SetLocalDescription(caller_set_local_description_observer);
+ auto caller_set_local_description_observer =
+ MockSetSessionDescriptionObserver::Create();
+ caller->pc()->SetLocalDescription(
+ caller_set_local_description_observer.get());
EXPECT_TRUE_WAIT(caller_set_local_description_observer->called(),
kWaitTimeout);
ASSERT_TRUE(caller->pc()->pending_local_description());
// SetRemoteDescription(offer)
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- callee_set_remote_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ auto callee_set_remote_description_observer =
+ MockSetSessionDescriptionObserver::Create();
callee->pc()->SetRemoteDescription(
- callee_set_remote_description_observer.get(),
+ callee_set_remote_description_observer,
CloneSessionDescription(caller->pc()->pending_local_description())
.release());
// SetLocalDescription(), implicitly creating an answer.
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- callee_set_local_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
- callee->pc()->SetLocalDescription(callee_set_local_description_observer);
+ auto callee_set_local_description_observer =
+ MockSetSessionDescriptionObserver::Create();
+ callee->pc()->SetLocalDescription(
+ callee_set_local_description_observer.get());
EXPECT_TRUE_WAIT(callee_set_local_description_observer->called(),
kWaitTimeout);
// Chaining guarantees SetRemoteDescription() happened before
@@ -717,9 +788,8 @@
EXPECT_TRUE(callee->pc()->current_local_description());
// SetRemoteDescription(answer)
- rtc::scoped_refptr<MockSetSessionDescriptionObserver>
- caller_set_remote_description_observer(
- new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ auto caller_set_remote_description_observer =
+ MockSetSessionDescriptionObserver::Create();
caller->pc()->SetRemoteDescription(
caller_set_remote_description_observer,
CloneSessionDescription(callee->pc()->current_local_description())
@@ -737,7 +807,7 @@
auto observer = MockSetSessionDescriptionObserver::Create();
caller->pc()->Close();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
// The operation should fail asynchronously.
EXPECT_FALSE(observer->called());
@@ -756,7 +826,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer);
+ caller->pc()->SetLocalDescription(observer.get());
caller->pc()->Close();
// The operation should fail asynchronously.
@@ -788,14 +858,15 @@
// unique to Unified Plan, but the transceivers used to verify this are only
// available in Unified Plan.
TEST_F(PeerConnectionSignalingUnifiedPlanTest,
- SetLocalDescriptionExecutesImmediately) {
+ SetLocalDescriptionExecutesImmediatelyUsingOldObserver) {
auto caller = CreatePeerConnectionWithAudioVideo();
// This offer will cause transceiver mids to get assigned.
auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
// By not waiting for the observer's callback we can verify that the operation
- // executed immediately.
+ // executed immediately. The old observer is invoked in a posted message, so
+ // waiting for it would not ensure synchronicity.
RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
caller->pc()->SetLocalDescription(
new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
@@ -804,6 +875,22 @@
}
TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+ SetLocalDescriptionExecutesImmediatelyUsingNewObserver) {
+ auto caller = CreatePeerConnectionWithAudioVideo();
+
+ // This offer will cause transceiver mids to get assigned.
+ auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+ // Verify that mids were assigned without waiting for the observer. (However,
+ // the new observer should also be invoked synchronously - as is ensured by
+ // other tests.)
+ RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
+ caller->pc()->SetLocalDescription(std::move(offer),
+ new FakeSetLocalDescriptionObserver());
+ EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value());
+}
+
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
SetLocalDescriptionExecutesImmediatelyInsideCreateOfferCallback) {
auto caller = CreatePeerConnectionWithAudioVideo();
diff --git a/pc/peer_connection_wrapper.cc b/pc/peer_connection_wrapper.cc
index 7c0b339..328f579 100644
--- a/pc/peer_connection_wrapper.cc
+++ b/pc/peer_connection_wrapper.cc
@@ -166,8 +166,8 @@
bool PeerConnectionWrapper::SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
RTCError* error_out) {
- rtc::scoped_refptr<MockSetRemoteDescriptionObserver> observer =
- new MockSetRemoteDescriptionObserver();
+ rtc::scoped_refptr<FakeSetRemoteDescriptionObserver> observer =
+ new FakeSetRemoteDescriptionObserver();
pc()->SetRemoteDescription(std::move(desc), observer);
EXPECT_EQ_WAIT(true, observer->called(), kDefaultTimeout);
bool ok = observer->error().ok();
diff --git a/pc/test/mock_peer_connection_observers.h b/pc/test/mock_peer_connection_observers.h
index 2017735..0a83571 100644
--- a/pc/test/mock_peer_connection_observers.h
+++ b/pc/test/mock_peer_connection_observers.h
@@ -297,7 +297,26 @@
std::string error_;
};
-class MockSetRemoteDescriptionObserver
+class FakeSetLocalDescriptionObserver
+ : public rtc::RefCountedObject<SetLocalDescriptionObserverInterface> {
+ public:
+ bool called() const { return error_.has_value(); }
+ RTCError& error() {
+ RTC_DCHECK(error_.has_value());
+ return *error_;
+ }
+
+ // SetLocalDescriptionObserverInterface implementation.
+ void OnSetLocalDescriptionComplete(RTCError error) override {
+ error_ = std::move(error);
+ }
+
+ private:
+ // Set on complete, on success this is set to an RTCError::OK() error.
+ absl::optional<RTCError> error_;
+};
+
+class FakeSetRemoteDescriptionObserver
: public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
public:
bool called() const { return error_.has_value(); }