Reland "[Perfect Negotiation] Implement non-racy version of SetLocalDescription."
This is a reland of d4089cae47334a4228b69d6bb23f2e49ebb7496e
with the following fix:
Invoke MaybeStartGathering as the last step of DoSetLocalDescription.
This ensures that candidates and onicegatheringstatechange does not
happen before SLD is resolved. This is important for passing
external/wpt/webrtc/RTCPeerConnection-iceGatheringState.html.
Original change's description:
> [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}
TBR=hta@webrtc.org
Bug: chromium:1071733
Change-Id: Ic6e8d96afa1c19604762f373716c08dbfa9d178c
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180481
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31804}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 7e7bd1a..728cc4f 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..982e42a 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,20 +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.
- transport_controller_->MaybeStartGathering();
-
if (local_description()->GetType() == SdpType::kAnswer) {
// TODO(deadbeef): We already had to hop to the network thread for
// MaybeStartGathering...
@@ -2587,7 +2599,13 @@
}
}
+ observer->OnSetLocalDescriptionComplete(RTCError::OK());
NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
+
+ // MaybeStartGathering needs to be called after informing the observer so that
+ // we don't signal any candidates before signaling that SetLocalDescription
+ // completed.
+ transport_controller_->MaybeStartGathering();
}
RTCError PeerConnection::ApplyLocalDescription(
@@ -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(); }
