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}

api/BUILD.gn

@@ -149,7 +149,6 @@ 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",

api/DEPS

@@ -172,9 +172,6 @@ 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",
   ],

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 @@ 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.
-  // According to spec, the local session description MUST be the same as was
+  // The |observer| callback will be called when done.
-  // returned by CreateOffer() or CreateAnswer() or else the operation should
+  // TODO(deadbeef): Change |desc| to be a unique_ptr, to make it clear
-  // fail. Our implementation however allows some amount of "SDP munging", but
+  // that this method always takes ownership of it.
-  // 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.
-  // (Unlike "SDP munging" before SetLocalDescription(), modifying a remote
+  // The |observer| callback will be called when done.
-  // offer or answer is allowed by the spec.)
+  // TODO(hbos): Remove when Chrome implements the new signature.
-  //
+  virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
-  // The observer is invoked as soon as the operation completes, which could be
+                                    SessionDescriptionInterface* desc) {}
-  // 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;
 

api/peer_connection_proxy.h

@@ -96,26 +96,19 @@ 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,
-              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,

api/set_local_description_observer_interface.h

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

pc/peer_connection.cc

@@ -708,12 +708,17 @@ 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<SetLocalDescriptionObserverInterface>
+      rtc::scoped_refptr<SetSessionDescriptionObserver>
           set_local_description_observer)
       : pc_(std::move(pc)),
         set_local_description_observer_(
@@ -739,27 +744,42 @@ class PeerConnection::ImplicitCreateSessionDescriptionObserver
       operation_complete_callback_();
       return;
     }
-    // DoSetLocalDescription() is a synchronous operation that invokes
+    // DoSetLocalDescription() is currently implemented as a synchronous
-    // |set_local_description_observer_| with the result.
+    // 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 "
+    // Abort early if |pc_| is no longer valid.
-                                  "session description - ") +
+    if (!pc_) {
-                          error.message()));
+      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 @@ class PeerConnection::LocalIceCredentialsToReplace {
   std::set<std::pair<std::string, std::string>> ice_credentials_;
 };
 
-// Wrapper for SetSessionDescriptionObserver that invokes the success or failure
+// Upon completion, posts a task to execute the callback of the
-// callback in a posted message handled by the peer connection. This introduces
+// SetSessionDescriptionObserver asynchronously on the same thread. At this
-// a delay that prevents recursive API calls by the observer, but this also
+// point, the state of the peer connection might no longer reflect the effects
-// means that the PeerConnection can be modified before the observer sees the
+// of the SetRemoteDescription operation, as the peer connection could have been
-// result of the operation. This is ill-advised for synchronizing states.
+// modified during the post.
-//
+// TODO(hbos): Remove this class once we remove the version of
-// Implements both the SetLocalDescriptionObserverInterface and the
+// PeerConnectionInterface::SetRemoteDescription() that takes a
-// SetRemoteDescriptionObserverInterface.
+// SetSessionDescriptionObserver as an argument.
-class PeerConnection::SetSessionDescriptionObserverAdapter
+class PeerConnection::SetRemoteDescriptionObserverAdapter
-    : public SetLocalDescriptionObserverInterface,
+    : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
-      public SetRemoteDescriptionObserverInterface {
  public:
-  SetSessionDescriptionObserverAdapter(
+  SetRemoteDescriptionObserverAdapter(
-      rtc::WeakPtr<PeerConnection> pc,
+      rtc::scoped_refptr<PeerConnection> pc,
-      rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer)
+      rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
-      : pc_(std::move(pc)), inner_observer_(std::move(inner_observer)) {}
+      : 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) {
+  rtc::scoped_refptr<PeerConnection> pc_;
-    if (!pc_)
+  rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
-      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==(
@@ -2399,51 +2407,22 @@ 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 SetSessionDescriptionObserverAdapter whose
+          // For consistency with DoSetLocalDescription(), we DO NOT inform the
-          // posted messages doesn't get processed when the PC is destroyed, we
+          // |observer_refptr| that the operation failed in this case.
-          // do not inform |observer_refptr| that the operation failed.
+          // 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
+        this_weak_ptr->DoSetLocalDescription(std::move(desc),
-        // |observer_refptr| is invoked in a posted message.
+                                             std::move(observer_refptr));
-        this_weak_ptr->DoSetLocalDescription(
+        // DoSetLocalDescription() is currently implemented as a synchronous
-            std::move(desc),
+        // operation but where the |observer|'s callbacks are invoked
-            rtc::scoped_refptr<SetLocalDescriptionObserverInterface>(
+        // 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 a post), so that the operation chain
+        // completed here (rather than in OnMessage()). This ensures that
-        // is not blocked by this operation when the observer is invoked. This
+        // subsequent offer/answer operations can start immediately (without
-        // allows the observer to trigger subsequent offer/answer operations
+        // waiting for OnMessage()).
-        // 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();
       });
 }
@@ -2451,20 +2430,13 @@ 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(), 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::SetLocalDescription(
 
 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 @@ void PeerConnection::DoSetLocalDescription(
   }
 
   if (!desc) {
-    observer->OnSetLocalDescriptionComplete(
+    PostSetSessionDescriptionFailure(
+        observer,
         RTCError(RTCErrorType::INTERNAL_ERROR, "SessionDescription is NULL."));
     return;
   }
@@ -2532,7 +2505,8 @@ void PeerConnection::DoSetLocalDescription(
   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 @@ void PeerConnection::DoSetLocalDescription(
   // 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(
+      PostSetSessionDescriptionFailure(
-          RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
+          observer, RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
-                   "Rollback not supported in Plan B"));
+                             "Rollback not supported in Plan B"));
     }
     return;
   }
@@ -2554,7 +2533,8 @@ void PeerConnection::DoSetLocalDescription(
     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 @@ void PeerConnection::DoSetLocalDescription(
     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 @@ void PeerConnection::DoSetLocalDescription(
     }
   }
 
-  observer->OnSetLocalDescriptionComplete(RTCError::OK());
   NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
 }
 
@@ -2899,24 +2881,27 @@ 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 SetSessionDescriptionObserverAdapter whose
+          // For consistency with SetRemoteDescriptionObserverAdapter, we DO NOT
-          // posted messages doesn't get processed when the PC is destroyed, we
+          // inform the |observer_refptr| that the operation failed in this
-          // do not inform |observer_refptr| that the operation failed.
+          // 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>(
+                new SetRemoteDescriptionObserverAdapter(
-                    this_weak_ptr, observer_refptr)));
+                    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
+        // completed here (rather than in OnMessage()). This ensures that
-        // is not blocked by this operation when the observer is invoked. This
+        // subsequent offer/answer operations can start immediately (without
-        // allows the observer to trigger subsequent offer/answer operations
+        // waiting for OnMessage()).
-        // synchronously if the operation chain is now empty.
         operations_chain_callback();
       });
 }
@@ -2934,17 +2919,21 @@ 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::INTERNAL_ERROR,
+              RTCErrorType::INVALID_STATE,
-              "SetRemoteDescription failed because the session was shut down"));
+              "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.
+        // DoSetRemoteDescription() is currently implemented as a synchronous
-        // The |observer| will already have been informed that it completed, and
+        // operation. The |observer| will already have been informed that it
-        // we can mark this operation as complete without any loose ends.
+        // completed, and we can mark this operation as complete without any
+        // loose ends.
         operations_chain_callback();
       });
 }

pc/peer_connection.h

@@ -207,29 +207,15 @@ 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;
@@ -347,8 +333,8 @@ class PeerConnection : public PeerConnectionInternal,
  private:
   class ImplicitCreateSessionDescriptionObserver;
   friend class ImplicitCreateSessionDescriptionObserver;
-  class SetSessionDescriptionObserverAdapter;
+  class SetRemoteDescriptionObserverAdapter;
-  friend class SetSessionDescriptionObserverAdapter;
+  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 @@ class PeerConnection : public PeerConnectionInternal,
       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);

pc/peer_connection_signaling_unittest.cc

@@ -565,102 +565,30 @@ TEST_P(PeerConnectionSignalingTest, CloseCreateOfferAndShutdown) {
   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(
+  auto observer = MockSetSessionDescriptionObserver::Create();
-      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();
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+  auto observer = MockSetSessionDescriptionObserver::Create();
-      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();
-  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+  auto observer = MockSetSessionDescriptionObserver::Create();
-      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) {
@@ -673,7 +601,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 FakeSetRemoteDescriptionObserver());
+                                     new MockSetRemoteDescriptionObserver());
   EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
 }
 
@@ -692,7 +620,7 @@ TEST_P(PeerConnectionSignalingTest, CreateOfferBlocksSetRemoteDescription) {
   // 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 @@ TEST_P(PeerConnectionSignalingTest,
   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 @@ TEST_P(PeerConnectionSignalingTest,
   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 @@ TEST_P(PeerConnectionSignalingTest,
   auto callee = CreatePeerConnectionWithAudioVideo();
 
   // SetLocalDescription(), implicitly creating an offer.
-  auto caller_set_local_description_observer =
+  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
-      MockSetSessionDescriptionObserver::Create();
+      caller_set_local_description_observer(
-  caller->pc()->SetLocalDescription(
+          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
-      caller_set_local_description_observer.get());
+  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 =
+  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
-      MockSetSessionDescriptionObserver::Create();
+      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 =
+  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
-      MockSetSessionDescriptionObserver::Create();
+      callee_set_local_description_observer(
-  callee->pc()->SetLocalDescription(
+          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
-      callee_set_local_description_observer.get());
+  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 @@ TEST_P(PeerConnectionSignalingTest,
   EXPECT_TRUE(callee->pc()->current_local_description());
 
   // SetRemoteDescription(answer)
-  auto caller_set_remote_description_observer =
+  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
-      MockSetSessionDescriptionObserver::Create();
+      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 @@ TEST_P(PeerConnectionSignalingTest,
 
   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 @@ TEST_P(PeerConnectionSignalingTest,
   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 @@ class PeerConnectionSignalingUnifiedPlanTest
 // 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
+  // executed immediately.
-  // waiting for it would not ensure synchronicity.
   RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
   caller->pc()->SetLocalDescription(
       new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
@@ -874,22 +803,6 @@ 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();

pc/peer_connection_wrapper.cc

@@ -166,8 +166,8 @@ bool PeerConnectionWrapper::SetRemoteDescription(
 bool PeerConnectionWrapper::SetRemoteDescription(
     std::unique_ptr<SessionDescriptionInterface> desc,
     RTCError* error_out) {
-  rtc::scoped_refptr<FakeSetRemoteDescriptionObserver> observer =
+  rtc::scoped_refptr<MockSetRemoteDescriptionObserver> observer =
-      new FakeSetRemoteDescriptionObserver();
+      new MockSetRemoteDescriptionObserver();
   pc()->SetRemoteDescription(std::move(desc), observer);
   EXPECT_EQ_WAIT(true, observer->called(), kDefaultTimeout);
   bool ok = observer->error().ok();

pc/test/mock_peer_connection_observers.h

@@ -297,26 +297,7 @@ class MockSetSessionDescriptionObserver
   std::string error_;
 };
 
-class FakeSetLocalDescriptionObserver
+class MockSetRemoteDescriptionObserver
-    : 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(); }