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