Fire RTCPeerConnection.onsignalingstatechange in the correct order.

With this change, onsignalingstatechange always fires before the
setLocalDescription's promise resolves.

Before this change, signaling state changes were surfaced with callbacks
firing separately from the callback that setLocalDescription or
setRemoteDescription had completed. As a result, the result of a SLD/SRD
operation was surfaced in two PostTasks instead of one. This could lead
to flaky behavior where JavaScript task execeution cycles may or may not
execute between PostTasks which lead to undefined order. Also when the
signalingstatechange was surfaced to the JS thread the firing of the
event was delayed by a task execution cycle due to
ScheduleDispatchEvent.
- If both PostTasks were handled in the same cycle, the event firing
  would be scheduled and the promise resolved, such that the promise's
  then() is invoked just before the event fires (scheduled after
  promise). This is NOT in accordance with the spec.
- If onsignalingstatechange's PostTask was handled in its own cycle,
  the event would be scheduled such that by the time the
  setLocalDescription's callback's PostTask occurs resolving the
  promise, the event would fire before the promise's then(). This IS
  in accordance with the spec.

With this change, the onsignalingstatechange event is fired without
delay, and it is processed in by the same observer that handles the
SLD/SRD callback, so the order is always guaranteed to be the same.

In addition, this CL removes the usage of
WebRTCPeerConnectionHandlerClient::SignalingState and relies on the
webrtc::PeerConnectionInterface::SignalingState enum directly. This
is part of the Onion Souping work (crbug.com/787254).

Bug: 848768, 787254
Change-Id: I7bc10439c44c6ff6beab0289975295ff2e051b9c
Reviewed-on: https://chromium-review.googlesource.com/1085302
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Guido Urdaneta <guidou@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565291}

content/renderer/media/webrtc/mock_web_rtc_peer_connection_handler_client.h

@@ -27,7 +27,8 @@ class MockWebRTCPeerConnectionHandlerClient
   MOCK_METHOD0(NegotiationNeeded, void());
   MOCK_METHOD1(DidGenerateICECandidate,
                void(scoped_refptr<blink::WebRTCICECandidate> candidate));
-  MOCK_METHOD1(DidChangeSignalingState, void(SignalingState state));
+  MOCK_METHOD1(DidChangeSignalingState,
+               void(webrtc::PeerConnectionInterface::SignalingState state));
   MOCK_METHOD1(DidChangeICEGatheringState, void(ICEGatheringState state));
   MOCK_METHOD1(DidChangeICEConnectionState, void(ICEConnectionState state));
   void DidAddRemoteTrack(

content/renderer/media/webrtc/peer_connection_tracker.cc

@@ -215,15 +215,21 @@ static std::string SerializeConfiguration(
 // strings on chrome://webrtc-internals.
 
 static const char* GetSignalingStateString(
-    WebRTCPeerConnectionHandlerClient::SignalingState state) {
+    webrtc::PeerConnectionInterface::SignalingState state) {
   const char* result = "";
   switch (state) {
-    GET_STRING_OF_STATE(SignalingStateStable)
-    GET_STRING_OF_STATE(SignalingStateHaveLocalOffer)
-    GET_STRING_OF_STATE(SignalingStateHaveRemoteOffer)
-    GET_STRING_OF_STATE(SignalingStateHaveLocalPrAnswer)
-    GET_STRING_OF_STATE(SignalingStateHaveRemotePrAnswer)
-    GET_STRING_OF_STATE(SignalingStateClosed)
+    case webrtc::PeerConnectionInterface::SignalingState::kStable:
+      return "SignalingStateStable";
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveLocalOffer:
+      return "SignalingStateHaveLocalOffer";
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveRemoteOffer:
+      return "SignalingStateHaveRemoteOffer";
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveLocalPrAnswer:
+      return "SignalingStateHaveLocalPrAnswer";
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveRemotePrAnswer:
+      return "SignalingStateHaveRemotePrAnswer";
+    case webrtc::PeerConnectionInterface::SignalingState::kClosed:
+      return "SignalingStateClosed";
     default:
       NOTREACHED();
       break;
@@ -663,8 +669,8 @@ void PeerConnectionTracker::TrackStop(RTCPeerConnectionHandler* pc_handler) {
 }
 
 void PeerConnectionTracker::TrackSignalingStateChange(
-      RTCPeerConnectionHandler* pc_handler,
-      WebRTCPeerConnectionHandlerClient::SignalingState state) {
+    RTCPeerConnectionHandler* pc_handler,
+    webrtc::PeerConnectionInterface::SignalingState state) {
   DCHECK(main_thread_.CalledOnValidThread());
   int id = GetLocalIDForHandler(pc_handler);
   if (id == -1)

content/renderer/media/webrtc/peer_connection_tracker.h

@@ -137,7 +137,7 @@ class CONTENT_EXPORT PeerConnectionTracker
   // Sends an update when the signaling state of a PeerConnection has changed.
   virtual void TrackSignalingStateChange(
       RTCPeerConnectionHandler* pc_handler,
-      blink::WebRTCPeerConnectionHandlerClient::SignalingState state);
+      webrtc::PeerConnectionInterface::SignalingState state);
 
   // Sends an update when the Ice connection state
   // of a PeerConnection has changed.

content/renderer/media/webrtc/rtc_peer_connection_handler.cc

@@ -119,30 +119,6 @@ GetWebKitIceConnectionState(
   }
 }
 
-blink::WebRTCPeerConnectionHandlerClient::SignalingState
-GetWebKitSignalingState(webrtc::PeerConnectionInterface::SignalingState state) {
-  using blink::WebRTCPeerConnectionHandlerClient;
-  switch (state) {
-    case webrtc::PeerConnectionInterface::kStable:
-      return WebRTCPeerConnectionHandlerClient::kSignalingStateStable;
-    case webrtc::PeerConnectionInterface::kHaveLocalOffer:
-      return WebRTCPeerConnectionHandlerClient::kSignalingStateHaveLocalOffer;
-    case webrtc::PeerConnectionInterface::kHaveLocalPrAnswer:
-      return WebRTCPeerConnectionHandlerClient::
-          kSignalingStateHaveLocalPrAnswer;
-    case webrtc::PeerConnectionInterface::kHaveRemoteOffer:
-      return WebRTCPeerConnectionHandlerClient::kSignalingStateHaveRemoteOffer;
-    case webrtc::PeerConnectionInterface::kHaveRemotePrAnswer:
-      return WebRTCPeerConnectionHandlerClient::
-          kSignalingStateHaveRemotePrAnswer;
-    case webrtc::PeerConnectionInterface::kClosed:
-      return WebRTCPeerConnectionHandlerClient::kSignalingStateClosed;
-    default:
-      NOTREACHED();
-      return WebRTCPeerConnectionHandlerClient::kSignalingStateClosed;
-  }
-}
-
 blink::WebRTCSessionDescription CreateWebKitSessionDescription(
     const std::string& sdp, const std::string& type) {
   blink::WebRTCSessionDescription description;
@@ -433,61 +409,6 @@ class CreateSessionDescriptionRequest
   SessionDescriptionRequestTracker tracker_;
 };
 
-// Class mapping responses from calls to libjingle SetLocalDescription and a
-// blink::WebRTCVoidRequest.
-class SetLocalDescriptionRequest
-    : public webrtc::SetSessionDescriptionObserver {
- public:
-  explicit SetLocalDescriptionRequest(
-      const scoped_refptr<base::SingleThreadTaskRunner>& main_thread,
-      const blink::WebRTCVoidRequest& request,
-      const base::WeakPtr<RTCPeerConnectionHandler>& handler,
-      const base::WeakPtr<PeerConnectionTracker>& tracker,
-      PeerConnectionTracker::Action action)
-      : main_thread_(main_thread),
-        webkit_request_(request),
-        tracker_(handler, tracker, action) {}
-
-  void OnSuccess() override {
-    if (!main_thread_->BelongsToCurrentThread()) {
-      main_thread_->PostTask(
-          FROM_HERE,
-          base::BindOnce(&SetLocalDescriptionRequest::OnSuccess, this));
-      return;
-    }
-    tracker_.TrackOnSuccess(nullptr);
-    webkit_request_.RequestSucceeded();
-    webkit_request_.Reset();
-  }
-  void OnFailure(webrtc::RTCError error) override {
-    if (!main_thread_->BelongsToCurrentThread()) {
-      main_thread_->PostTask(
-          FROM_HERE, base::BindOnce(&SetLocalDescriptionRequest::OnFailure,
-                                    this, std::move(error)));
-      return;
-    }
-    tracker_.TrackOnFailure(error);
-    webkit_request_.RequestFailed(error);
-    webkit_request_.Reset();
-  }
-
- protected:
-  ~SetLocalDescriptionRequest() override {
-    // This object is reference counted and its callback methods |OnSuccess| and
-    // |OnFailure| will be invoked on libjingle's signaling thread and posted to
-    // the main thread. Since the main thread may complete before the signaling
-    // thread has deferenced this object there is no guarantee that this object
-    // is destructed on the main thread.
-    DLOG_IF(ERROR, !webkit_request_.IsNull())
-        << "SetLocalDescriptionRequest not completed. Shutting down?";
-  }
-
- private:
-  const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
-  blink::WebRTCVoidRequest webkit_request_;
-  SessionDescriptionRequestTracker tracker_;
-};
-
 blink::WebRTCLegacyStatsMemberType
 WebRTCLegacyStatsMemberTypeFromStatsValueType(
     webrtc::StatsReport::Value::Type type) {
@@ -978,6 +899,73 @@ void LocalRTCStatsResponse::addStats(const blink::WebRTCLegacyStats& stats) {
   impl_.AddStats(stats);
 }
 
+// Class mapping responses from calls to libjingle SetLocalDescription and a
+// blink::WebRTCVoidRequest.
+class RTCPeerConnectionHandler::SetLocalDescriptionRequest
+    : public webrtc::SetSessionDescriptionObserver {
+ public:
+  explicit SetLocalDescriptionRequest(
+      const scoped_refptr<base::SingleThreadTaskRunner>& main_thread,
+      const blink::WebRTCVoidRequest& request,
+      const base::WeakPtr<RTCPeerConnectionHandler>& handler,
+      const base::WeakPtr<PeerConnectionTracker>& tracker,
+      PeerConnectionTracker::Action action)
+      : handler_(handler),
+        main_thread_(main_thread),
+        native_peer_connection_(handler_->native_peer_connection()),
+        webkit_request_(request),
+        tracker_(handler, tracker, action) {}
+
+  void OnSuccess() override {
+    DCHECK(!main_thread_->BelongsToCurrentThread());
+    // We must read the signaling state before jumping thread to ensure we
+    // surface the state change of this operation and not a future one.
+    auto signaling_state = native_peer_connection_->signaling_state();
+    main_thread_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&SetLocalDescriptionRequest::OnSuccessOnMainThread, this,
+                       signaling_state));
+  }
+  void OnFailure(webrtc::RTCError error) override {
+    if (!main_thread_->BelongsToCurrentThread()) {
+      main_thread_->PostTask(
+          FROM_HERE, base::BindOnce(&SetLocalDescriptionRequest::OnFailure,
+                                    this, std::move(error)));
+      return;
+    }
+    tracker_.TrackOnFailure(error);
+    webkit_request_.RequestFailed(error);
+    webkit_request_.Reset();
+  }
+
+ protected:
+  ~SetLocalDescriptionRequest() override {
+    // This object is reference counted and its callback methods |OnSuccess| and
+    // |OnFailure| will be invoked on libjingle's signaling thread and posted to
+    // the main thread. Since the main thread may complete before the signaling
+    // thread has deferenced this object there is no guarantee that this object
+    // is destructed on the main thread.
+    DLOG_IF(ERROR, !webkit_request_.IsNull())
+        << "SetLocalDescriptionRequest not completed. Shutting down?";
+  }
+
+ private:
+  void OnSuccessOnMainThread(
+      webrtc::PeerConnectionInterface::SignalingState signaling_state) {
+    DCHECK(main_thread_->BelongsToCurrentThread());
+    handler_->OnSignalingChange(signaling_state);
+    tracker_.TrackOnSuccess(nullptr);
+    webkit_request_.RequestSucceeded();
+    webkit_request_.Reset();
+  }
+
+  const base::WeakPtr<RTCPeerConnectionHandler> handler_;
+  const scoped_refptr<base::SingleThreadTaskRunner> main_thread_;
+  scoped_refptr<webrtc::PeerConnectionInterface> native_peer_connection_;
+  blink::WebRTCVoidRequest webkit_request_;
+  SessionDescriptionRequestTracker tracker_;
+};
+
 // Processes the resulting state changes of a SetRemoteDescription call.
 class RTCPeerConnectionHandler::WebRtcSetRemoteDescriptionObserverImpl
     : public WebRtcSetRemoteDescriptionObserver {
@@ -1010,6 +998,8 @@ class RTCPeerConnectionHandler::WebRtcSetRemoteDescriptionObserverImpl
 
     auto& states = states_or_error.value();
     if (handler_) {
+      handler_->OnSignalingChange(states.signaling_state);
+
       // Determine which receivers have been removed before processing the
       // removal as to not invalidate the iterator.
       std::vector<RTCRtpReceiver*> removed_receivers;
@@ -1181,19 +1171,8 @@ class RTCPeerConnectionHandler::Observer
   friend class base::RefCountedThreadSafe<RTCPeerConnectionHandler::Observer>;
   ~Observer() override = default;
 
-  void OnSignalingChange(
-      PeerConnectionInterface::SignalingState new_state) override {
-    if (!main_thread_->BelongsToCurrentThread()) {
-      main_thread_->PostTask(
-          FROM_HERE,
-          base::BindOnce(&RTCPeerConnectionHandler::Observer::OnSignalingChange,
-                         this, new_state));
-    } else if (handler_) {
-      handler_->OnSignalingChange(new_state);
-    }
-  }
-
   // TODO(hbos): Remove once no longer mandatory to implement.
+  void OnSignalingChange(PeerConnectionInterface::SignalingState) override {}
   void OnAddStream(rtc::scoped_refptr<MediaStreamInterface>) override {}
   void OnRemoveStream(rtc::scoped_refptr<MediaStreamInterface>) override {}
 
@@ -1967,12 +1946,10 @@ void RTCPeerConnectionHandler::OnSignalingChange(
   DCHECK(thread_checker_.CalledOnValidThread());
   TRACE_EVENT0("webrtc", "RTCPeerConnectionHandler::OnSignalingChange");
 
-  blink::WebRTCPeerConnectionHandlerClient::SignalingState state =
-      GetWebKitSignalingState(new_state);
   if (peer_connection_tracker_)
-    peer_connection_tracker_->TrackSignalingStateChange(this, state);
+    peer_connection_tracker_->TrackSignalingStateChange(this, new_state);
   if (!is_closed_)
-    client_->DidChangeSignalingState(state);
+    client_->DidChangeSignalingState(new_state);
 }
 
 // Called any time the IceConnectionState changes

content/renderer/media/webrtc/rtc_peer_connection_handler.h

@@ -45,6 +45,7 @@ namespace content {
 class PeerConnectionDependencyFactory;
 class PeerConnectionTracker;
 class RtcDataChannelHandler;
+class SetLocalDescriptionRequest;
 
 // Mockable wrapper for blink::WebRTCStatsResponse
 class CONTENT_EXPORT LocalRTCStatsResponse : public rtc::RefCountInterface {
@@ -187,6 +188,8 @@ class CONTENT_EXPORT RTCPeerConnectionHandler
   friend class Observer;
   class WebRtcSetRemoteDescriptionObserverImpl;
   friend class WebRtcSetRemoteDescriptionObserverImpl;
+  class SetLocalDescriptionRequest;
+  friend class SetLocalDescriptionRequest;
 
   void OnSignalingChange(
       webrtc::PeerConnectionInterface::SignalingState new_state);

content/renderer/media/webrtc/rtc_peer_connection_handler_unittest.cc

@@ -188,11 +188,11 @@ class MockPeerConnectionTracker : public PeerConnectionTracker {
   MOCK_METHOD1(TrackStop, void(RTCPeerConnectionHandler* pc_handler));
   MOCK_METHOD2(TrackSignalingStateChange,
                void(RTCPeerConnectionHandler* pc_handler,
-                    WebRTCPeerConnectionHandlerClient::SignalingState state));
+                    webrtc::PeerConnectionInterface::SignalingState state));
   MOCK_METHOD2(
       TrackIceConnectionStateChange,
       void(RTCPeerConnectionHandler* pc_handler,
-           WebRTCPeerConnectionHandlerClient::ICEConnectionState state));
+           MockWebRTCPeerConnectionHandlerClient::ICEConnectionState state));
   MOCK_METHOD2(
       TrackIceGatheringStateChange,
       void(RTCPeerConnectionHandler* pc_handler,
@@ -1049,67 +1049,6 @@ TEST_F(RTCPeerConnectionHandlerTest, GetRTCStats) {
   EXPECT_EQ(defined_stats_count, 1);
 }
 
-TEST_F(RTCPeerConnectionHandlerTest, OnSignalingChange) {
-  testing::InSequence sequence;
-
-  webrtc::PeerConnectionInterface::SignalingState new_state =
-      webrtc::PeerConnectionInterface::kHaveRemoteOffer;
-  EXPECT_CALL(
-      *mock_tracker_.get(),
-      TrackSignalingStateChange(
-          pc_handler_.get(),
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveRemoteOffer));
-  EXPECT_CALL(
-      *mock_client_.get(),
-      DidChangeSignalingState(
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveRemoteOffer));
-  pc_handler_->observer()->OnSignalingChange(new_state);
-
-  new_state = webrtc::PeerConnectionInterface::kHaveLocalPrAnswer;
-  EXPECT_CALL(
-      *mock_tracker_.get(),
-      TrackSignalingStateChange(
-          pc_handler_.get(),
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveLocalPrAnswer));
-  EXPECT_CALL(
-      *mock_client_.get(),
-      DidChangeSignalingState(
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveLocalPrAnswer));
-  pc_handler_->observer()->OnSignalingChange(new_state);
-
-  new_state = webrtc::PeerConnectionInterface::kHaveLocalOffer;
-  EXPECT_CALL(
-      *mock_tracker_.get(),
-      TrackSignalingStateChange(
-          pc_handler_.get(),
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveLocalOffer));
-  EXPECT_CALL(
-      *mock_client_.get(),
-      DidChangeSignalingState(
-          WebRTCPeerConnectionHandlerClient::kSignalingStateHaveLocalOffer));
-  pc_handler_->observer()->OnSignalingChange(new_state);
-
-  new_state = webrtc::PeerConnectionInterface::kHaveRemotePrAnswer;
-  EXPECT_CALL(*mock_tracker_.get(),
-              TrackSignalingStateChange(pc_handler_.get(),
-                                        WebRTCPeerConnectionHandlerClient::
-                                            kSignalingStateHaveRemotePrAnswer));
-  EXPECT_CALL(*mock_client_.get(),
-              DidChangeSignalingState(WebRTCPeerConnectionHandlerClient::
-                                          kSignalingStateHaveRemotePrAnswer));
-  pc_handler_->observer()->OnSignalingChange(new_state);
-
-  new_state = webrtc::PeerConnectionInterface::kClosed;
-  EXPECT_CALL(*mock_tracker_.get(),
-              TrackSignalingStateChange(
-                  pc_handler_.get(),
-                  WebRTCPeerConnectionHandlerClient::kSignalingStateClosed));
-  EXPECT_CALL(*mock_client_.get(),
-              DidChangeSignalingState(
-                  WebRTCPeerConnectionHandlerClient::kSignalingStateClosed));
-  pc_handler_->observer()->OnSignalingChange(new_state);
-}
-
 TEST_F(RTCPeerConnectionHandlerTest, OnIceConnectionChange) {
   testing::InSequence sequence;
 

content/renderer/media/webrtc/webrtc_set_remote_description_observer.cc

@@ -24,15 +24,19 @@ WebRtcReceiverState& WebRtcReceiverState::operator=(
 
 WebRtcReceiverState::~WebRtcReceiverState() {}
 
-WebRtcSetRemoteDescriptionObserver::States::States() {}
+WebRtcSetRemoteDescriptionObserver::States::States()
+    : signaling_state(
+          webrtc::PeerConnectionInterface::SignalingState::kClosed) {}
 
 WebRtcSetRemoteDescriptionObserver::States::States(States&& other)
-    : receiver_states(std::move(other.receiver_states)) {}
+    : signaling_state(other.signaling_state),
+      receiver_states(std::move(other.receiver_states)) {}
 
 WebRtcSetRemoteDescriptionObserver::States::~States() {}
 
 WebRtcSetRemoteDescriptionObserver::States&
 WebRtcSetRemoteDescriptionObserver::States::operator=(States&& other) {
+  signaling_state = other.signaling_state;
   receiver_states = std::move(other.receiver_states);
   return *this;
 }
@@ -86,6 +90,7 @@ void WebRtcSetRemoteDescriptionObserverHandler::OnSetRemoteDescriptionComplete(
       states_or_error;
   if (error.ok()) {
     WebRtcSetRemoteDescriptionObserver::States states;
+    states.signaling_state = pc_->signaling_state();
     for (const auto& webrtc_receiver : pc_->GetReceivers()) {
       std::unique_ptr<WebRtcMediaStreamTrackAdapterMap::AdapterRef> track_ref =
           track_adapter_map()->GetOrCreateRemoteTrackAdapter(

content/renderer/media/webrtc/webrtc_set_remote_description_observer.h

@@ -16,6 +16,7 @@
 #include "content/renderer/media/webrtc/rtc_peer_connection_handler.h"
 #include "content/renderer/media/webrtc/webrtc_media_stream_adapter_map.h"
 #include "content/renderer/media/webrtc/webrtc_media_stream_track_adapter.h"
+#include "third_party/webrtc/api/peerconnectioninterface.h"
 #include "third_party/webrtc/api/rtcerror.h"
 #include "third_party/webrtc/api/rtpreceiverinterface.h"
 #include "third_party/webrtc/api/setremotedescriptionobserverinterface.h"
@@ -70,6 +71,7 @@ class CONTENT_EXPORT WebRtcSetRemoteDescriptionObserver
 
     States& operator=(States&& other);
 
+    webrtc::PeerConnectionInterface::SignalingState signaling_state;
     // The receivers at the time of the event.
     std::vector<WebRtcReceiverState> receiver_states;
     // Check that the invariants for this structure hold.

third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-expected.txt

 This is a testharness.js-based test.
 FAIL Calling createOffer() and setLocalDescription() again after one round of local-offer/remote-answer should succeed assert_not_equals: Expect session description to be defined got disallowed value undefined
 FAIL Switching role from answerer to offerer after going back to stable state should succeed assert_not_equals: Expect session description to be defined got disallowed value undefined
+PASS onsignalingstatechange fires before setLocalDescription resolves
 Harness: the test ran to completion.
 

third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription.html

@@ -96,6 +96,21 @@
 
   }, 'Switching role from answerer to offerer after going back to stable state should succeed');
 
+  promise_test(async t => {
+    const pc = new RTCPeerConnection();
+    const offer = await pc.createOffer();
+    let eventSequence = '';
+    const signalingstatechangeResolver = new Resolver();
+    pc.onsignalingstatechange = () => {
+      eventSequence += 'onsignalingstatechange;';
+      signalingstatechangeResolver.resolve();
+    };
+    await pc.setLocalDescription(offer);
+    eventSequence += 'setLocalDescription;';
+    await signalingstatechangeResolver.promise;
+    assert_equals(eventSequence, 'onsignalingstatechange;setLocalDescription;');
+  }, 'onsignalingstatechange fires before setLocalDescription resolves');
+
   /*
     TODO
       4.3.2.  setLocalDescription

third_party/blink/public/platform/web_rtc_peer_connection_handler_client.h

@@ -35,6 +35,7 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "third_party/blink/public/platform/web_common.h"
+#include "third_party/webrtc/api/peerconnectioninterface.h"
 
 namespace blink {
 
@@ -44,15 +45,6 @@ class WebRTCRtpReceiver;
 
 class BLINK_PLATFORM_EXPORT WebRTCPeerConnectionHandlerClient {
  public:
-  enum SignalingState {
-    kSignalingStateStable = 1,
-    kSignalingStateHaveLocalOffer = 2,
-    kSignalingStateHaveRemoteOffer = 3,
-    kSignalingStateHaveLocalPrAnswer = 4,
-    kSignalingStateHaveRemotePrAnswer = 5,
-    kSignalingStateClosed = 6,
-  };
-
   enum ICEConnectionState {
     kICEConnectionStateNew = 1,
     kICEConnectionStateChecking = 2,
@@ -80,7 +72,8 @@ class BLINK_PLATFORM_EXPORT WebRTCPeerConnectionHandlerClient {
 
   virtual void NegotiationNeeded() = 0;
   virtual void DidGenerateICECandidate(scoped_refptr<WebRTCICECandidate>) = 0;
-  virtual void DidChangeSignalingState(SignalingState) = 0;
+  virtual void DidChangeSignalingState(
+      webrtc::PeerConnectionInterface::SignalingState) = 0;
   virtual void DidChangeICEGatheringState(ICEGatheringState) = 0;
   virtual void DidChangeICEConnectionState(ICEConnectionState) = 0;
   virtual void DidAddRemoteTrack(std::unique_ptr<WebRTCRtpReceiver>) = 0;

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.cc

@@ -124,9 +124,9 @@ const char kModifiedSdpMessage[] =
 const long kMaxPeerConnections = 500;
 
 bool ThrowExceptionIfSignalingStateClosed(
-    RTCPeerConnection::SignalingState state,
+    webrtc::PeerConnectionInterface::SignalingState state,
     ExceptionState& exception_state) {
-  if (state == RTCPeerConnection::kSignalingStateClosed) {
+  if (state == webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     exception_state.ThrowDOMException(DOMExceptionCode::kInvalidStateError,
                                       kSignalingStateClosedMessage);
     return true;
@@ -146,9 +146,9 @@ void AsyncCallErrorCallback(V8RTCPeerConnectionErrorCallback* error_callback,
 }
 
 bool CallErrorCallbackIfSignalingStateClosed(
-    RTCPeerConnection::SignalingState state,
+    webrtc::PeerConnectionInterface::SignalingState state,
     V8RTCPeerConnectionErrorCallback* error_callback) {
-  if (state == RTCPeerConnection::kSignalingStateClosed) {
+  if (state == webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     if (error_callback) {
       AsyncCallErrorCallback(
           error_callback,
@@ -529,7 +529,8 @@ RTCPeerConnection::RTCPeerConnection(ExecutionContext* context,
                                      WebMediaConstraints constraints,
                                      ExceptionState& exception_state)
     : PausableObject(context),
-      signaling_state_(kSignalingStateStable),
+      signaling_state_(
+          webrtc::PeerConnectionInterface::SignalingState::kStable),
       ice_gathering_state_(kICEGatheringStateNew),
       ice_connection_state_(kICEConnectionStateNew),
       // WebRTC spec specifies kNetworking as task source.
@@ -613,7 +614,8 @@ void RTCPeerConnection::Dispose() {
 
 ScriptPromise RTCPeerConnection::createOffer(ScriptState* script_state,
                                              const RTCOfferOptions& options) {
-  if (signaling_state_ == kSignalingStateClosed) {
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     return ScriptPromise::RejectWithDOMException(
         script_state, DOMException::Create(DOMExceptionCode::kInvalidStateError,
                                            kSignalingStateClosedMessage));
@@ -697,7 +699,8 @@ ScriptPromise RTCPeerConnection::createOffer(
 
 ScriptPromise RTCPeerConnection::createAnswer(ScriptState* script_state,
                                               const RTCAnswerOptions& options) {
-  if (signaling_state_ == kSignalingStateClosed) {
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     return ScriptPromise::RejectWithDOMException(
         script_state, DOMException::Create(DOMExceptionCode::kInvalidStateError,
                                            kSignalingStateClosedMessage));
@@ -757,7 +760,8 @@ DOMException* RTCPeerConnection::checkSdpForStateErrors(
     ExecutionContext* context,
     const RTCSessionDescriptionInit& session_description_init,
     String* sdp) {
-  if (signaling_state_ == kSignalingStateClosed) {
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     return DOMException::Create(DOMExceptionCode::kInvalidStateError,
                                 kSignalingStateClosedMessage);
   }
@@ -863,7 +867,8 @@ RTCSessionDescription* RTCPeerConnection::localDescription() {
 ScriptPromise RTCPeerConnection::setRemoteDescription(
     ScriptState* script_state,
     const RTCSessionDescriptionInit& session_description_init) {
-  if (signaling_state_ == kSignalingStateClosed) {
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     return ScriptPromise::RejectWithDOMException(
         script_state, DOMException::Create(DOMExceptionCode::kInvalidStateError,
                                            kSignalingStateClosedMessage));
@@ -1088,7 +1093,8 @@ ScriptPromise RTCPeerConnection::addIceCandidate(
     ScriptState* script_state,
     const RTCIceCandidateInitOrRTCIceCandidate& candidate,
     ExceptionState& exception_state) {
-  if (signaling_state_ == kSignalingStateClosed) {
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     return ScriptPromise::RejectWithDOMException(
         script_state, DOMException::Create(DOMExceptionCode::kInvalidStateError,
                                            kSignalingStateClosedMessage));
@@ -1151,17 +1157,17 @@ ScriptPromise RTCPeerConnection::addIceCandidate(
 
 String RTCPeerConnection::signalingState() const {
   switch (signaling_state_) {
-    case kSignalingStateStable:
+    case webrtc::PeerConnectionInterface::SignalingState::kStable:
       return "stable";
-    case kSignalingStateHaveLocalOffer:
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveLocalOffer:
       return "have-local-offer";
-    case kSignalingStateHaveRemoteOffer:
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveRemoteOffer:
       return "have-remote-offer";
-    case kSignalingStateHaveLocalPrAnswer:
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveLocalPrAnswer:
       return "have-local-pranswer";
-    case kSignalingStateHaveRemotePrAnswer:
+    case webrtc::PeerConnectionInterface::SignalingState::kHaveRemotePrAnswer:
       return "have-remote-pranswer";
-    case kSignalingStateClosed:
+    case webrtc::PeerConnectionInterface::SignalingState::kClosed:
       return "closed";
   }
 
@@ -1618,7 +1624,8 @@ RTCDTMFSender* RTCPeerConnection::createDTMFSender(
 }
 
 void RTCPeerConnection::close() {
-  if (signaling_state_ == RTCPeerConnection::kSignalingStateClosed)
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed)
     return;
 
   CloseInternal();
@@ -1685,10 +1692,11 @@ void RTCPeerConnection::DidGenerateICECandidate(
   ScheduleDispatchEvent(RTCPeerConnectionIceEvent::Create(ice_candidate));
 }
 
-void RTCPeerConnection::DidChangeSignalingState(SignalingState new_state) {
+void RTCPeerConnection::DidChangeSignalingState(
+    webrtc::PeerConnectionInterface::SignalingState new_state) {
   DCHECK(!closed_);
   DCHECK(GetExecutionContext()->IsContextThread());
-  ChangeSignalingState(new_state);
+  ChangeSignalingState(new_state, true);
 }
 
 void RTCPeerConnection::DidChangeICEGatheringState(
@@ -1709,7 +1717,8 @@ void RTCPeerConnection::DidAddRemoteTrack(
     std::unique_ptr<WebRTCRtpReceiver> web_rtp_receiver) {
   DCHECK(!closed_);
   DCHECK(GetExecutionContext()->IsContextThread());
-  if (signaling_state_ == kSignalingStateClosed)
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed)
     return;
   HeapVector<Member<MediaStream>> streams;
   WebVector<WebMediaStream> web_streams = web_rtp_receiver->Streams();
@@ -1836,7 +1845,8 @@ void RTCPeerConnection::DidAddRemoteDataChannel(
   DCHECK(!closed_);
   DCHECK(GetExecutionContext()->IsContextThread());
 
-  if (signaling_state_ == kSignalingStateClosed)
+  if (signaling_state_ ==
+      webrtc::PeerConnectionInterface::SignalingState::kClosed)
     return;
 
   RTCDataChannel* channel =
@@ -1852,7 +1862,7 @@ void RTCPeerConnection::ReleasePeerConnectionHandler() {
 
   stopped_ = true;
   ice_connection_state_ = kICEConnectionStateClosed;
-  signaling_state_ = kSignalingStateClosed;
+  signaling_state_ = webrtc::PeerConnectionInterface::SignalingState::kClosed;
 
   dispatch_scheduled_event_runner_->Stop();
 
@@ -1862,7 +1872,8 @@ void RTCPeerConnection::ReleasePeerConnectionHandler() {
 }
 
 void RTCPeerConnection::ClosePeerConnection() {
-  DCHECK(signaling_state_ != RTCPeerConnection::kSignalingStateClosed);
+  DCHECK(signaling_state_ !=
+         webrtc::PeerConnectionInterface::SignalingState::kClosed);
   CloseInternal();
 }
 
@@ -1893,10 +1904,19 @@ void RTCPeerConnection::ContextDestroyed(ExecutionContext*) {
   ReleasePeerConnectionHandler();
 }
 
-void RTCPeerConnection::ChangeSignalingState(SignalingState signaling_state) {
-  if (signaling_state_ != kSignalingStateClosed) {
+void RTCPeerConnection::ChangeSignalingState(
+    webrtc::PeerConnectionInterface::SignalingState signaling_state,
+    bool dispatch_event_immediately) {
+  if (signaling_state_ == signaling_state)
+    return;
+  if (signaling_state_ !=
+      webrtc::PeerConnectionInterface::SignalingState::kClosed) {
     signaling_state_ = signaling_state;
-    ScheduleDispatchEvent(Event::Create(EventTypeNames::signalingstatechange));
+    Event* event = Event::Create(EventTypeNames::signalingstatechange);
+    if (dispatch_event_immediately)
+      DispatchEvent(event);
+    else
+      ScheduleDispatchEvent(event);
   }
 }
 
@@ -1949,12 +1969,14 @@ bool RTCPeerConnection::SetIceConnectionState(
 }
 
 void RTCPeerConnection::CloseInternal() {
-  DCHECK(signaling_state_ != RTCPeerConnection::kSignalingStateClosed);
+  DCHECK(signaling_state_ !=
+         webrtc::PeerConnectionInterface::SignalingState::kClosed);
   peer_handler_->Stop();
   closed_ = true;
 
   ChangeIceConnectionState(kICEConnectionStateClosed);
-  ChangeSignalingState(kSignalingStateClosed);
+  ChangeSignalingState(webrtc::PeerConnectionInterface::SignalingState::kClosed,
+                       false);
   Document* document = ToDocument(GetExecutionContext());
   HostsUsingFeatures::CountAnyWorld(
       *document, HostsUsingFeatures::Feature::kRTCPeerConnectionUsed);

third_party/blink/renderer/modules/peerconnection/rtc_peer_connection.h

@@ -215,7 +215,8 @@ class MODULES_EXPORT RTCPeerConnection final
   // WebRTCPeerConnectionHandlerClient
   void NegotiationNeeded() override;
   void DidGenerateICECandidate(scoped_refptr<WebRTCICECandidate>) override;
-  void DidChangeSignalingState(SignalingState) override;
+  void DidChangeSignalingState(
+      webrtc::PeerConnectionInterface::SignalingState) override;
   void DidChangeICEGatheringState(ICEGatheringState) override;
   void DidChangeICEConnectionState(ICEConnectionState) override;
   void DidAddRemoteTrack(std::unique_ptr<WebRTCRtpReceiver>) override;
@@ -287,7 +288,16 @@ class MODULES_EXPORT RTCPeerConnection final
   HeapVector<Member<RTCRtpReceiver>>::iterator FindReceiver(
       const WebRTCRtpReceiver& web_receiver);
 
-  // The "Change" methods set the state asynchronously and fire the
+  // Sets the signaling state synchronously, and dispatches a
+  // signalingstatechange event synchronously or asynchronously depending on
+  // |dispatch_event_immediately|.
+  // TODO(hbos): The ability to not fire the event asynchronously is there
+  // because CloseInternal() has historically fired asynchronously along with
+  // other asynchronously fired events. If close() does not fire any events,
+  // |dispatch_event_immediately| can be removed. https://crbug.com/849247
+  void ChangeSignalingState(webrtc::PeerConnectionInterface::SignalingState,
+                            bool dispatch_event_immediately);
+  // The remaining "Change" methods set the state asynchronously and fire the
   // corresponding event immediately after changing the state (if it was really
   // changed).
   //
@@ -300,15 +310,6 @@ class MODULES_EXPORT RTCPeerConnection final
   // possible to, for example, end up with two "icegatheringstatechange" events
   // that are delayed somehow and cause the application to read a "complete"
   // gathering state twice, missing the "gathering" state in the middle.
-  //
-  // TODO(deadbeef): This wasn't done for the signaling state because it
-  // resulted in a change to the order of the signaling state being updated
-  // relative to the SetLocalDescription or SetRemoteDescription promise being
-  // resolved. Some additional refactoring would be necessary to fix this; for
-  // example, passing the new signaling state along with the SRD/SLD callbacks
-  // as opposed to relying on a separate event.
-  void ChangeSignalingState(WebRTCPeerConnectionHandlerClient::SignalingState);
-
   void ChangeIceGatheringState(
       WebRTCPeerConnectionHandlerClient::ICEGatheringState);
   bool SetIceGatheringState(
@@ -327,7 +328,7 @@ class MODULES_EXPORT RTCPeerConnection final
                                        const RTCSessionDescriptionInit&,
                                        String* sdp);
 
-  SignalingState signaling_state_;
+  webrtc::PeerConnectionInterface::SignalingState signaling_state_;
   ICEGatheringState ice_gathering_state_;
   ICEConnectionState ice_connection_state_;