Revert "SetRemoteDescriptionObserverInterface added."

This reverts commit 6c7ec32bd63ab2b45d4d83ae1de817ee946b4d72.

Reason for revert: Third party project breaks due to use-after-free
in the callback. I suspect this is because the adapter is processing
the async callback instead of the pc, i.e. callback is called from
SetRemoteDescriptionObserverAdapter::OnMessage instead of from
PeerConnection::OnMessage. This makes it possible for the callback to
be invoked after the PC is destroyed.

I argue this is how it should be done, and that if you're using a raw
pointer in an async callback you're doing it wrong, but I will reland
this CL with the callback processed in PeerConnection::OnMessage
instead as to not change the behavior of the old SRD signature.

Original change's description:
> SetRemoteDescriptionObserverInterface added.
> 
> The new observer replaced SetSessionDescriptionObserver for
> SetRemoteDescription. Unlike SetSessionDescriptionObserver,
> SetRemoteDescriptionObserverInterface is invoked synchronously so
> that the you can rely on the state of the PeerConnection to represent
> the result of the SetRemoteDescription call in the callback.
> 
> The new observer succeeds or fails with an RTCError.
> 
> This deprecates the need for PeerConnectionObserver::OnAdd/RemoveTrack
> and SetSessionDescriptionObserver, with the benefit that all media
> object changes can be processed in a single callback by the application
> in a synchronous callback. This will help Chromium keep objects in-sync
> across layers and threads in a non-racy and straight-forward way, see
> design doc (Proposal 2):
> https://docs.google.com/a/google.com/document/d/1-cDDC82mgU5zrHacfFz720p3xwRtuBkOPSRchh07Ho0/edit?usp=sharing
> 
> An adapter for SetSessionDescriptionObserver is added to allow calling
> the old SetRemoteDescription signature and get the old behavior
> (OnSuccess/OnFailure callback in a Post) until third parties switch.
> 
> Bug: webrtc:8473
> Change-Id: I3d4eb60da6dd34615f2c9f384aeaf4634e648c99
> Reviewed-on: https://webrtc-review.googlesource.com/17523
> Commit-Queue: Henrik Boström <hbos@webrtc.org>
> Reviewed-by: Peter Thatcher <pthatcher@webrtc.org>
> Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#20841}

TBR=hbos@webrtc.org,hta@webrtc.org,pthatcher@webrtc.org,guidou@webrtc.org

Change-Id: I715555e084d9aae49ee2a8831c70dc006dbdb74c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8473
Reviewed-on: https://webrtc-review.googlesource.com/25580
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#20850}

api/BUILD.gn

@@ -63,8 +63,6 @@ rtc_static_library("libjingle_peerconnection_api") {
     "rtpreceiverinterface.h",
     "rtpsenderinterface.h",
     "rtptransceiverinterface.h",
-    "setremotedescriptionobserverinterface.cc",
-    "setremotedescriptionobserverinterface.h",
     "statstypes.cc",
     "statstypes.h",
     "turncustomizer.h",
@@ -381,7 +379,6 @@ if (rtc_include_tests) {
     deps = [
       ":array_view",
       ":libjingle_peerconnection_api",
-      ":libjingle_peerconnection_test_api",
       ":optional",
       ":ortc_api",
       "../rtc_base:rtc_base_approved",

api/peerconnectioninterface.h

@@ -82,7 +82,6 @@
 #include "api/rtceventlogoutput.h"
 #include "api/rtpreceiverinterface.h"
 #include "api/rtpsenderinterface.h"
-#include "api/setremotedescriptionobserverinterface.h"
 #include "api/stats/rtcstatscollectorcallback.h"
 #include "api/statstypes.h"
 #include "api/turncustomizer.h"
@@ -727,18 +726,8 @@ class PeerConnectionInterface : public rtc::RefCountInterface {
   // Sets the remote session description.
   // The PeerConnection takes the ownership of |desc| even if it fails.
   // The |observer| callback will be called when done.
-  // TODO(hbos): Remove when Chrome implements the new signature.
   virtual void SetRemoteDescription(SetSessionDescriptionObserver* observer,
-                                    SessionDescriptionInterface* desc) {
+                                    SessionDescriptionInterface* desc) = 0;
-    SetRemoteDescription(
-        std::unique_ptr<SessionDescriptionInterface>(desc),
-        rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>(
-            new SetRemoteDescriptionObserverAdapter(observer)));
-  }
-  // TODO(hbos): Make pure virtual when Chrome has updated its signature.
-  virtual void SetRemoteDescription(
-      std::unique_ptr<SessionDescriptionInterface> desc,
-      rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer) {}
   // Deprecated; Replaced by SetConfiguration.
   // TODO(deadbeef): Remove once Chrome is moved over to SetConfiguration.
   virtual bool UpdateIce(const IceServers& configuration,

api/peerconnectionproxy.h

@@ -88,10 +88,6 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnection)
                 SetRemoteDescription,
                 SetSessionDescriptionObserver*,
                 SessionDescriptionInterface*)
-  PROXY_METHOD2(void,
-                SetRemoteDescription,
-                std::unique_ptr<SessionDescriptionInterface>,
-                rtc::scoped_refptr<SetRemoteDescriptionObserverInterface>);
   PROXY_METHOD0(PeerConnectionInterface::RTCConfiguration, GetConfiguration);
   PROXY_METHOD2(bool,
                 SetConfiguration,

api/setremotedescriptionobserverinterface.cc

@@ -1,61 +0,0 @@
-/*
- *  Copyright 2017 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.
- */
-
-#include "api/setremotedescriptionobserverinterface.h"
-
-#include <string>
-
-namespace webrtc {
-
-// The message keeps the observer alive through reference counting.
-class SetRemoteDescriptionObserverAdapter::MessageData
-    : public rtc::MessageData {
- public:
-  static MessageData* Create(
-      rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer,
-      RTCError error) {
-    return new MessageData(std::move(observer), std::move(error));
-  }
-
-  const RTCError& error() const { return error_; }
-
- private:
-  MessageData(rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer,
-              RTCError error)
-      : observer_(std::move(observer)), error_(std::move(error)) {}
-
-  rtc::scoped_refptr<SetRemoteDescriptionObserverAdapter> observer_;
-  RTCError error_;
-};
-
-SetRemoteDescriptionObserverAdapter::SetRemoteDescriptionObserverAdapter(
-    rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper)
-    : wrapper_(std::move(wrapper)) {}
-
-void SetRemoteDescriptionObserverAdapter::OnSetRemoteDescriptionComplete(
-    RTCError error) {
-  // MessageData keeps a reference to |this|, ensuring it is not deleted until
-  // OnMessage().
-  rtc::Thread::Current()->Post(RTC_FROM_HERE, this, 0,
-                               MessageData::Create(this, std::move(error)));
-}
-
-void SetRemoteDescriptionObserverAdapter::OnMessage(rtc::Message* msg) {
-  MessageData* message = static_cast<MessageData*>(msg->pdata);
-  if (message->error().ok())
-    wrapper_->OnSuccess();
-  else
-    wrapper_->OnFailure(message->error().message());
-  // Delete the message data, this may delete |this|. Don't use member variables
-  // after this line.
-  delete message;
-}
-
-}  // namespace webrtc

api/setremotedescriptionobserverinterface.h

@@ -1,69 +0,0 @@
-/*
- *  Copyright 2017 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_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_
-#define API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_
-
-#include <utility>
-#include <vector>
-
-#include "api/jsep.h"
-#include "api/mediastreaminterface.h"
-#include "api/rtcerror.h"
-#include "api/rtpreceiverinterface.h"
-#include "rtc_base/bind.h"
-#include "rtc_base/messagehandler.h"
-#include "rtc_base/refcount.h"
-#include "rtc_base/refcountedobject.h"
-#include "rtc_base/scoped_ref_ptr.h"
-#include "rtc_base/thread.h"
-
-namespace webrtc {
-
-// An observer for PeerConnectionInterface::SetRemoteDescription(). The
-// callback is invoked such that the state of the peer connection can be
-// examined to accurately reflect the effects of the SetRemoteDescription
-// operation.
-class SetRemoteDescriptionObserverInterface : public rtc::RefCountInterface {
- public:
-  // On success, |error.ok()| is true.
-  virtual void OnSetRemoteDescriptionComplete(RTCError error) = 0;
-};
-
-// Upon completion, posts a task to execute the callback of the
-// SetSessionDescriptionObserver asynchronously on the same thread. At this
-// point, the state of the peer connection might no longer reflect the effects
-// of the SetRemoteDescription operation, as the peer connection could have been
-// modified during the post.
-// TODO(hbos): Remove this class once we remove the version of
-// PeerConnectionInterface::SetRemoteDescription() that takes a
-// SetSessionDescriptionObserver as an argument.
-class SetRemoteDescriptionObserverAdapter
-    : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface>,
-      public rtc::MessageHandler {
- public:
-  SetRemoteDescriptionObserverAdapter(
-      rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper);
-
-  // SetRemoteDescriptionObserverInterface implementation.
-  void OnSetRemoteDescriptionComplete(RTCError error) override;
-
-  // rtc::MessageHandler implementation.
-  void OnMessage(rtc::Message* msg) override;
-
- private:
-  class MessageData;
-
-  rtc::scoped_refptr<SetSessionDescriptionObserver> wrapper_;
-};
-
-}  // namespace webrtc
-
-#endif  // API_SETREMOTEDESCRIPTIONOBSERVERINTERFACE_H_

pc/BUILD.gn

@@ -402,7 +402,6 @@ if (rtc_include_tests) {
       "rtcstatscollector_unittest.cc",
       "rtpsenderreceiver_unittest.cc",
       "sctputils_unittest.cc",
-      "setremotedescriptionobserverinterface_unittest.cc",
       "statscollector_unittest.cc",
       "test/fakeaudiocapturemodule_unittest.cc",
       "test/testsdpstrings.h",
@@ -444,7 +443,6 @@ if (rtc_include_tests) {
       "..:webrtc_common",
       "../api:fakemetricsobserver",
       "../api:libjingle_peerconnection_test_api",
-      "../api:optional",
       "../api:rtc_stats_api",
       "../api/audio_codecs:audio_codecs_api",
       "../api/audio_codecs:builtin_audio_decoder_factory",

pc/peerconnection.cc

@@ -1463,7 +1463,7 @@ void PeerConnection::SetLocalDescription(
   std::string error;
   // Takes the ownership of |desc_temp|. On success, local_description() is
   // updated to reflect the description that was passed in.
-  if (!SetCurrentOrPendingLocalDescription(std::move(desc_temp), &error)) {
+  if (!SetLocalDescription(std::move(desc_temp), &error)) {
     PostSetSessionDescriptionFailure(observer, error);
     return;
   }
@@ -1541,25 +1541,26 @@ void PeerConnection::SetLocalDescription(
 }
 
 void PeerConnection::SetRemoteDescription(
-    std::unique_ptr<SessionDescriptionInterface> desc,
+    SetSessionDescriptionObserver* observer,
-    rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer) {
+    SessionDescriptionInterface* desc) {
   TRACE_EVENT0("webrtc", "PeerConnection::SetRemoteDescription");
   if (!observer) {
     RTC_LOG(LS_ERROR) << "SetRemoteDescription - observer is NULL.";
     return;
   }
   if (!desc) {
-    observer->OnSetRemoteDescriptionComplete(RTCError(
+    PostSetSessionDescriptionFailure(observer, "SessionDescription is NULL.");
-        RTCErrorType::UNSUPPORTED_PARAMETER, "SessionDescription is NULL."));
     return;
   }
 
+  // Takes the ownership of |desc| regardless of the result.
+  std::unique_ptr<SessionDescriptionInterface> desc_temp(desc);
+
   if (IsClosed()) {
-    std::string error = "Failed to set remote " + desc->type() +
+    std::string error = "Failed to set remote " + desc_temp->type() +
                         " sdp: Called in wrong state: STATE_CLOSED";
     RTC_LOG(LS_ERROR) << error;
-    observer->OnSetRemoteDescriptionComplete(
+    PostSetSessionDescriptionFailure(observer, error);
-        RTCError(RTCErrorType::INVALID_STATE, std::move(error)));
     return;
   }
 
@@ -1567,11 +1568,10 @@ void PeerConnection::SetRemoteDescription(
   // streams that might be removed by updating the session description.
   stats_->UpdateStats(kStatsOutputLevelStandard);
   std::string error;
-  // Takes the ownership of |desc|. On success, remote_description() is updated
+  // Takes the ownership of |desc_temp|. On success, remote_description() is
-  // to reflect the description that was passed in.
+  // updated to reflect the description that was passed in.
-  if (!SetCurrentOrPendingRemoteDescription(std::move(desc), &error)) {
+  if (!SetRemoteDescription(std::move(desc_temp), &error)) {
-    observer->OnSetRemoteDescriptionComplete(
+    PostSetSessionDescriptionFailure(observer, error);
-        RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, std::move(error)));
     return;
   }
   RTC_DCHECK(remote_description());
@@ -1661,7 +1661,9 @@ void PeerConnection::SetRemoteDescription(
 
   UpdateEndedRemoteMediaStreams();
 
-  observer->OnSetRemoteDescriptionComplete(RTCError::OK());
+  SetSessionDescriptionMsg* msg = new SetSessionDescriptionMsg(observer);
+  signaling_thread()->Post(RTC_FROM_HERE, this,
+                           MSG_SET_SESSIONDESCRIPTION_SUCCESS, msg);
 
   if (remote_description()->type() == SessionDescriptionInterface::kAnswer) {
     // TODO(deadbeef): We already had to hop to the network thread for
@@ -3258,7 +3260,7 @@ bool PeerConnection::GetSslRole(const std::string& content_name,
                                            role);
 }
 
-bool PeerConnection::SetCurrentOrPendingLocalDescription(
+bool PeerConnection::SetLocalDescription(
     std::unique_ptr<SessionDescriptionInterface> desc,
     std::string* err_desc) {
   RTC_DCHECK(signaling_thread()->IsCurrent());
@@ -3314,7 +3316,7 @@ bool PeerConnection::SetCurrentOrPendingLocalDescription(
   return true;
 }
 
-bool PeerConnection::SetCurrentOrPendingRemoteDescription(
+bool PeerConnection::SetRemoteDescription(
     std::unique_ptr<SessionDescriptionInterface> desc,
     std::string* err_desc) {
   RTC_DCHECK(signaling_thread()->IsCurrent());

pc/peerconnection.h

@@ -150,10 +150,8 @@ class PeerConnection : public PeerConnectionInterface,
                     const RTCOfferAnswerOptions& options) override;
   void SetLocalDescription(SetSessionDescriptionObserver* observer,
                            SessionDescriptionInterface* desc) override;
-  void SetRemoteDescription(
+  void SetRemoteDescription(SetSessionDescriptionObserver* observer,
-      std::unique_ptr<SessionDescriptionInterface> desc,
+                            SessionDescriptionInterface* desc) override;
-      rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> observer)
-      override;
   PeerConnectionInterface::RTCConfiguration GetConfiguration() override;
   bool SetConfiguration(
       const PeerConnectionInterface::RTCConfiguration& configuration,
@@ -541,15 +539,10 @@ class PeerConnection : public PeerConnectionInterface,
   // Get current SSL role used by SCTP's underlying transport.
   bool GetSctpSslRole(rtc::SSLRole* role);
 
-  // Validates and takes ownership of the description, setting it as the current
+  bool SetLocalDescription(std::unique_ptr<SessionDescriptionInterface> desc,
-  // or pending description (depending on the description's action) if it is
+                           std::string* err_desc);
-  // valid. Also updates ice role, candidates, creates and destroys channels.
+  bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> desc,
-  bool SetCurrentOrPendingLocalDescription(
+                            std::string* err_desc);
-      std::unique_ptr<SessionDescriptionInterface> desc,
-      std::string* err_desc);
-  bool SetCurrentOrPendingRemoteDescription(
-      std::unique_ptr<SessionDescriptionInterface> desc,
-      std::string* err_desc);
 
   cricket::IceConfig ParseIceConfig(
       const PeerConnectionInterface::RTCConfiguration& config) const;

pc/peerconnection_rtp_unittest.cc

@@ -33,25 +33,6 @@
 
 namespace {
 
-const uint32_t kDefaultTimeout = 10000u;
-
-template <typename MethodFunctor>
-class OnSuccessObserver : public rtc::RefCountedObject<
-                              webrtc::SetRemoteDescriptionObserverInterface> {
- public:
-  explicit OnSuccessObserver(MethodFunctor on_success)
-      : on_success_(std::move(on_success)) {}
-
-  // webrtc::SetRemoteDescriptionObserverInterface implementation.
-  void OnSetRemoteDescriptionComplete(webrtc::RTCError error) override {
-    RTC_CHECK(error.ok());
-    on_success_();
-  }
-
- private:
-  MethodFunctor on_success_;
-};
-
 class PeerConnectionRtpTest : public testing::Test {
  public:
   PeerConnectionRtpTest()
@@ -79,31 +60,25 @@ class PeerConnectionRtpTest : public testing::Test {
   rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> pc_factory_;
 };
 
-// These tests cover |webrtc::PeerConnectionObserver| callbacks firing upon
+TEST_F(PeerConnectionRtpTest, AddTrackWithoutStreamFiresOnAddTrack) {
-// setting the remote description.
-class PeerConnectionRtpCallbacksTest : public PeerConnectionRtpTest {};
-
-TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
   rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
       pc_factory_->CreateAudioTrack("audio_track", nullptr));
   EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {}));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
 
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
-  // TODO(hbos): When "no stream" is handled correctly we would expect
+  // TODO(deadbeef): When no stream is handled correctly we would expect
   // |add_track_events_[0].streams| to be empty. https://crbug.com/webrtc/7933
   auto& add_track_event = callee->observer()->add_track_events_[0];
-  ASSERT_EQ(add_track_event.streams.size(), 1u);
+  ASSERT_EQ(1u, add_track_event.streams.size());
   EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track"));
   EXPECT_EQ(add_track_event.streams, add_track_event.receiver->streams());
 }
 
-TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithStreamFiresOnAddTrack) {
+TEST_F(PeerConnectionRtpTest, AddTrackWithStreamFiresOnAddTrack) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -111,42 +86,34 @@ TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithStreamFiresOnAddTrack) {
       pc_factory_->CreateAudioTrack("audio_track", nullptr));
   auto stream = webrtc::MediaStream::Create("audio_stream");
   EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {stream.get()}));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
 
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
   auto& add_track_event = callee->observer()->add_track_events_[0];
-  ASSERT_EQ(add_track_event.streams.size(), 1u);
+  ASSERT_EQ(1u, add_track_event.streams.size());
   EXPECT_EQ("audio_stream", add_track_event.streams[0]->label());
   EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track"));
   EXPECT_EQ(add_track_event.streams, add_track_event.receiver->streams());
 }
 
-TEST_F(PeerConnectionRtpCallbacksTest,
+TEST_F(PeerConnectionRtpTest, RemoveTrackWithoutStreamFiresOnRemoveTrack) {
-       RemoveTrackWithoutStreamFiresOnRemoveTrack) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
   rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
       pc_factory_->CreateAudioTrack("audio_track", nullptr));
   auto sender = caller->pc()->AddTrack(audio_track.get(), {});
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
   EXPECT_TRUE(caller->pc()->RemoveTrack(sender));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
 
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
   EXPECT_EQ(callee->observer()->GetAddTrackReceivers(),
             callee->observer()->remove_track_events_);
 }
 
-TEST_F(PeerConnectionRtpCallbacksTest,
+TEST_F(PeerConnectionRtpTest, RemoveTrackWithStreamFiresOnRemoveTrack) {
-       RemoveTrackWithStreamFiresOnRemoveTrack) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -154,22 +121,17 @@ TEST_F(PeerConnectionRtpCallbacksTest,
       pc_factory_->CreateAudioTrack("audio_track", nullptr));
   auto stream = webrtc::MediaStream::Create("audio_stream");
   auto sender = caller->pc()->AddTrack(audio_track.get(), {stream.get()});
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
   EXPECT_TRUE(caller->pc()->RemoveTrack(sender));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
 
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u);
+  ASSERT_EQ(1u, callee->observer()->add_track_events_.size());
   EXPECT_EQ(callee->observer()->GetAddTrackReceivers(),
             callee->observer()->remove_track_events_);
 }
 
-TEST_F(PeerConnectionRtpCallbacksTest,
+TEST_F(PeerConnectionRtpTest, RemoveTrackWithSharedStreamFiresOnRemoveTrack) {
-       RemoveTrackWithSharedStreamFiresOnRemoveTrack) {
   auto caller = CreatePeerConnection();
   auto callee = CreatePeerConnection();
 
@@ -181,18 +143,14 @@ TEST_F(PeerConnectionRtpCallbacksTest,
   std::vector<webrtc::MediaStreamInterface*> streams{stream.get()};
   auto sender1 = caller->pc()->AddTrack(audio_track1.get(), streams);
   auto sender2 = caller->pc()->AddTrack(audio_track2.get(), streams);
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
 
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u);
+  ASSERT_EQ(2u, callee->observer()->add_track_events_.size());
 
   // Remove "audio_track1".
   EXPECT_TRUE(caller->pc()->RemoveTrack(sender1));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
+  ASSERT_EQ(2u, callee->observer()->add_track_events_.size());
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u);
   EXPECT_EQ(
       std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{
           callee->observer()->add_track_events_[0].receiver},
@@ -200,211 +158,10 @@ TEST_F(PeerConnectionRtpCallbacksTest,
 
   // Remove "audio_track2".
   EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
-  ASSERT_TRUE(
+  ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
+  ASSERT_EQ(2u, callee->observer()->add_track_events_.size());
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u);
   EXPECT_EQ(callee->observer()->GetAddTrackReceivers(),
             callee->observer()->remove_track_events_);
 }
 
-// These tests examine the state of the peer connection as a result of
-// performing SetRemoteDescription().
-class PeerConnectionRtpObserverTest : public PeerConnectionRtpTest {};
-
-TEST_F(PeerConnectionRtpObserverTest, AddSenderWithoutStreamAddsReceiver) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
-      pc_factory_->CreateAudioTrack("audio_track", nullptr));
-  EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {}));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-
-  EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u);
-  auto receiver_added = callee->pc()->GetReceivers()[0];
-  EXPECT_EQ("audio_track", receiver_added->track()->id());
-  // TODO(hbos): When "no stream" is handled correctly we would expect
-  // |receiver_added->streams()| to be empty. https://crbug.com/webrtc/7933
-  EXPECT_EQ(receiver_added->streams().size(), 1u);
-  EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track"));
-}
-
-TEST_F(PeerConnectionRtpObserverTest, AddSenderWithStreamAddsReceiver) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
-      pc_factory_->CreateAudioTrack("audio_track", nullptr));
-  auto stream = webrtc::MediaStream::Create("audio_stream");
-  EXPECT_TRUE(caller->pc()->AddTrack(audio_track.get(), {stream}));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-
-  EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u);
-  auto receiver_added = callee->pc()->GetReceivers()[0];
-  EXPECT_EQ("audio_track", receiver_added->track()->id());
-  EXPECT_EQ(receiver_added->streams().size(), 1u);
-  EXPECT_EQ("audio_stream", receiver_added->streams()[0]->label());
-  EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track"));
-}
-
-TEST_F(PeerConnectionRtpObserverTest,
-       RemoveSenderWithoutStreamRemovesReceiver) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
-      pc_factory_->CreateAudioTrack("audio_track", nullptr));
-  auto sender = caller->pc()->AddTrack(audio_track.get(), {});
-  ASSERT_TRUE(sender);
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u);
-  auto receiver = callee->pc()->GetReceivers()[0];
-  ASSERT_TRUE(caller->pc()->RemoveTrack(sender));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-
-  // TODO(hbos): When we implement Unified Plan, receivers will not be removed.
-  // Instead, the transceiver owning the receiver will become inactive.
-  EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u);
-}
-
-TEST_F(PeerConnectionRtpObserverTest, RemoveSenderWithStreamRemovesReceiver) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
-      pc_factory_->CreateAudioTrack("audio_track", nullptr));
-  auto stream = webrtc::MediaStream::Create("audio_stream");
-  auto sender = caller->pc()->AddTrack(audio_track.get(), {stream});
-  ASSERT_TRUE(sender);
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u);
-  auto receiver = callee->pc()->GetReceivers()[0];
-  ASSERT_TRUE(caller->pc()->RemoveTrack(sender));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-
-  // TODO(hbos): When we implement Unified Plan, receivers will not be removed.
-  // Instead, the transceiver owning the receiver will become inactive.
-  EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u);
-}
-
-TEST_F(PeerConnectionRtpObserverTest,
-       RemoveSenderWithSharedStreamRemovesReceiver) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track1(
-      pc_factory_->CreateAudioTrack("audio_track1", nullptr));
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track2(
-      pc_factory_->CreateAudioTrack("audio_track2", nullptr));
-  auto stream = webrtc::MediaStream::Create("shared_audio_stream");
-  std::vector<webrtc::MediaStreamInterface*> streams{stream.get()};
-  auto sender1 = caller->pc()->AddTrack(audio_track1.get(), streams);
-  auto sender2 = caller->pc()->AddTrack(audio_track2.get(), streams);
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-
-  ASSERT_EQ(callee->pc()->GetReceivers().size(), 2u);
-  rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver1;
-  rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver2;
-  if (callee->pc()->GetReceivers()[0]->track()->id() == "audio_track1") {
-    receiver1 = callee->pc()->GetReceivers()[0];
-    receiver2 = callee->pc()->GetReceivers()[1];
-  } else {
-    receiver1 = callee->pc()->GetReceivers()[1];
-    receiver2 = callee->pc()->GetReceivers()[0];
-  }
-  EXPECT_EQ("audio_track1", receiver1->track()->id());
-  EXPECT_EQ("audio_track2", receiver2->track()->id());
-
-  // Remove "audio_track1".
-  EXPECT_TRUE(caller->pc()->RemoveTrack(sender1));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  // Only |receiver2| should remain.
-  // TODO(hbos): When we implement Unified Plan, receivers will not be removed.
-  // Instead, the transceiver owning the receiver will become inactive.
-  EXPECT_EQ(
-      std::vector<rtc::scoped_refptr<webrtc::RtpReceiverInterface>>{receiver2},
-      callee->pc()->GetReceivers());
-
-  // Remove "audio_track2".
-  EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
-  ASSERT_TRUE(
-      callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
-                                   static_cast<webrtc::RTCError*>(nullptr)));
-  // TODO(hbos): When we implement Unified Plan, receivers will not be removed.
-  // Instead, the transceiver owning the receiver will become inactive.
-  EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u);
-}
-
-// Invokes SetRemoteDescription() twice in a row without synchronizing the two
-// calls and examine the state of the peer connection inside the callbacks to
-// ensure that the second call does not occur prematurely, contaminating the
-// state of the peer connection of the first callback.
-TEST_F(PeerConnectionRtpObserverTest,
-       StatesCorrelateWithSetRemoteDescriptionCall) {
-  auto caller = CreatePeerConnection();
-  auto callee = CreatePeerConnection();
-
-  rtc::scoped_refptr<webrtc::AudioTrackInterface> audio_track(
-      pc_factory_->CreateAudioTrack("audio_track", nullptr));
-  // Create SDP for adding a track and for removing it. This will be used in the
-  // first and second SetRemoteDescription() calls.
-  auto sender = caller->pc()->AddTrack(audio_track.get(), {});
-  auto srd1_sdp = caller->CreateOfferAndSetAsLocal();
-  EXPECT_TRUE(caller->pc()->RemoveTrack(sender));
-  auto srd2_sdp = caller->CreateOfferAndSetAsLocal();
-
-  // In the first SetRemoteDescription() callback, check that we have a
-  // receiver for the track.
-  auto pc = callee->pc();
-  bool srd1_callback_called = false;
-  auto srd1_callback = [&srd1_callback_called, &pc]() {
-    EXPECT_EQ(pc->GetReceivers().size(), 1u);
-    srd1_callback_called = true;
-  };
-
-  // In the second SetRemoteDescription() callback, check that the receiver has
-  // been removed.
-  // TODO(hbos): When we implement Unified Plan, receivers will not be removed.
-  // Instead, the transceiver owning the receiver will become inactive.
-  // https://crbug.com/webrtc/7600
-  bool srd2_callback_called = false;
-  auto srd2_callback = [&srd2_callback_called, &pc]() {
-    EXPECT_TRUE(pc->GetReceivers().empty());
-    srd2_callback_called = true;
-  };
-
-  // Invoke SetRemoteDescription() twice in a row without synchronizing the two
-  // calls. The callbacks verify that the two calls are synchronized, as in, the
-  // effects of the second SetRemoteDescription() call must not have happened by
-  // the time the first callback is invoked. If it has then the receiver that is
-  // added as a result of the first SetRemoteDescription() call will already
-  // have been removed as a result of the second SetRemoteDescription() call
-  // when the first callback is invoked.
-  callee->pc()->SetRemoteDescription(
-      std::move(srd1_sdp),
-      new OnSuccessObserver<decltype(srd1_callback)>(srd1_callback));
-  callee->pc()->SetRemoteDescription(
-      std::move(srd2_sdp),
-      new OnSuccessObserver<decltype(srd2_callback)>(srd2_callback));
-  EXPECT_TRUE_WAIT(srd1_callback_called, kDefaultTimeout);
-  EXPECT_TRUE_WAIT(srd2_callback_called, kDefaultTimeout);
-}
-
 }  // namespace

pc/peerconnectionwrapper.cc

@@ -153,19 +153,6 @@ bool PeerConnectionWrapper::SetRemoteDescription(
       error_out);
 }
 
-bool PeerConnectionWrapper::SetRemoteDescription(
-    std::unique_ptr<SessionDescriptionInterface> desc,
-    RTCError* error_out) {
-  rtc::scoped_refptr<MockSetRemoteDescriptionObserver> observer =
-      new MockSetRemoteDescriptionObserver();
-  pc()->SetRemoteDescription(std::move(desc), observer);
-  EXPECT_EQ_WAIT(true, observer->called(), kDefaultTimeout);
-  bool ok = observer->error().ok();
-  if (error_out)
-    *error_out = std::move(observer->error());
-  return ok;
-}
-
 bool PeerConnectionWrapper::SetSdp(
     rtc::FunctionView<void(SetSessionDescriptionObserver*)> fn,
     std::string* error_out) {

pc/peerconnectionwrapper.h

@@ -90,8 +90,6 @@ class PeerConnectionWrapper {
   // Returns true if the description was successfully set.
   bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> desc,
                             std::string* error_out = nullptr);
-  bool SetRemoteDescription(std::unique_ptr<SessionDescriptionInterface> desc,
-                            RTCError* error_out);
 
   // Calls the underlying PeerConnection's AddTrack method with an audio media
   // stream track not bound to any source.

pc/setremotedescriptionobserverinterface_unittest.cc

@@ -1,75 +0,0 @@
-/*
- *  Copyright 2017 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.
- */
-
-#include "api/setremotedescriptionobserverinterface.h"
-
-#include <string>
-
-#include "api/jsep.h"
-#include "api/optional.h"
-#include "api/rtcerror.h"
-#include "pc/test/mockpeerconnectionobservers.h"
-#include "rtc_base/checks.h"
-#include "rtc_base/gunit.h"
-#include "rtc_base/scoped_ref_ptr.h"
-#include "rtc_base/thread.h"
-
-// TODO(hbos): This is a test for api/setremotedescriptionobserverinterface.h
-// and should be in api/ instead of pc/, but the dependency on
-// pc/test/mockpeerconnectionobservers.h and rtc_base/thread.h is not allowed
-// from api:rtc_api_unittests.
-
-const int kDefaultTimeoutMs = 1000;
-
-class SetRemoteDescriptionObserverWrapperTest : public testing::Test {
- public:
-  SetRemoteDescriptionObserverWrapperTest()
-      : set_desc_observer_(new rtc::RefCountedObject<
-                           webrtc::MockSetSessionDescriptionObserver>()),
-        observer_(new webrtc::SetRemoteDescriptionObserverAdapter(
-            set_desc_observer_)) {}
-
- protected:
-  rtc::scoped_refptr<webrtc::MockSetSessionDescriptionObserver>
-      set_desc_observer_;
-  rtc::scoped_refptr<webrtc::SetRemoteDescriptionObserverAdapter> observer_;
-};
-
-TEST_F(SetRemoteDescriptionObserverWrapperTest, OnCompleteWithSuccess) {
-  observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK());
-  EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs);
-  EXPECT_TRUE(set_desc_observer_->result());
-}
-
-TEST_F(SetRemoteDescriptionObserverWrapperTest, OnCompleteWithFailure) {
-  observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError(
-      webrtc::RTCErrorType::INVALID_PARAMETER, "FailureMessage"));
-  EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs);
-  EXPECT_FALSE(set_desc_observer_->result());
-  EXPECT_EQ(set_desc_observer_->error(), "FailureMessage");
-}
-
-TEST_F(SetRemoteDescriptionObserverWrapperTest, IsAsynchronous) {
-  observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK());
-  // Untill this thread's messages are processed by EXPECT_TRUE_WAIT,
-  // |set_desc_observer_| should not have been called.
-  EXPECT_FALSE(set_desc_observer_->called());
-  EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs);
-  EXPECT_TRUE(set_desc_observer_->result());
-}
-
-TEST_F(SetRemoteDescriptionObserverWrapperTest, SurvivesDereferencing) {
-  observer_->OnSetRemoteDescriptionComplete(webrtc::RTCError::OK());
-  // Even if there are no external references to |observer_| the operation
-  // should complete.
-  observer_ = nullptr;
-  EXPECT_TRUE_WAIT(set_desc_observer_->called(), kDefaultTimeoutMs);
-  EXPECT_TRUE(set_desc_observer_->result());
-}

pc/test/mockpeerconnectionobservers.h

@@ -213,12 +213,12 @@ class MockSetSessionDescriptionObserver
   MockSetSessionDescriptionObserver()
       : called_(false),
         error_("MockSetSessionDescriptionObserver not called") {}
-  ~MockSetSessionDescriptionObserver() override {}
+  virtual ~MockSetSessionDescriptionObserver() {}
-  void OnSuccess() override {
+  virtual void OnSuccess() {
     called_ = true;
     error_ = "";
   }
-  void OnFailure(const std::string& error) override {
+  virtual void OnFailure(const std::string& error) {
     called_ = true;
     error_ = error;
   }
@@ -231,25 +231,6 @@ class MockSetSessionDescriptionObserver
   std::string error_;
 };
 
-class MockSetRemoteDescriptionObserver
-    : public rtc::RefCountedObject<SetRemoteDescriptionObserverInterface> {
- public:
-  bool called() const { return error_.has_value(); }
-  RTCError& error() {
-    RTC_DCHECK(error_.has_value());
-    return *error_;
-  }
-
-  // SetRemoteDescriptionObserverInterface implementation.
-  void OnSetRemoteDescriptionComplete(RTCError error) override {
-    error_ = std::move(error);
-  }
-
- private:
-  // Set on complete, on success this is set to an RTCError::OK() error.
-  rtc::Optional<RTCError> error_;
-};
-
 class MockDataChannelObserver : public webrtc::DataChannelObserver {
  public:
   explicit MockDataChannelObserver(webrtc::DataChannelInterface* channel)