diff --git a/api/BUILD.gn b/api/BUILD.gn index 0d4ba2ca46..4ca6c9b0e6 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -149,6 +149,7 @@ rtc_library("libjingle_peerconnection_api") { "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 220b30b3cf..4fc132bdc0 100644 --- a/api/DEPS +++ b/api/DEPS @@ -172,6 +172,9 @@ specific_include_rules = { "+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 fd4d2df6a7..52d0d8789a 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 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface { 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 desc, + rtc::scoped_refptr 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 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 desc, rtc::scoped_refptr 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 23887e53da..aa72d7cb10 100644 --- a/api/peer_connection_proxy.h +++ b/api/peer_connection_proxy.h @@ -96,19 +96,26 @@ PROXY_METHOD2(void, CreateAnswer, CreateSessionDescriptionObserver*, const RTCOfferAnswerOptions&) +PROXY_METHOD2(void, + SetLocalDescription, + std::unique_ptr, + rtc::scoped_refptr) +PROXY_METHOD1(void, + SetLocalDescription, + rtc::scoped_refptr) PROXY_METHOD2(void, SetLocalDescription, SetSessionDescriptionObserver*, SessionDescriptionInterface*) PROXY_METHOD1(void, SetLocalDescription, SetSessionDescriptionObserver*) -PROXY_METHOD2(void, - SetRemoteDescription, - SetSessionDescriptionObserver*, - SessionDescriptionInterface*) PROXY_METHOD2(void, SetRemoteDescription, std::unique_ptr, rtc::scoped_refptr) +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 0000000000..90d000cd81 --- /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 1e738a933d..2682f83621 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -708,17 +708,12 @@ bool NeedIceRestart(bool surface_ice_candidates_on_ice_transport_type_changed, // 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 pc, - rtc::scoped_refptr + rtc::scoped_refptr set_local_description_observer) : pc_(std::move(pc)), set_local_description_observer_( @@ -744,42 +739,27 @@ class PeerConnection::ImplicitCreateSessionDescriptionObserver 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 pc_; - rtc::scoped_refptr + rtc::scoped_refptr set_local_description_observer_; std::function operation_complete_callback_; }; @@ -833,33 +813,45 @@ class PeerConnection::LocalIceCredentialsToReplace { std::set> 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 { +// 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 pc, - rtc::scoped_refptr wrapper) - : pc_(std::move(pc)), wrapper_(std::move(wrapper)) {} + SetSessionDescriptionObserverAdapter( + rtc::WeakPtr pc, + rtc::scoped_refptr 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 pc_; - rtc::scoped_refptr 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 pc_; + rtc::scoped_refptr inner_observer_; }; bool PeerConnectionInterface::RTCConfiguration::operator==( @@ -2407,22 +2399,51 @@ void PeerConnection::SetLocalDescription( std::function 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( + new rtc::RefCountedObject( + 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 desc, + rtc::scoped_refptr 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 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( void PeerConnection::SetLocalDescription( SetSessionDescriptionObserver* observer) { RTC_DCHECK_RUN_ON(signaling_thread()); + SetLocalDescription( + new rtc::RefCountedObject( + weak_ptr_factory_.GetWeakPtr(), observer)); +} + +void PeerConnection::SetLocalDescription( + rtc::scoped_refptr 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 create_sdp_observer( new rtc::RefCountedObject( - weak_ptr_factory_.GetWeakPtr(), - rtc::scoped_refptr(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::SetLocalDescription( void PeerConnection::DoSetLocalDescription( std::unique_ptr desc, - rtc::scoped_refptr observer) { + rtc::scoped_refptr observer) { RTC_DCHECK_RUN_ON(signaling_thread()); TRACE_EVENT0("webrtc", "PeerConnection::DoSetLocalDescription"); @@ -2494,8 +2522,7 @@ void PeerConnection::DoSetLocalDescription( } if (!desc) { - PostSetSessionDescriptionFailure( - observer, + observer->OnSetLocalDescriptionComplete( RTCError(RTCErrorType::INTERNAL_ERROR, "SessionDescription is NULL.")); return; } @@ -2505,8 +2532,7 @@ void PeerConnection::DoSetLocalDescription( 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 @@ void PeerConnection::DoSetLocalDescription( // 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 @@ void PeerConnection::DoSetLocalDescription( 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 @@ void PeerConnection::DoSetLocalDescription( 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 @@ void PeerConnection::DoSetLocalDescription( } } + observer->OnSetLocalDescriptionComplete(RTCError::OK()); NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED); } @@ -2881,27 +2899,24 @@ void PeerConnection::SetRemoteDescription( std::function 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( - 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( + 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 @@ void PeerConnection::SetRemoteDescription( std::function 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 2591c4b75f..1cb575244f 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -207,15 +207,29 @@ class PeerConnection : public PeerConnectionInternal, const RTCOfferAnswerOptions& options) override; void CreateAnswer(CreateSessionDescriptionObserver* observer, const RTCOfferAnswerOptions& options) override; + + void SetLocalDescription( + std::unique_ptr desc, + rtc::scoped_refptr observer) + override; + void SetLocalDescription( + rtc::scoped_refptr 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 desc, rtc::scoped_refptr 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 @@ class PeerConnection : public PeerConnectionInternal, 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 @@ class PeerConnection : public PeerConnectionInternal, rtc::scoped_refptr observer); void DoSetLocalDescription( std::unique_ptr desc, - rtc::scoped_refptr observer); + rtc::scoped_refptr observer); void DoSetRemoteDescription( std::unique_ptr desc, rtc::scoped_refptr observer); diff --git a/pc/peer_connection_signaling_unittest.cc b/pc/peer_connection_signaling_unittest.cc index 30b11ceaa7..72cbffbbc5 100644 --- a/pc/peer_connection_signaling_unittest.cc +++ b/pc/peer_connection_signaling_unittest.cc @@ -565,30 +565,102 @@ TEST_P(PeerConnectionSignalingTest, CloseCreateOfferAndShutdown) { 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 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 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 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 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 @@ TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) { // 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 @@ TEST_P(PeerConnectionSignalingTest, CreateOfferBlocksSetRemoteDescription) { // 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 @@ TEST_P(PeerConnectionSignalingTest, 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 @@ TEST_P(PeerConnectionSignalingTest, 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 @@ TEST_P(PeerConnectionSignalingTest, auto callee = CreatePeerConnectionWithAudioVideo(); // SetLocalDescription(), implicitly creating an offer. - rtc::scoped_refptr - caller_set_local_description_observer( - new rtc::RefCountedObject()); - 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 - callee_set_remote_description_observer( - new rtc::RefCountedObject()); + 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 - callee_set_local_description_observer( - new rtc::RefCountedObject()); - 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 @@ TEST_P(PeerConnectionSignalingTest, EXPECT_TRUE(callee->pc()->current_local_description()); // SetRemoteDescription(answer) - rtc::scoped_refptr - caller_set_remote_description_observer( - new rtc::RefCountedObject()); + 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 @@ TEST_P(PeerConnectionSignalingTest, 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 @@ TEST_P(PeerConnectionSignalingTest, 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 @@ class PeerConnectionSignalingUnifiedPlanTest // 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(), @@ -803,6 +874,22 @@ TEST_F(PeerConnectionSignalingUnifiedPlanTest, EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value()); } +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 7c0b3391d0..328f5795e2 100644 --- a/pc/peer_connection_wrapper.cc +++ b/pc/peer_connection_wrapper.cc @@ -166,8 +166,8 @@ bool PeerConnectionWrapper::SetRemoteDescription( bool PeerConnectionWrapper::SetRemoteDescription( std::unique_ptr desc, RTCError* error_out) { - rtc::scoped_refptr observer = - new MockSetRemoteDescriptionObserver(); + rtc::scoped_refptr 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 2017735dc7..0a835713a9 100644 --- a/pc/test/mock_peer_connection_observers.h +++ b/pc/test/mock_peer_connection_observers.h @@ -297,7 +297,26 @@ class MockSetSessionDescriptionObserver std::string error_; }; -class MockSetRemoteDescriptionObserver +class FakeSetLocalDescriptionObserver + : public rtc::RefCountedObject { + 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 error_; +}; + +class FakeSetRemoteDescriptionObserver : public rtc::RefCountedObject { public: bool called() const { return error_.has_value(); }