[Perfect Negotiation] Implement non-racy version of SetLocalDescription.
BACKGROUND
When SLD is invoked with SetSessionDescriptionObserver, the observer is
called by posting a message back to the execution thread, delaying the
call. This delay is "artificial" - it's not necessary; the operation is
already complete. It's a post from the signaling thread to the signaling
thread. The rationale for the post was to avoid the observer making
recursive calls back into the PeerConnection. The problem with this is
that by the time the observer is called, the PeerConnection could
already have executed other operations and modified its states.
This causes the referenced bug: one can have a race where SLD is
resolved "too late" (after a pending SRD is executed) and the signaling
state observed when SLD resolves doesn't make sense.
When implementing Unified Plan, we fixed similar issues for SRD by
adding a version that takes SetRemoteDescriptionObserverInterface as
argument instead of SetSessionDescriptionObserver. The new version did
not have the delay. The old version had to be kept around not to break
downstream projects that had dependencies both on he delay and on
allowing the PC to be destroyed midst-operation without informing its
observers.
THIS CL
This does the old SRD fix for SLD as well: A new observer interface is
added, SetLocalDescriptionObserverInterface, and
PeerConnection::SetLocalDescription() is overloaded. If you call it with
the old observer, you get the delay, but if you call it with the new
observer, you don't get a delay.
- SetLocalDescriptionObserverInterface is added.
- SetLocalDescription is overloaded.
- The adapter for SetSessionDescriptionObserver that causes the delay
previously only used for SRD is updated to handle both SLD and SRD.
- FakeSetLocalDescriptionObserver is added and
MockSetRemoteDescriptionObserver is renamed "Fake...".
Bug: chromium:1071733
Change-Id: I920368e648bede481058ac22f5b8794752a220b3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/179100
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31798}
diff --git a/api/BUILD.gn b/api/BUILD.gn
index 0d4ba2c..4ca6c9b 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -149,6 +149,7 @@
"rtp_transceiver_interface.h",
"sctp_transport_interface.cc",
"sctp_transport_interface.h",
+ "set_local_description_observer_interface.h",
"set_remote_description_observer_interface.h",
"stats_types.cc",
"stats_types.h",
diff --git a/api/DEPS b/api/DEPS
index 220b30b..4fc132b 100644
--- a/api/DEPS
+++ b/api/DEPS
@@ -172,6 +172,9 @@
"+rtc_base/ref_count.h",
],
+ "set_local_description_observer_interface\.h": [
+ "+rtc_base/ref_count.h",
+ ],
"set_remote_description_observer_interface\.h": [
"+rtc_base/ref_count.h",
],
diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index fd4d2df..52d0d87 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -97,6 +97,7 @@
#include "api/rtp_sender_interface.h"
#include "api/rtp_transceiver_interface.h"
#include "api/sctp_transport_interface.h"
+#include "api/set_local_description_observer_interface.h"
#include "api/set_remote_description_observer_interface.h"
#include "api/stats/rtc_stats_collector_callback.h"
#include "api/stats_types.h"
@@ -947,26 +948,56 @@
const RTCOfferAnswerOptions& options) = 0;
// Sets the local session description.
- // The PeerConnection takes the ownership of |desc| even if it fails.
- // The |observer| callback will be called when done.
- // TODO(deadbeef): Change |desc| to be a unique_ptr, to make it clear
- // that this method always takes ownership of it.
+ //
+ // According to spec, the local session description MUST be the same as was
+ // returned by CreateOffer() or CreateAnswer() or else the operation should
+ // fail. Our implementation however allows some amount of "SDP munging", but
+ // please note that this is HIGHLY DISCOURAGED. If you do not intent to munge
+ // SDP, the method below that doesn't take |desc| as an argument will create
+ // the offer or answer for you.
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetLocalDescription() method has exited.
+ virtual void SetLocalDescription(
+ std::unique_ptr<SessionDescriptionInterface> desc,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {}
+ // Creates an offer or answer (depending on current signaling state) and sets
+ // it as the local session description.
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetLocalDescription() method has exited.
+ virtual void SetLocalDescription(
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {}
+ // Like SetLocalDescription() above, but the observer is invoked with a delay
+ // after the operation completes. This helps avoid recursive calls by the
+ // observer but also makes it possible for states to change in-between the
+ // operation completing and the observer getting called. This makes them racy
+ // for synchronizing peer connection states to the application.
+ // TODO(https://crbug.com/webrtc/11798): Delete these methods in favor of the
+ // ones taking SetLocalDescriptionObserverInterface as argument.
virtual void SetLocalDescription(SetSessionDescriptionObserver* observer,
SessionDescriptionInterface* desc) = 0;
- // Implicitly creates an offer or answer (depending on the current signaling
- // state) and performs SetLocalDescription() with the newly generated session
- // description.
- // TODO(hbos): Make pure virtual when implemented by downstream projects.
virtual void SetLocalDescription(SetSessionDescriptionObserver* observer) {}
+
// Sets the remote session description.
- // The PeerConnection takes the ownership of |desc| even if it fails.
- // The |observer| callback will be called when done.
- // TODO(hbos): Remove when Chrome implements the new signature.
- virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
- SessionDescriptionInterface* desc) {}
+ //
+ // (Unlike "SDP munging" before SetLocalDescription(), modifying a remote
+ // offer or answer is allowed by the spec.)
+ //
+ // The observer is invoked as soon as the operation completes, which could be
+ // before or after the SetRemoteDescription() method has exited.
virtual void SetRemoteDescription(
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer) = 0;
+ // Like SetRemoteDescription() above, but the observer is invoked with a delay
+ // after the operation completes. This helps avoid recursive calls by the
+ // observer but also makes it possible for states to change in-between the
+ // operation completing and the observer getting called. This makes them racy
+ // for synchronizing peer connection states to the application.
+ // TODO(https://crbug.com/webrtc/11798): Delete this method in favor of the
+ // ones taking SetRemoteDescriptionObserverInterface as argument.
+ virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+ SessionDescriptionInterface* desc) {}
virtual PeerConnectionInterface::RTCConfiguration GetConfiguration() = 0;
diff --git a/api/peer_connection_proxy.h b/api/peer_connection_proxy.h
index 23887e5..aa72d7c 100644
--- a/api/peer_connection_proxy.h
+++ b/api/peer_connection_proxy.h
@@ -98,17 +98,24 @@
const RTCOfferAnswerOptions&)
PROXY_METHOD2(void,
SetLocalDescription,
+ std::unique_ptr<SessionDescriptionInterface>,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
+PROXY_METHOD1(void,
+ SetLocalDescription,
+ rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
+PROXY_METHOD2(void,
+ SetLocalDescription,
SetSessionDescriptionObserver*,
SessionDescriptionInterface*)
PROXY_METHOD1(void, SetLocalDescription, SetSessionDescriptionObserver*)
PROXY_METHOD2(void,
SetRemoteDescription,
- SetSessionDescriptionObserver*,
- SessionDescriptionInterface*)
-PROXY_METHOD2(void,
- SetRemoteDescription,
std::unique_ptr<SessionDescriptionInterface>,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>)
+PROXY_METHOD2(void,
+ SetRemoteDescription,
+ SetSessionDescriptionObserver*,
+ SessionDescriptionInterface*)
PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration)
PROXY_METHOD1(RTCError,
SetConfiguration,
diff --git a/api/set_local_description_observer_interface.h b/api/set_local_description_observer_interface.h
new file mode 100644
index 0000000..90d000c
--- /dev/null
+++ b/api/set_local_description_observer_interface.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright 2020 The WebRTC project authors. All Rights Reserved.
+ *
+ * Use of this source code is governed by a BSD-style license
+ * that can be found in the LICENSE file in the root of the source
+ * tree. An additional intellectual property rights grant can be found
+ * in the file PATENTS. All contributing project authors may
+ * be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_
+#define API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_
+
+#include "api/rtc_error.h"
+#include "rtc_base/ref_count.h"
+
+namespace webrtc {
+
+// OnSetLocalDescriptionComplete() invokes as soon as
+// PeerConnectionInterface::SetLocalDescription() operation completes, allowing
+// the observer to examine the effects of the operation without delay.
+class SetLocalDescriptionObserverInterface : public rtc::RefCountInterface {
+ public:
+ // On success, |error.ok()| is true.
+ virtual void OnSetLocalDescriptionComplete(RTCError error) = 0;
+};
+
+} // namespace webrtc
+
+#endif // API_SET_LOCAL_DESCRIPTION_OBSERVER_INTERFACE_H_