[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}

api/BUILD.gn

@@ -149,6 +149,7 @@ rtc_library("libjingle_peerconnection_api") {
     "rtp_transceiver_interface.h",
     "sctp_transport_interface.cc",
     "sctp_transport_interface.h",
+    "set_local_description_observer_interface.h",
     "set_remote_description_observer_interface.h",
     "stats_types.cc",
     "stats_types.h",

api/DEPS

@@ -172,6 +172,9 @@ specific_include_rules = {
     "+rtc_base/ref_count.h",
   ],
 
+  "set_local_description_observer_interface\.h": [
+    "+rtc_base/ref_count.h",
+  ],
   "set_remote_description_observer_interface\.h": [
     "+rtc_base/ref_count.h",
   ],

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 @@ class RTC_EXPORT PeerConnectionInterface : public rtc::RefCountInterface {
                             const RTCOfferAnswerOptions& options) = 0;
 
   // Sets the local session description.
-  // The PeerConnection takes the ownership of |desc| even if it fails.
+  //
-  // The |observer| callback will be called when done.
+  // According to spec, the local session description MUST be the same as was
-  // TODO(deadbeef): Change |desc| to be a unique_ptr, to make it clear
+  // returned by CreateOffer() or CreateAnswer() or else the operation should
-  // that this method always takes ownership of it.
+  // 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.
+  // (Unlike "SDP munging" before SetLocalDescription(), modifying a remote
-  // TODO(hbos): Remove when Chrome implements the new signature.
+  // offer or answer is allowed by the spec.)
-  virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+  //
-                                    SessionDescriptionInterface* desc) {}
+  // 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;
 

api/peer_connection_proxy.h

@@ -96,19 +96,26 @@ PROXY_METHOD2(void,
               CreateAnswer,
               CreateSessionDescriptionObserver*,
               const RTCOfferAnswerOptions&)
+PROXY_METHOD2(void,
+              SetLocalDescription,
+              std::unique_ptr<SessionDescriptionInterface>,
+              rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
+PROXY_METHOD1(void,
+              SetLocalDescription,
+              rtc::scoped_refptr<SetLocalDescriptionObserverInterface>)
 PROXY_METHOD2(void,
               SetLocalDescription,
               SetSessionDescriptionObserver*,
               SessionDescriptionInterface*)
 PROXY_METHOD1(void, SetLocalDescription, SetSessionDescriptionObserver*)
-PROXY_METHOD2(void,
-              SetRemoteDescription,
-              SetSessionDescriptionObserver*,
-              SessionDescriptionInterface*)
 PROXY_METHOD2(void,
               SetRemoteDescription,
               std::unique_ptr<SessionDescriptionInterface>,
               rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>)
+PROXY_METHOD2(void,
+              SetRemoteDescription,
+              SetSessionDescriptionObserver*,
+              SessionDescriptionInterface*)
 PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration)
 PROXY_METHOD1(RTCError,
               SetConfiguration,

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_

pc/peer_connection.cc

@@ -708,17 +708,12 @@ bool NeedIceRestart(bool surface_ice_candidates_on_ice_transport_type_changed,
 // Used by parameterless SetLocalDescription() to create an offer or answer.
 // Upon completion of creating the session description, SetLocalDescription() is
 // invoked with the result.
-// For consistency with DoSetLocalDescription(), if the PeerConnection is
-// destroyed midst operation, we DO NOT inform the
-// |set_local_description_observer| that the operation failed.
-// TODO(hbos): If/when we process SLD messages in ~PeerConnection, the
-// consistent thing would be to inform the observer here.
 class PeerConnection::ImplicitCreateSessionDescriptionObserver
     : public CreateSessionDescriptionObserver {
  public:
   ImplicitCreateSessionDescriptionObserver(
       rtc::WeakPtr<PeerConnection> pc,
-      rtc::scoped_refptr<SetSessionDescriptionObserver>
+      rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
           set_local_description_observer)
       : pc_(std::move(pc)),
         set_local_description_observer_(
@@ -744,33 +739,18 @@ class PeerConnection::ImplicitCreateSessionDescriptionObserver
       operation_complete_callback_();
       return;
     }
-    // DoSetLocalDescription() is currently implemented as a synchronous
+    // DoSetLocalDescription() is a synchronous operation that invokes
-    // operation but where the |set_local_description_observer_|'s callbacks are
+    // |set_local_description_observer_| with the result.
-    // 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(
-    // Abort early if |pc_| is no longer valid.
+        error.type(), std::string("SetLocalDescription failed to create "
-    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_();
@@ -779,7 +759,7 @@ class PeerConnection::ImplicitCreateSessionDescriptionObserver
  private:
   bool was_called_ = false;
   rtc::WeakPtr<PeerConnection> pc_;
-  rtc::scoped_refptr<SetSessionDescriptionObserver>
+  rtc::scoped_refptr<SetLocalDescriptionObserverInterface>
       set_local_description_observer_;
   std::function<void()> operation_complete_callback_;
 };
@@ -833,33 +813,45 @@ class PeerConnection::LocalIceCredentialsToReplace {
   std::set<std::pair<std::string, std::string>> ice_credentials_;
 };
 
-// Upon completion, posts a task to execute the callback of the
+// Wrapper for SetSessionDescriptionObserver that invokes the success or failure
-// SetSessionDescriptionObserver asynchronously on the same thread. At this
+// callback in a posted message handled by the peer connection. This introduces
-// point, the state of the peer connection might no longer reflect the effects
+// a delay that prevents recursive API calls by the observer, but this also
-// of the SetRemoteDescription operation, as the peer connection could have been
+// means that the PeerConnection can be modified before the observer sees the
-// modified during the post.
+// result of the operation. This is ill-advised for synchronizing states.
-// TODO(hbos): Remove this class once we remove the version of
+//
-// PeerConnectionInterface::SetRemoteDescription() that takes a
+// Implements both the SetLocalDescriptionObserverInterface and the
-// SetSessionDescriptionObserver as an argument.
+// SetRemoteDescriptionObserverInterface.
-class PeerConnection::SetRemoteDescriptionObserverAdapter
+class PeerConnection::SetSessionDescriptionObserverAdapter
-    : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
+    : public SetLocalDescriptionObserverInterface,
+      public SetRemoteDescriptionObserverInterface {
  public:
-  SetRemoteDescriptionObserverAdapter(
+  SetSessionDescriptionObserverAdapter(
-      rtc::scoped_refptr<PeerConnection> pc,
+      rtc::WeakPtr<PeerConnection> pc,
-      rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
+      rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer)
-      : pc_(std::move(pc)), wrapper_(std::move(wrapper)) {}
+      : pc_(std::move(pc)), inner_observer_(std::move(inner_observer)) {}
 
+  // SetLocalDescriptionObserverInterface implementation.
+  void OnSetLocalDescriptionComplete(RTCError error) override {
+    OnSetDescriptionComplete(std::move(error));
+  }
   // SetRemoteDescriptionObserverInterface implementation.
   void OnSetRemoteDescriptionComplete(RTCError error) override {
-    if (error.ok())
+    OnSetDescriptionComplete(std::move(error));
-      pc_->PostSetSessionDescriptionSuccess(wrapper_);
-    else
-      pc_->PostSetSessionDescriptionFailure(wrapper_, std::move(error));
   }
 
  private:
-  rtc::scoped_refptr<PeerConnection> pc_;
+  void OnSetDescriptionComplete(RTCError error) {
-  rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
+    if (!pc_)
+      return;
+    if (error.ok()) {
+      pc_->PostSetSessionDescriptionSuccess(inner_observer_);
+    } else {
+      pc_->PostSetSessionDescriptionFailure(inner_observer_, std::move(error));
+    }
+  }
+
+  rtc::WeakPtr<PeerConnection> pc_;
+  rtc::scoped_refptr<SetSessionDescriptionObserver> inner_observer_;
 };
 
 bool PeerConnectionInterface::RTCConfiguration::operator==(
@@ -2407,22 +2399,51 @@ void PeerConnection::SetLocalDescription(
           std::function<void()> operations_chain_callback) mutable {
         // Abort early if |this_weak_ptr| is no longer valid.
         if (!this_weak_ptr) {
-          // For consistency with DoSetLocalDescription(), we DO NOT inform the
+          // For consistency with SetSessionDescriptionObserverAdapter whose
-          // |observer_refptr| that the operation failed in this case.
+          // posted messages doesn't get processed when the PC is destroyed, we
-          // TODO(hbos): If/when we process SLD messages in ~PeerConnection,
+          // do not inform |observer_refptr| that the operation failed.
-          // the consistent thing would be to inform the observer here.
           operations_chain_callback();
           return;
         }
-        this_weak_ptr->DoSetLocalDescription(std::move(desc),
+        // SetSessionDescriptionObserverAdapter takes care of making sure the
-                                             std::move(observer_refptr));
+        // |observer_refptr| is invoked in a posted message.
-        // DoSetLocalDescription() is currently implemented as a synchronous
+        this_weak_ptr->DoSetLocalDescription(
-        // operation but where the |observer|'s callbacks are invoked
+            std::move(desc),
-        // asynchronously in a post to OnMessage().
+            rtc::scoped_refptr<SetLocalDescriptionObserverInterface>(
+                new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
+                    this_weak_ptr, observer_refptr)));
         // For backwards-compatability reasons, we declare the operation as
-        // completed here (rather than in OnMessage()). This ensures that
+        // completed here (rather than in a post), so that the operation chain
-        // subsequent offer/answer operations can start immediately (without
+        // is not blocked by this operation when the observer is invoked. This
-        // waiting for OnMessage()).
+        // allows the observer to trigger subsequent offer/answer operations
+        // synchronously if the operation chain is now empty.
+        operations_chain_callback();
+      });
+}
+
+void PeerConnection::SetLocalDescription(
+    std::unique_ptr<SessionDescriptionInterface> desc,
+    rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
+  RTC_DCHECK_RUN_ON(signaling_thread());
+  // Chain this operation. If asynchronous operations are pending on the chain,
+  // this operation will be queued to be invoked, otherwise the contents of the
+  // lambda will execute immediately.
+  operations_chain_->ChainOperation(
+      [this_weak_ptr = weak_ptr_factory_.GetWeakPtr(), observer,
+       desc = std::move(desc)](
+          std::function<void()> operations_chain_callback) mutable {
+        // Abort early if |this_weak_ptr| is no longer valid.
+        if (!this_weak_ptr) {
+          observer->OnSetLocalDescriptionComplete(RTCError(
+              RTCErrorType::INTERNAL_ERROR,
+              "SetLocalDescription failed because the session was shut down"));
+          operations_chain_callback();
+          return;
+        }
+        this_weak_ptr->DoSetLocalDescription(std::move(desc), observer);
+        // DoSetLocalDescription() is implemented as a synchronous operation.
+        // The |observer| will already have been informed that it completed, and
+        // we can mark this operation as complete without any loose ends.
         operations_chain_callback();
       });
 }
@@ -2430,13 +2451,20 @@ void PeerConnection::SetLocalDescription(
 void PeerConnection::SetLocalDescription(
     SetSessionDescriptionObserver* observer) {
   RTC_DCHECK_RUN_ON(signaling_thread());
+  SetLocalDescription(
+      new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
+          weak_ptr_factory_.GetWeakPtr(), observer));
+}
+
+void PeerConnection::SetLocalDescription(
+    rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
+  RTC_DCHECK_RUN_ON(signaling_thread());
   // The |create_sdp_observer| handles performing DoSetLocalDescription() with
   // the resulting description as well as completing the operation.
   rtc::scoped_refptr<ImplicitCreateSessionDescriptionObserver>
       create_sdp_observer(
           new rtc::RefCountedObject<ImplicitCreateSessionDescriptionObserver>(
-              weak_ptr_factory_.GetWeakPtr(),
+              weak_ptr_factory_.GetWeakPtr(), observer));
-              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.
@@ -2484,7 +2512,7 @@ void PeerConnection::SetLocalDescription(
 
 void PeerConnection::DoSetLocalDescription(
     std::unique_ptr<SessionDescriptionInterface> desc,
-    rtc::scoped_refptr<SetSessionDescriptionObserver> observer) {
+    rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
   RTC_DCHECK_RUN_ON(signaling_thread());
   TRACE_EVENT0("webrtc", "PeerConnection::DoSetLocalDescription");
 
@@ -2494,8 +2522,7 @@ void PeerConnection::DoSetLocalDescription(
   }
 
   if (!desc) {
-    PostSetSessionDescriptionFailure(
+    observer->OnSetLocalDescriptionComplete(
-        observer,
         RTCError(RTCErrorType::INTERNAL_ERROR, "SessionDescription is NULL."));
     return;
   }
@@ -2505,8 +2532,7 @@ void PeerConnection::DoSetLocalDescription(
   if (session_error() != SessionError::kNone) {
     std::string error_message = GetSessionErrorMsg();
     RTC_LOG(LS_ERROR) << "SetLocalDescription: " << error_message;
-    PostSetSessionDescriptionFailure(
+    observer->OnSetLocalDescriptionComplete(
-        observer,
         RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
     return;
   }
@@ -2514,15 +2540,10 @@ void PeerConnection::DoSetLocalDescription(
   // For SLD we support only explicit rollback.
   if (desc->GetType() == SdpType::kRollback) {
     if (IsUnifiedPlan()) {
-      RTCError error = Rollback(desc->GetType());
+      observer->OnSetLocalDescriptionComplete(Rollback(desc->GetType()));
-      if (error.ok()) {
-        PostSetSessionDescriptionSuccess(observer);
     } else {
-        PostSetSessionDescriptionFailure(observer, std::move(error));
+      observer->OnSetLocalDescriptionComplete(
-      }
+          RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
-    } else {
-      PostSetSessionDescriptionFailure(
-          observer, RTCError(RTCErrorType::UNSUPPORTED_OPERATION,
                    "Rollback not supported in Plan B"));
     }
     return;
@@ -2533,8 +2554,7 @@ void PeerConnection::DoSetLocalDescription(
     std::string error_message = GetSetDescriptionErrorMessage(
         cricket::CS_LOCAL, desc->GetType(), error);
     RTC_LOG(LS_ERROR) << error_message;
-    PostSetSessionDescriptionFailure(
+    observer->OnSetLocalDescriptionComplete(
-        observer,
         RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
     return;
   }
@@ -2554,15 +2574,12 @@ void PeerConnection::DoSetLocalDescription(
     std::string error_message =
         GetSetDescriptionErrorMessage(cricket::CS_LOCAL, type, error);
     RTC_LOG(LS_ERROR) << error_message;
-    PostSetSessionDescriptionFailure(
+    observer->OnSetLocalDescriptionComplete(
-        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.
@@ -2587,6 +2604,7 @@ void PeerConnection::DoSetLocalDescription(
     }
   }
 
+  observer->OnSetLocalDescriptionComplete(RTCError::OK());
   NoteUsageEvent(UsageEvent::SET_LOCAL_DESCRIPTION_SUCCEEDED);
 }
 
@@ -2881,27 +2899,24 @@ void PeerConnection::SetRemoteDescription(
           std::function<void()> operations_chain_callback) mutable {
         // Abort early if |this_weak_ptr| is no longer valid.
         if (!this_weak_ptr) {
-          // For consistency with SetRemoteDescriptionObserverAdapter, we DO NOT
+          // For consistency with SetSessionDescriptionObserverAdapter whose
-          // inform the |observer_refptr| that the operation failed in this
+          // posted messages doesn't get processed when the PC is destroyed, we
-          // case.
+          // do not inform |observer_refptr| that the operation failed.
-          // 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 SetRemoteDescriptionObserverAdapter(
+                new rtc::RefCountedObject<SetSessionDescriptionObserverAdapter>(
-                    this_weak_ptr.get(), std::move(observer_refptr))));
+                    this_weak_ptr, 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 OnMessage()). This ensures that
+        // completed here (rather than in a post), so that the operation chain
-        // subsequent offer/answer operations can start immediately (without
+        // is not blocked by this operation when the observer is invoked. This
-        // waiting for OnMessage()).
+        // allows the observer to trigger subsequent offer/answer operations
+        // synchronously if the operation chain is now empty.
         operations_chain_callback();
       });
 }
@@ -2919,21 +2934,17 @@ void PeerConnection::SetRemoteDescription(
           std::function<void()> operations_chain_callback) mutable {
         // Abort early if |this_weak_ptr| is no longer valid.
         if (!this_weak_ptr) {
-          // For consistency with DoSetRemoteDescription(), we DO inform the
-          // |observer| that the operation failed in this case.
           observer->OnSetRemoteDescriptionComplete(RTCError(
-              RTCErrorType::INVALID_STATE,
+              RTCErrorType::INTERNAL_ERROR,
-              "Failed to set remote offer sdp: failed because the session was "
+              "SetRemoteDescription failed because the session was shut down"));
-              "shut down"));
           operations_chain_callback();
           return;
         }
         this_weak_ptr->DoSetRemoteDescription(std::move(desc),
                                               std::move(observer));
-        // DoSetRemoteDescription() is currently implemented as a synchronous
+        // DoSetRemoteDescription() is implemented as a synchronous operation.
-        // operation. The |observer| will already have been informed that it
+        // The |observer| will already have been informed that it completed, and
-        // completed, and we can mark this operation as complete without any
+        // we can mark this operation as complete without any loose ends.
-        // loose ends.
         operations_chain_callback();
       });
 }

pc/peer_connection.h

@@ -207,15 +207,29 @@ class PeerConnection : public PeerConnectionInternal,
                    const RTCOfferAnswerOptions& options) override;
   void CreateAnswer(CreateSessionDescriptionObserver* observer,
                     const RTCOfferAnswerOptions& options) override;
+
+  void SetLocalDescription(
+      std::unique_ptr<SessionDescriptionInterface> desc,
+      rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer)
+      override;
+  void SetLocalDescription(
+      rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer)
+      override;
+  // TODO(https://crbug.com/webrtc/11798): Delete these methods in favor of the
+  // ones taking SetLocalDescriptionObserverInterface as argument.
   void SetLocalDescription(SetSessionDescriptionObserver* observer,
                            SessionDescriptionInterface* desc) override;
   void SetLocalDescription(SetSessionDescriptionObserver* observer) override;
-  void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+
-                            SessionDescriptionInterface* desc) override;
   void SetRemoteDescription(
       std::unique_ptr<SessionDescriptionInterface> desc,
       rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer)
       override;
+  // TODO(https://crbug.com/webrtc/11798): Delete this methods in favor of the
+  // ones taking SetRemoteDescriptionObserverInterface as argument.
+  void SetRemoteDescription(SetSessionDescriptionObserver* observer,
+                            SessionDescriptionInterface* desc) override;
+
   PeerConnectionInterface::RTCConfiguration GetConfiguration() override;
   RTCError SetConfiguration(
       const PeerConnectionInterface::RTCConfiguration& configuration) override;
@@ -333,8 +347,8 @@ class PeerConnection : public PeerConnectionInternal,
  private:
   class ImplicitCreateSessionDescriptionObserver;
   friend class ImplicitCreateSessionDescriptionObserver;
-  class SetRemoteDescriptionObserverAdapter;
+  class SetSessionDescriptionObserverAdapter;
-  friend class SetRemoteDescriptionObserverAdapter;
+  friend class SetSessionDescriptionObserverAdapter;
 
   // Represents the [[LocalIceCredentialsToReplace]] internal slot in the spec.
   // It makes the next CreateOffer() produce new ICE credentials even if
@@ -428,7 +442,7 @@ class PeerConnection : public PeerConnectionInternal,
       rtc::scoped_refptr<CreateSessionDescriptionObserver> observer);
   void DoSetLocalDescription(
       std::unique_ptr<SessionDescriptionInterface> desc,
-      rtc::scoped_refptr<SetSessionDescriptionObserver> observer);
+      rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer);
   void DoSetRemoteDescription(
       std::unique_ptr<SessionDescriptionInterface> desc,
       rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer);

pc/peer_connection_signaling_unittest.cc

@@ -565,30 +565,102 @@ TEST_P(PeerConnectionSignalingTest, CloseCreateOfferAndShutdown) {
   EXPECT_TRUE(observer->called());
 }
 
-TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+TEST_P(PeerConnectionSignalingTest,
+       ImplicitCreateOfferAndShutdownWithOldObserver) {
   auto caller = CreatePeerConnection();
   auto observer = MockSetSessionDescriptionObserver::Create();
+  caller->pc()->SetLocalDescription(observer.get());
+  caller.reset(nullptr);
+  // The old observer does not get invoked because posted messages are lost.
+  EXPECT_FALSE(observer->called());
+}
+
+TEST_P(PeerConnectionSignalingTest, ImplicitCreateOfferAndShutdown) {
+  auto caller = CreatePeerConnection();
+  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+      new FakeSetLocalDescriptionObserver());
   caller->pc()->SetLocalDescription(observer);
   caller.reset(nullptr);
+  // The new observer gets invoked because it is called immediately.
+  EXPECT_TRUE(observer->called());
+  EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+       CloseBeforeImplicitCreateOfferAndShutdownWithOldObserver) {
+  auto caller = CreatePeerConnection();
+  auto observer = MockSetSessionDescriptionObserver::Create();
+  caller->pc()->Close();
+  caller->pc()->SetLocalDescription(observer.get());
+  caller.reset(nullptr);
+  // The old observer does not get invoked because posted messages are lost.
   EXPECT_FALSE(observer->called());
 }
 
 TEST_P(PeerConnectionSignalingTest, CloseBeforeImplicitCreateOfferAndShutdown) {
   auto caller = CreatePeerConnection();
-  auto observer = MockSetSessionDescriptionObserver::Create();
+  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+      new FakeSetLocalDescriptionObserver());
   caller->pc()->Close();
   caller->pc()->SetLocalDescription(observer);
   caller.reset(nullptr);
+  // The new observer gets invoked because it is called immediately.
+  EXPECT_TRUE(observer->called());
+  EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+       CloseAfterImplicitCreateOfferAndShutdownWithOldObserver) {
+  auto caller = CreatePeerConnection();
+  auto observer = MockSetSessionDescriptionObserver::Create();
+  caller->pc()->SetLocalDescription(observer.get());
+  caller->pc()->Close();
+  caller.reset(nullptr);
+  // The old observer does not get invoked because posted messages are lost.
   EXPECT_FALSE(observer->called());
 }
 
 TEST_P(PeerConnectionSignalingTest, CloseAfterImplicitCreateOfferAndShutdown) {
   auto caller = CreatePeerConnection();
-  auto observer = MockSetSessionDescriptionObserver::Create();
+  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+      new FakeSetLocalDescriptionObserver());
   caller->pc()->SetLocalDescription(observer);
   caller->pc()->Close();
   caller.reset(nullptr);
+  // The new observer gets invoked because it is called immediately.
+  EXPECT_TRUE(observer->called());
+  EXPECT_FALSE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+       SetLocalDescriptionNewObserverIsInvokedImmediately) {
+  auto caller = CreatePeerConnection();
+  auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+  rtc::scoped_refptr<FakeSetLocalDescriptionObserver> observer(
+      new FakeSetLocalDescriptionObserver());
+  caller->pc()->SetLocalDescription(std::move(offer), observer);
+  // The new observer is invoked immediately.
+  EXPECT_TRUE(observer->called());
+  EXPECT_TRUE(observer->error().ok());
+}
+
+TEST_P(PeerConnectionSignalingTest,
+       SetLocalDescriptionOldObserverIsInvokedInAPostedMessage) {
+  auto caller = CreatePeerConnection();
+  auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+  auto observer = MockSetSessionDescriptionObserver::Create();
+  caller->pc()->SetLocalDescription(observer, offer.release());
+  // The old observer is not invoked immediately.
   EXPECT_FALSE(observer->called());
+  // Process all currently pending messages by waiting for a posted task to run.
+  bool checkpoint_reached = false;
+  rtc::Thread::Current()->PostTask(
+      RTC_FROM_HERE, [&checkpoint_reached] { checkpoint_reached = true; });
+  EXPECT_TRUE_WAIT(checkpoint_reached, kWaitTimeout);
+  // If resolving the observer was pending, it must now have been called.
+  EXPECT_TRUE(observer->called());
 }
 
 TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) {
@@ -601,7 +673,7 @@ TEST_P(PeerConnectionSignalingTest, SetRemoteDescriptionExecutesImmediately) {
   // By not waiting for the observer's callback we can verify that the operation
   // executed immediately.
   callee->pc()->SetRemoteDescription(std::move(offer),
-                                     new MockSetRemoteDescriptionObserver());
+                                     new FakeSetRemoteDescriptionObserver());
   EXPECT_EQ(2u, callee->pc()->GetReceivers().size());
 }
 
@@ -620,7 +692,7 @@ TEST_P(PeerConnectionSignalingTest, CreateOfferBlocksSetRemoteDescription) {
   // asynchronously, when CreateOffer() completes.
   callee->pc()->CreateOffer(offer_observer, RTCOfferAnswerOptions());
   callee->pc()->SetRemoteDescription(std::move(offer),
-                                     new MockSetRemoteDescriptionObserver());
+                                     new FakeSetRemoteDescriptionObserver());
   // CreateOffer() is asynchronous; without message processing this operation
   // should not have completed.
   EXPECT_FALSE(offer_observer->called());
@@ -639,7 +711,7 @@ TEST_P(PeerConnectionSignalingTest,
   auto caller = CreatePeerConnectionWithAudioVideo();
 
   auto observer = MockSetSessionDescriptionObserver::Create();
-  caller->pc()->SetLocalDescription(observer);
+  caller->pc()->SetLocalDescription(observer.get());
 
   // The offer is created asynchronously; message processing is needed for it to
   // complete.
@@ -665,7 +737,7 @@ TEST_P(PeerConnectionSignalingTest,
   EXPECT_EQ(PeerConnection::kHaveRemoteOffer, callee->signaling_state());
 
   auto observer = MockSetSessionDescriptionObserver::Create();
-  callee->pc()->SetLocalDescription(observer);
+  callee->pc()->SetLocalDescription(observer.get());
 
   // The answer is created asynchronously; message processing is needed for it
   // to complete.
@@ -687,28 +759,27 @@ TEST_P(PeerConnectionSignalingTest,
   auto callee = CreatePeerConnectionWithAudioVideo();
 
   // SetLocalDescription(), implicitly creating an offer.
-  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+  auto caller_set_local_description_observer =
-      caller_set_local_description_observer(
+      MockSetSessionDescriptionObserver::Create();
-          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+  caller->pc()->SetLocalDescription(
-  caller->pc()->SetLocalDescription(caller_set_local_description_observer);
+      caller_set_local_description_observer.get());
   EXPECT_TRUE_WAIT(caller_set_local_description_observer->called(),
                    kWaitTimeout);
   ASSERT_TRUE(caller->pc()->pending_local_description());
 
   // SetRemoteDescription(offer)
-  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+  auto callee_set_remote_description_observer =
-      callee_set_remote_description_observer(
+      MockSetSessionDescriptionObserver::Create();
-          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
   callee->pc()->SetRemoteDescription(
-      callee_set_remote_description_observer.get(),
+      callee_set_remote_description_observer,
       CloneSessionDescription(caller->pc()->pending_local_description())
           .release());
 
   // SetLocalDescription(), implicitly creating an answer.
-  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+  auto callee_set_local_description_observer =
-      callee_set_local_description_observer(
+      MockSetSessionDescriptionObserver::Create();
-          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+  callee->pc()->SetLocalDescription(
-  callee->pc()->SetLocalDescription(callee_set_local_description_observer);
+      callee_set_local_description_observer.get());
   EXPECT_TRUE_WAIT(callee_set_local_description_observer->called(),
                    kWaitTimeout);
   // Chaining guarantees SetRemoteDescription() happened before
@@ -717,9 +788,8 @@ TEST_P(PeerConnectionSignalingTest,
   EXPECT_TRUE(callee->pc()->current_local_description());
 
   // SetRemoteDescription(answer)
-  rtc::scoped_refptr<MockSetSessionDescriptionObserver>
+  auto caller_set_remote_description_observer =
-      caller_set_remote_description_observer(
+      MockSetSessionDescriptionObserver::Create();
-          new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
   caller->pc()->SetRemoteDescription(
       caller_set_remote_description_observer,
       CloneSessionDescription(callee->pc()->current_local_description())
@@ -737,7 +807,7 @@ TEST_P(PeerConnectionSignalingTest,
 
   auto observer = MockSetSessionDescriptionObserver::Create();
   caller->pc()->Close();
-  caller->pc()->SetLocalDescription(observer);
+  caller->pc()->SetLocalDescription(observer.get());
 
   // The operation should fail asynchronously.
   EXPECT_FALSE(observer->called());
@@ -756,7 +826,7 @@ TEST_P(PeerConnectionSignalingTest,
   auto caller = CreatePeerConnectionWithAudioVideo();
 
   auto observer = MockSetSessionDescriptionObserver::Create();
-  caller->pc()->SetLocalDescription(observer);
+  caller->pc()->SetLocalDescription(observer.get());
   caller->pc()->Close();
 
   // The operation should fail asynchronously.
@@ -788,14 +858,15 @@ class PeerConnectionSignalingUnifiedPlanTest
 // unique to Unified Plan, but the transceivers used to verify this are only
 // available in Unified Plan.
 TEST_F(PeerConnectionSignalingUnifiedPlanTest,
-       SetLocalDescriptionExecutesImmediately) {
+       SetLocalDescriptionExecutesImmediatelyUsingOldObserver) {
   auto caller = CreatePeerConnectionWithAudioVideo();
 
   // This offer will cause transceiver mids to get assigned.
   auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
 
   // By not waiting for the observer's callback we can verify that the operation
-  // executed immediately.
+  // executed immediately. The old observer is invoked in a posted message, so
+  // waiting for it would not ensure synchronicity.
   RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
   caller->pc()->SetLocalDescription(
       new rtc::RefCountedObject<MockSetSessionDescriptionObserver>(),
@@ -803,6 +874,22 @@ TEST_F(PeerConnectionSignalingUnifiedPlanTest,
   EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value());
 }
 
+TEST_F(PeerConnectionSignalingUnifiedPlanTest,
+       SetLocalDescriptionExecutesImmediatelyUsingNewObserver) {
+  auto caller = CreatePeerConnectionWithAudioVideo();
+
+  // This offer will cause transceiver mids to get assigned.
+  auto offer = caller->CreateOffer(RTCOfferAnswerOptions());
+
+  // Verify that mids were assigned without waiting for the observer. (However,
+  // the new observer should also be invoked synchronously - as is ensured by
+  // other tests.)
+  RTC_DCHECK(!caller->pc()->GetTransceivers()[0]->mid().has_value());
+  caller->pc()->SetLocalDescription(std::move(offer),
+                                    new FakeSetLocalDescriptionObserver());
+  EXPECT_TRUE(caller->pc()->GetTransceivers()[0]->mid().has_value());
+}
+
 TEST_F(PeerConnectionSignalingUnifiedPlanTest,
        SetLocalDescriptionExecutesImmediatelyInsideCreateOfferCallback) {
   auto caller = CreatePeerConnectionWithAudioVideo();

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<MockSetRemoteDescriptionObserver> observer =
+  rtc::scoped_refptr<FakeSetRemoteDescriptionObserver> observer =
-      new MockSetRemoteDescriptionObserver();
+      new FakeSetRemoteDescriptionObserver();
   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,7 +297,26 @@ class MockSetSessionDescriptionObserver
   std::string error_;
 };
 
-class MockSetRemoteDescriptionObserver
+class FakeSetLocalDescriptionObserver
+    : public rtc::RefCountedObject<SetLocalDescriptionObserverInterface> {
+ public:
+  bool called() const { return error_.has_value(); }
+  RTCError& error() {
+    RTC_DCHECK(error_.has_value());
+    return *error_;
+  }
+
+  // SetLocalDescriptionObserverInterface implementation.
+  void OnSetLocalDescriptionComplete(RTCError error) override {
+    error_ = std::move(error);
+  }
+
+ private:
+  // Set on complete, on success this is set to an RTCError::OK() error.
+  absl::optional<RTCError> error_;
+};
+
+class FakeSetRemoteDescriptionObserver
     : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
  public:
   bool called() const { return error_.has_value(); }