zqz/platform-external-webrtc
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reland "[Perfect Negotiation] Implement non-racy version of SetLocalDescription."

Browse Source 
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}
This commit is contained in:
Henrik Boström
2020-07-29 12:04:00 +02:00
committed by Commit Bot
parent 3872873889
commit 831ae4ef65
10 changed files with 364 additions and 161 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
api/BUILD.gn
View File

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

3
api/DEPS
View File

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

57
api/peer_connection_interface.h
View File

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

15
api/peer_connection_proxy.h
View File

@ -96,19 +96,26 @@ PROXY_METHOD2(void,
CreateAnswer,
CreateSessionDescriptionObserver*,
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,

30
api/set_local_description_observer_interface.h Normal file
View File

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

231
pc/peer_connection.cc
View File

@ -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<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 @@ 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<PeerConnection> pc_;
rtc::scoped_refptr<SetSessionDescriptionObserver>
rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
set_local_description_observer_;
std::function<void()> operation_complete_callback_;
};
@ -833,33 +813,45 @@ class PeerConnection::LocalIceCredentialsToReplace {
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 @@ void PeerConnection::SetLocalDescription(
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(
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::SetLocalDescription(
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 @@ 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,20 +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.
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 @@ void PeerConnection::DoSetLocalDescription(
}
}
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 @@ void PeerConnection::SetRemoteDescription(
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 @@ void PeerConnection::SetRemoteDescription(
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();
});
}

24
pc/peer_connection.h
View File

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

139
pc/peer_connection_signaling_unittest.cc
View File

@ -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<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 @@ 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<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 @@ TEST_P(PeerConnectionSignalingTest,
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 @@ 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<MockSetSessionDescriptionObserver>(),
@ -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();

4
pc/peer_connection_wrapper.cc
View File

@ -166,8 +166,8 @@ bool PeerConnectionWrapper::SetRemoteDescription(
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();

21
pc/test/mock_peer_connection_observers.h
View File

@ -297,7 +297,26 @@ class MockSetSessionDescriptionObserver
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(); }