Revert "[Perfect Negotiation] Implement non-racy version of SetLocalDescription."
This reverts commit d4089cae47334a4228b69d6bb23f2e49ebb7496e.
Reason for revert: Breaks chromium WPT that is timing sensitive to onicegatheringstatechanges.
This CL accidentally moved the MaybeStartGatheringIceCandidates to after completing the SLD call. The fix is to move it back. I'll do that in a re-land.
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=hbos@webrtc.org,hta@webrtc.org
Change-Id: Ie1e1ecc49f3b1d7a7e230db6d36decbc4cbe8c86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:1071733
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/180480
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31802}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 728cc4f..7e7bd1a 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -149,7 +149,6 @@
"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 4fc132b..220b30b 100644
--- a/api/DEPS
+++ b/api/DEPS
@@ -172,9 +172,6 @@
"+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 52d0d87..fd4d2df 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -97,7 +97,6 @@
#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"
@@ -948,56 +947,26 @@
const RTCOfferAnswerOptions& options) = 0;
// Sets the local session description.
- //
- // 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.
+ // 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.
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.
- //
- // (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.
+ // 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) {}
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 aa72d7c..23887e5 100644
--- a/api/peer_connection_proxy.h
+++ b/api/peer_connection_proxy.h
@@ -98,24 +98,17 @@
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,
- std::unique_ptr<SessionDescriptionInterface>,
- rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>)
-PROXY_METHOD2(void,
- SetRemoteDescription,
SetSessionDescriptionObserver*,
SessionDescriptionInterface*)
+PROXY_METHOD2(void,
+ SetRemoteDescription,
+ std::unique_ptr<SessionDescriptionInterface>,
+ rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>)
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
deleted file mode 100644
index 90d000c..0000000
--- a/api/set_local_description_observer_interface.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * 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 2682f83..1e738a9 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -708,12 +708,17 @@
// 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<SetLocalDescriptionObserverInterface>
+ rtc::scoped_refptr<SetSessionDescriptionObserver>
set_local_description_observer)
: pc_(std::move(pc)),
set_local_description_observer_(
@@ -739,27 +744,42 @@
operation_complete_callback_();
return;
}
- // DoSetLocalDescription() is a synchronous operation that invokes
- // |set_local_description_observer_| with the result.
+ // 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().
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;
- set_local_description_observer_->OnSetLocalDescriptionComplete(RTCError(
- error.type(), std::string("SetLocalDescription failed to create "
- "session description - ") +
- error.message()));
+
+ // 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()));
operation_complete_callback_();
}
private:
bool was_called_ = false;
rtc::WeakPtr<PeerConnection> pc_;
- rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
+ rtc::scoped_refptr<SetSessionDescriptionObserver>
set_local_description_observer_;
std::function<void()> operation_complete_callback_;
};
@@ -813,45 +833,33 @@
std::set<std::pair<std::string, std::string>> ice_credentials_;
};
-// 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 {
+// 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> {
public:
- SetSessionDescriptionObserverAdapter(
- rtc::WeakPtr<PeerConnection> pc,
- rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer)
- : pc_(std::move(pc)), inner_observer_(std::move(inner_observer)) {}
+ SetRemoteDescriptionObserverAdapter(
+ rtc::scoped_refptr<PeerConnection> pc,
+ rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
+ : pc_(std::move(pc)), wrapper_(std::move(wrapper)) {}
- // SetLocalDescriptionObserverInterface implementation.
- void OnSetLocalDescriptionComplete(RTCError error) override {
- OnSetDescriptionComplete(std::move(error));
- }
// SetRemoteDescriptionObserverInterface implementation.
void OnSetRemoteDescriptionComplete(RTCError error) override {
- OnSetDescriptionComplete(std::move(error));
+ if (error.ok())
+ pc_->PostSetSessionDescriptionSuccess(wrapper_);
+ else
+ pc_->PostSetSessionDescriptionFailure(wrapper_, std::move(error));
}
private:
- 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_;
+ rtc::scoped_refptr<PeerConnection> pc_;
+ rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
};
bool PeerConnectionInterface::RTCConfiguration::operator==(
@@ -2399,51 +2407,22 @@
std::function<void()> operations_chain_callback) mutable {
// Abort early if |this_weak_ptr| is no longer valid.
if (!this_weak_ptr) {
- // 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.
+ // 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.
operations_chain_callback();
return;
}
- // 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)));
+ 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().
// For backwards-compatability reasons, we declare the operation as
- // 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.
+ // completed here (rather than in OnMessage()). This ensures that
+ // subsequent offer/answer operations can start immediately (without
+ // waiting for OnMessage()).
operations_chain_callback();
});
}
@@ -2451,20 +2430,13 @@
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(), observer));
+ weak_ptr_factory_.GetWeakPtr(),
+ rtc::scoped_refptr<SetSessionDescriptionObserver>(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.
@@ -2512,7 +2484,7 @@
void PeerConnection::DoSetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
+ rtc::scoped_refptr<SetSessionDescriptionObserver> observer) {
RTC_DCHECK_RUN_ON(signaling_thread());
TRACE_EVENT0("webrtc", "PeerConnection::DoSetLocalDescription");
@@ -2522,7 +2494,8 @@
}
if (!desc) {
- observer->OnSetLocalDescriptionComplete(
+ PostSetSessionDescriptionFailure(
+ observer,
RTCError(RTCErrorType::INTERNAL_ERROR, "SessionDescription is NULL."));
return;
}
@@ -2532,7 +2505,8 @@
if (session_error() != SessionError::kNone) {
std::string error_message = GetSessionErrorMsg();
RTC_LOG(LS_ERROR) << "SetLocalDescription: " << error_message;
- observer->OnSetLocalDescriptionComplete(
+ PostSetSessionDescriptionFailure(
+ observer,
RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
return;
}
@@ -2540,11 +2514,16 @@
// For SLD we support only explicit rollback.
if (desc->GetType() == SdpType::kRollback) {
if (IsUnifiedPlan()) {
- observer->OnSetLocalDescriptionComplete(Rollback(desc->GetType()));
+ RTCError error = Rollback(desc->GetType());
+ if (error.ok()) {
+ PostSetSessionDescriptionSuccess(observer);
+ } else {
+ PostSetSessionDescriptionFailure(observer, std::move(error));
+ }
} else {
- observer->OnSetLocalDescriptionComplete(
- RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
- "Rollback not supported in Plan B"));
+ PostSetSessionDescriptionFailure(
+ observer, RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+ "Rollback not supported in Plan B"));
}
return;
}
@@ -2554,7 +2533,8 @@
std::string error_message = GetSetDescriptionErrorMessage(
cricket::CS_LOCAL, desc->GetType(), error);
RTC_LOG(LS_ERROR) << error_message;
- observer->OnSetLocalDescriptionComplete(
+ PostSetSessionDescriptionFailure(
+ observer,
RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
return;
}
@@ -2574,12 +2554,15 @@
std::string error_message =
GetSetDescriptionErrorMessage(cricket::CS_LOCAL, type, error);
RTC_LOG(LS_ERROR) << error_message;
- observer->OnSetLocalDescriptionComplete(
+ PostSetSessionDescriptionFailure(
+ observer,
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.
@@ -2604,7 +2587,6 @@
}
}
- observer->OnSetLocalDescriptionComplete(RTCError::OK());
NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
}
@@ -2899,24 +2881,27 @@
std::function<void()> operations_chain_callback) mutable {
// Abort early if |this_weak_ptr| is no longer valid.
if (!this_weak_ptr) {
- // 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.
+ // 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.
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 rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
- this_weak_ptr, observer_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().
// For backwards-compatability reasons, we declare the operation as
- // 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.
+ // completed here (rather than in OnMessage()). This ensures that
+ // subsequent offer/answer operations can start immediately (without
+ // waiting for OnMessage()).
operations_chain_callback();
});
}
@@ -2934,17 +2919,21 @@
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::INTERNAL_ERROR,
- "SetRemoteDescription failed because the session was shut down"));
+ RTCErrorType::INVALID_STATE,
+ "Failed to set remote offer sdp: failed because the session was "
+ "shut down"));
operations_chain_callback();
return;
}
this_weak_ptr->DoSetRemoteDescription(std::move(desc),
std::move(observer));
- // 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.
+ // 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.
operations_chain_callback();
});
}
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 1cb5752..2591c4b 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -207,29 +207,15 @@
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;
@@ -347,8 +333,8 @@
private:
class ImplicitCreateSessionDescriptionObserver;
friend class ImplicitCreateSessionDescriptionObserver;
- class SetSessionDescriptionObserverAdapter;
- friend class SetSessionDescriptionObserverAdapter;
+ class SetRemoteDescriptionObserverAdapter;
+ friend class SetRemoteDescriptionObserverAdapter;
// Represents the [[LocalIceCredentialsToReplace]] internal slot in the spec.
// It makes the next CreateOffer() produce new ICE credentials even if
@@ -442,7 +428,7 @@
rtc::scoped_refptr<CreateSessionDescriptionObserver> observer);
void DoSetLocalDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
- rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer);
+ rtc::scoped_refptr<SetSessionDescriptionObserver> 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 72cbffb..30b11ce 100644
--- a/pc/peer_connection_signaling_unittest.cc
+++ b/pc/peer_connection_signaling_unittest.cc
@@ -565,102 +565,30 @@
EXPECT_TRUE(observer->called());
}
-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());
+ auto observer = MockSetSessionDescriptionObserver::Create();
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();
- rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
- new FakeSetLocalDescriptionObserver());
+ auto observer = MockSetSessionDescriptionObserver::Create();
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();
- rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
- new FakeSetLocalDescriptionObserver());
+ auto observer = MockSetSessionDescriptionObserver::Create();
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) {
@@ -673,7 +601,7 @@
// By not waiting for the observer's callback we can verify that the operation
// executed immediately.
callee->pc()->SetRemoteDescription(std::move(offer),
- new FakeSetRemoteDescriptionObserver());
+ new MockSetRemoteDescriptionObserver());
EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
}
@@ -692,7 +620,7 @@
// asynchronously, when CreateOffer() completes.
callee->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
callee->pc()->SetRemoteDescription(std::move(offer),
- new FakeSetRemoteDescriptionObserver());
+ new MockSetRemoteDescriptionObserver());
// CreateOffer() is asynchronous; without message processing this operation
// should not have completed.
EXPECT_FALSE(offer_observer->called());
@@ -711,7 +639,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer.get());
+ caller->pc()->SetLocalDescription(observer);
// The offer is created asynchronously; message processing is needed for it to
// complete.
@@ -737,7 +665,7 @@
EXPECT_EQ(PeerConnection::kHaveRemoteOffer, callee->signaling_state());
auto observer = MockSetSessionDescriptionObserver::Create();
- callee->pc()->SetLocalDescription(observer.get());
+ callee->pc()->SetLocalDescription(observer);
// The answer is created asynchronously; message processing is needed for it
// to complete.
@@ -759,27 +687,28 @@
auto callee = CreatePeerConnectionWithAudioVideo();
// SetLocalDescription(), implicitly creating an offer.
- auto caller_set_local_description_observer =
- MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(
- caller_set_local_description_observer.get());
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+ caller_set_local_description_observer(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ caller->pc()->SetLocalDescription(caller_set_local_description_observer);
EXPECT_TRUE_WAIT(caller_set_local_description_observer->called(),
kWaitTimeout);
ASSERT_TRUE(caller->pc()->pending_local_description());
// SetRemoteDescription(offer)
- auto callee_set_remote_description_observer =
- MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+ callee_set_remote_description_observer(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
callee->pc()->SetRemoteDescription(
- callee_set_remote_description_observer,
+ callee_set_remote_description_observer.get(),
CloneSessionDescription(caller->pc()->pending_local_description())
.release());
// SetLocalDescription(), implicitly creating an answer.
- auto callee_set_local_description_observer =
- MockSetSessionDescriptionObserver::Create();
- callee->pc()->SetLocalDescription(
- callee_set_local_description_observer.get());
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+ callee_set_local_description_observer(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+ callee->pc()->SetLocalDescription(callee_set_local_description_observer);
EXPECT_TRUE_WAIT(callee_set_local_description_observer->called(),
kWaitTimeout);
// Chaining guarantees SetRemoteDescription() happened before
@@ -788,8 +717,9 @@
EXPECT_TRUE(callee->pc()->current_local_description());
// SetRemoteDescription(answer)
- auto caller_set_remote_description_observer =
- MockSetSessionDescriptionObserver::Create();
+ rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+ caller_set_remote_description_observer(
+ new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
caller->pc()->SetRemoteDescription(
caller_set_remote_description_observer,
CloneSessionDescription(callee->pc()->current_local_description())
@@ -807,7 +737,7 @@
auto observer = MockSetSessionDescriptionObserver::Create();
caller->pc()->Close();
- caller->pc()->SetLocalDescription(observer.get());
+ caller->pc()->SetLocalDescription(observer);
// The operation should fail asynchronously.
EXPECT_FALSE(observer->called());
@@ -826,7 +756,7 @@
auto caller = CreatePeerConnectionWithAudioVideo();
auto observer = MockSetSessionDescriptionObserver::Create();
- caller->pc()->SetLocalDescription(observer.get());
+ caller->pc()->SetLocalDescription(observer);
caller->pc()->Close();
// The operation should fail asynchronously.
@@ -858,15 +788,14 @@
// unique to Unified Plan, but the transceivers used to verify this are only
// available in Unified Plan.
TEST_F(PeerConnectionSignalingUnifiedPlanTest,
- SetLocalDescriptionExecutesImmediatelyUsingOldObserver) {
+ SetLocalDescriptionExecutesImmediately) {
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. The old observer is invoked in a posted message, so
- // waiting for it would not ensure synchronicity.
+ // executed immediately.
RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
caller->pc()->SetLocalDescription(
new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
@@ -875,22 +804,6 @@
}
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 328f579..7c0b339 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<FakeSetRemoteDescriptionObserver> observer =
- new FakeSetRemoteDescriptionObserver();
+ rtc::scoped_refptr<MockSetRemoteDescriptionObserver> observer =
+ new MockSetRemoteDescriptionObserver();
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 0a83571..2017735 100644
--- a/pc/test/mock_peer_connection_observers.h
+++ b/pc/test/mock_peer_connection_observers.h
@@ -297,26 +297,7 @@
std::string error_;
};
-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
+class MockSetRemoteDescriptionObserver
: public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
public:
bool called() const { return error_.has_value(); }
