Send IceCandidates after offer/answer exchange

Only start sending IceCandidates after we exchanged the offer and answer
messages. This allows us to reject incoming IceCandidate messages for
already closed connections if they arrive after the connection has been
closed.

Bug: 1050041
Change-Id: I6f332ee44b43551d2fabc8633b3571fe3bae76db
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013127
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Himanshu Jaju <himanshujaju@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739464}

chrome/browser/sharing/webrtc/sharing_service_host.cc

@@ -180,8 +180,7 @@ void SharingServiceHost::DoSendMessageToDevice(
 
   // TODO(crbug.com/1044539): support multiple messages over the same connection
   // or queue messages instead of rejecting them here.
-  auto connection_iter = connections_.find(device.guid());
-  if (connection_iter != connections_.end()) {
+  if (connections_.find(device.guid()) != connections_.end()) {
     std::move(callback).Run(SharingSendMessageResult::kInternalError,
                             /*message_id=*/base::nullopt,
                             SharingChannelType::kWebRtc);
@@ -198,7 +197,7 @@ void SharingServiceHost::DoSendMessageToDevice(
     return;
   }
 
-  GetConnection(device.guid(), *fcm_configuration)
+  CreateConnection(device.guid(), *fcm_configuration)
       ->SendMessage(std::move(webrtc_message),
                     base::BindOnce(&OnMessageSent, std::move(callback),
                                    std::move(message_guid)));
@@ -217,7 +216,14 @@ void SharingServiceHost::OnOfferReceived(
     const chrome_browser_sharing::FCMChannelConfiguration& fcm_configuration,
     const std::string& offer,
     base::OnceCallback<void(const std::string&)> callback) {
-  GetConnection(device_guid, fcm_configuration)
+  // TODO(crbug.com/1044539): support multiple messages over the same connection
+  // or queue messages instead of rejecting them here.
+  if (connections_.find(device_guid) != connections_.end()) {
+    std::move(callback).Run(/*answer=*/std::string());
+    return;
+  }
+
+  CreateConnection(device_guid, fcm_configuration)
       ->OnOfferReceived(offer, std::move(callback));
 }
 
@@ -225,8 +231,13 @@ void SharingServiceHost::OnIceCandidatesReceived(
     const std::string& device_guid,
     const chrome_browser_sharing::FCMChannelConfiguration& fcm_configuration,
     std::vector<sharing::mojom::IceCandidatePtr> ice_candidates) {
-  GetConnection(device_guid, fcm_configuration)
-      ->OnIceCandidatesReceived(std::move(ice_candidates));
+  // IceCandidates should only be received to already open connections. Ignore
+  // all other ones as we don't want to start a new process here.
+  auto connection_iter = connections_.find(device_guid);
+  if (connection_iter == connections_.end())
+    return;
+
+  connection_iter->second->OnIceCandidatesReceived(std::move(ice_candidates));
 }
 
 void SharingServiceHost::SetSharingHandlerRegistry(
@@ -234,13 +245,20 @@ void SharingServiceHost::SetSharingHandlerRegistry(
   handler_registry_ = handler_registry;
 }
 
-SharingWebRtcConnectionHost* SharingServiceHost::GetConnection(
+SharingServiceHost::Connections&
+SharingServiceHost::GetConnectionsForTesting() {
+  return connections_;
+}
+
+void SharingServiceHost::BindSharingServiceForTesting(
+    mojo::PendingRemote<sharing::mojom::Sharing> service) {
+  sharing_utility_service_.Bind(std::move(service));
+  sharing_utility_service_.reset_on_disconnect();
+}
+
+SharingWebRtcConnectionHost* SharingServiceHost::CreateConnection(
     const std::string& device_guid,
     const chrome_browser_sharing::FCMChannelConfiguration& fcm_configuration) {
-  auto connection_iter = connections_.find(device_guid);
-  if (connection_iter != connections_.end())
-    return connection_iter->second.get();
-
   auto pipes = std::make_unique<SharingWebRtcMojoPipes>();
 
   auto signalling_host = std::make_unique<WebRtcSignallingHostFCM>(

chrome/browser/sharing/webrtc/sharing_service_host.h

@@ -19,6 +19,7 @@
 #include "chrome/services/sharing/public/mojom/sharing.mojom.h"
 #include "chrome/services/sharing/public/mojom/webrtc.mojom.h"
 #include "components/sync_device_info/device_info.h"
+#include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
@@ -45,6 +46,9 @@ class SharingServiceHost : public SharingMessageSender::SendMessageDelegate {
       base::OnceCallback<void(SharingSendMessageResult result,
                               base::Optional<std::string> message_id,
                               SharingChannelType channel_type)>;
+  using Connections =
+      std::unordered_map<std::string,
+                         std::unique_ptr<SharingWebRtcConnectionHost>>;
 
   SharingServiceHost(
       SharingMessageSender* message_sender,
@@ -75,10 +79,14 @@ class SharingServiceHost : public SharingMessageSender::SendMessageDelegate {
 
   void SetSharingHandlerRegistry(SharingHandlerRegistry* handler_registry);
 
+  Connections& GetConnectionsForTesting();
+  void BindSharingServiceForTesting(
+      mojo::PendingRemote<sharing::mojom::Sharing> service);
+
  private:
   void OnPeerConnectionClosed(const std::string& device_guid);
 
-  SharingWebRtcConnectionHost* GetConnection(
+  SharingWebRtcConnectionHost* CreateConnection(
       const std::string& device_guid,
       const chrome_browser_sharing::FCMChannelConfiguration& fcm_configuration);
 
@@ -100,8 +108,7 @@ class SharingServiceHost : public SharingMessageSender::SendMessageDelegate {
 
   // Map of device_guid to SharingWebRtcConnectionHost containing all currently
   // active connections.
-  std::unordered_map<std::string, std::unique_ptr<SharingWebRtcConnectionHost>>
-      connections_;
+  Connections connections_;
 
   // Will be set when a message handler for this is registered. Owned by the
   // SharingService KeyedService.

chrome/browser/sharing/webrtc/sharing_service_host_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sharing/webrtc/sharing_service_host.h"
+
+#include "base/strings/strcat.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/sharing/mock_sharing_device_source.h"
+#include "chrome/browser/sharing/sharing_sync_preference.h"
+#include "chrome/browser/sharing/webrtc/sharing_webrtc_connection_host.h"
+#include "chrome/services/sharing/public/mojom/webrtc.mojom.h"
+#include "components/sync_device_info/fake_device_info_sync_service.h"
+#include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "content/public/test/browser_task_environment.h"
+#include "google_apis/google_api_keys.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class FakeSharingMojoService : public sharing::mojom::Sharing,
+                               public sharing::mojom::SignallingReceiver {
+ public:
+  FakeSharingMojoService() = default;
+  ~FakeSharingMojoService() override = default;
+
+  // sharing::mojom::Sharing:
+  void CreateSharingWebRtcConnection(
+      mojo::PendingRemote<sharing::mojom::SignallingSender> signalling_sender,
+      mojo::PendingReceiver<sharing::mojom::SignallingReceiver>
+          signalling_receiver,
+      mojo::PendingRemote<sharing::mojom::SharingWebRtcConnectionDelegate>
+          delegate,
+      mojo::PendingReceiver<sharing::mojom::SharingWebRtcConnection> connection,
+      mojo::PendingRemote<network::mojom::P2PSocketManager> socket_manager,
+      mojo::PendingRemote<network::mojom::MdnsResponder> mdns_responder,
+      std::vector<sharing::mojom::IceServerPtr> ice_servers) override {
+    signaling_set.Add(this, std::move(signalling_receiver));
+  }
+
+  void OnOfferReceived(const std::string& offer,
+                       OnOfferReceivedCallback callback) override {
+    std::move(callback).Run(answer);
+  }
+
+  void OnIceCandidatesReceived(
+      std::vector<sharing::mojom::IceCandidatePtr> ice_candidates) override {}
+
+  mojo::Receiver<sharing::mojom::Sharing> receiver{this};
+  mojo::ReceiverSet<sharing::mojom::SignallingReceiver> signaling_set;
+  std::string answer;
+};
+
+}  // namespace
+
+class SharingServiceHostTest : public testing::Test {
+ public:
+  SharingServiceHostTest() {
+    SharingSyncPreference::RegisterProfilePrefs(prefs_.registry());
+
+    host_ = std::make_unique<SharingServiceHost>(
+        /*message_sender=*/nullptr,
+        /*gcm_driver=*/nullptr, &sync_prefs_, &device_source_,
+        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+            &test_url_loader_factory_));
+
+    host_->BindSharingServiceForTesting(
+        fake_mojo_service_.receiver.BindNewPipeAndPassRemote());
+  }
+
+  std::string GetApiUrl() const {
+    return base::StrCat(
+        {"https://networktraversal.googleapis.com/v1alpha/iceconfig?key=",
+         google_apis::GetSharingAPIKey()});
+  }
+
+  std::string OnOfferReceivedSync(const std::string& guid) {
+    std::string result;
+    base::RunLoop run_loop;
+    host_->OnOfferReceived(
+        guid, /*fcm_configuration=*/{}, "offer",
+        base::BindLambdaForTesting([&](const std::string& answer) {
+          result = answer;
+          run_loop.Quit();
+        }));
+    run_loop.Run();
+    return result;
+  }
+
+ protected:
+  content::BrowserTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME,
+      content::BrowserTaskEnvironment::MainThreadType::IO};
+  MockSharingDeviceSource device_source_;
+  sync_preferences::TestingPrefServiceSyncable prefs_;
+  syncer::FakeDeviceInfoSyncService fake_device_info_sync_service_;
+  SharingSyncPreference sync_prefs_{&prefs_, &fake_device_info_sync_service_};
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  FakeSharingMojoService fake_mojo_service_;
+
+  std::unique_ptr<SharingServiceHost> host_;
+};
+
+TEST_F(SharingServiceHostTest, OnIceCandidatesReceived_BeforeConnection) {
+  host_->OnIceCandidatesReceived("unknown_guid", {}, {});
+  // Receiving an IceCandidate should not open new connections.
+  EXPECT_TRUE(host_->GetConnectionsForTesting().empty());
+}
+
+TEST_F(SharingServiceHostTest, OnOfferReceived_BeforeConnection) {
+  std::string guid = "unknown_guid";
+  std::string answer = "answer";
+
+  fake_mojo_service_.answer = answer;
+
+  EXPECT_CALL(device_source_, GetDeviceByGuid(guid))
+      .WillOnce(testing::Return(testing::ByMove(nullptr)));
+  test_url_loader_factory_.AddResponse(GetApiUrl(), /*response=*/std::string(),
+                                       net::HTTP_OK);
+
+  EXPECT_EQ(answer, OnOfferReceivedSync(guid));
+  // Receiving an offer should initialize a WebRTC connection.
+  EXPECT_EQ(1u, host_->GetConnectionsForTesting().size());
+}
+
+TEST_F(SharingServiceHostTest, OnOfferReceived_ExistingConnection) {
+  std::string guid = "existing_guid";
+  fake_mojo_service_.answer = "answer";
+
+  // Simulate existing connection for |guid|
+  host_->GetConnectionsForTesting()[guid] = nullptr;
+
+  EXPECT_EQ("", OnOfferReceivedSync(guid));
+
+  // We should still have only one connection
+  auto& connections = host_->GetConnectionsForTesting();
+  EXPECT_EQ(1u, connections.size());
+  EXPECT_TRUE(connections.find(guid) != connections.end());
+}

chrome/services/sharing/webrtc/sharing_webrtc_connection.cc

@@ -287,18 +287,10 @@ void SharingWebRtcConnection::OnIceCandidatesReceived(
 
   timing_recorder_.LogEvent(WebRtcTimingEvent::kIceCandidateReceived);
 
-  // Store received ICE candidates until the signalling state is stable and
-  // there is no offer / answer exchange in progress anymore.
-  if (!peer_connection_->local_description() ||
-      peer_connection_->signaling_state() !=
-          webrtc::PeerConnectionInterface::SignalingState::kStable) {
-    ice_candidates_.insert(ice_candidates_.end(),
-                           std::make_move_iterator(ice_candidates.begin()),
-                           std::make_move_iterator(ice_candidates.end()));
-    return;
-  }
-
-  AddIceCandidates(std::move(ice_candidates));
+  remote_ice_candidates_.insert(remote_ice_candidates_.end(),
+                                std::make_move_iterator(ice_candidates.begin()),
+                                std::make_move_iterator(ice_candidates.end()));
+  HandlePendingIceCandidates();
 }
 
 void SharingWebRtcConnection::SendMessage(const std::vector<uint8_t>& message,
@@ -368,8 +360,11 @@ void SharingWebRtcConnection::SendMessage(const std::vector<uint8_t>& message,
 
 void SharingWebRtcConnection::OnSignalingChange(
     webrtc::PeerConnectionInterface::SignalingState new_state) {
-  if (new_state == webrtc::PeerConnectionInterface::SignalingState::kStable)
-    timing_recorder_.LogEvent(WebRtcTimingEvent::kSignalingStable);
+  if (new_state != webrtc::PeerConnectionInterface::SignalingState::kStable)
+    return;
+
+  timing_recorder_.LogEvent(WebRtcTimingEvent::kSignalingStable);
+  HandlePendingIceCandidates();
 }
 
 void SharingWebRtcConnection::OnDataChannel(
@@ -406,11 +401,10 @@ void SharingWebRtcConnection::OnIceCandidate(
     return;
   }
 
-  std::vector<mojom::IceCandidatePtr> ice_candidates;
-  ice_candidates.push_back(mojom::IceCandidate::New(
+  local_ice_candidates_.push_back(mojom::IceCandidate::New(
       candidate_string, candidate->sdp_mid(), candidate->sdp_mline_index()));
 
-  signalling_sender_->SendIceCandidates(std::move(ice_candidates));
+  HandlePendingIceCandidates();
 }
 
 void SharingWebRtcConnection::OnStateChange() {
@@ -511,8 +505,7 @@ void SharingWebRtcConnection::OnAnswerCreated(OnOfferReceivedCallback callback,
 
   timing_recorder_.LogEvent(WebRtcTimingEvent::kAnswerCreated);
 
-  AddIceCandidates(std::move(ice_candidates_));
-  ice_candidates_.clear();
+  HandlePendingIceCandidates();
   std::move(callback).Run(sdp);
 }
 
@@ -592,8 +585,7 @@ void SharingWebRtcConnection::OnRemoteDescriptionSet(webrtc::RTCError error) {
     return;
   }
 
-  AddIceCandidates(std::move(ice_candidates_));
-  ice_candidates_.clear();
+  HandlePendingIceCandidates();
 }
 
 void SharingWebRtcConnection::AddIceCandidates(
@@ -700,6 +692,28 @@ void SharingWebRtcConnection::MaybeSendQueuedMessages() {
   queued_messages_total_size_ = 0;
 }
 
+void SharingWebRtcConnection::HandlePendingIceCandidates() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Store received ICE candidates until the signalling state is stable and
+  // there is no offer / answer exchange in progress anymore.
+  if (!peer_connection_->local_description() ||
+      peer_connection_->signaling_state() !=
+          webrtc::PeerConnectionInterface::SignalingState::kStable) {
+    return;
+  }
+
+  if (!remote_ice_candidates_.empty()) {
+    AddIceCandidates(std::move(remote_ice_candidates_));
+    remote_ice_candidates_.clear();
+  }
+
+  if (!local_ice_candidates_.empty()) {
+    signalling_sender_->SendIceCandidates(std::move(local_ice_candidates_));
+    local_ice_candidates_.clear();
+  }
+}
+
 void SharingWebRtcConnection::OnStatsReceived(
     const rtc::scoped_refptr<const webrtc::RTCStatsReport>& report) {
   auto transport_stats_list =

chrome/services/sharing/webrtc/sharing_webrtc_connection.h

@@ -126,6 +126,8 @@ class SharingWebRtcConnection : public mojom::SignallingReceiver,
 
   void MaybeSendQueuedMessages();
 
+  void HandlePendingIceCandidates();
+
   void OnStatsReceived(
       const rtc::scoped_refptr<const webrtc::RTCStatsReport>& report);
 
@@ -144,7 +146,8 @@ class SharingWebRtcConnection : public mojom::SignallingReceiver,
   // being initiated.
   base::queue<PendingMessage> queued_messages_;
   size_t queued_messages_total_size_ = 0;
-  std::vector<mojom::IceCandidatePtr> ice_candidates_;
+  std::vector<mojom::IceCandidatePtr> remote_ice_candidates_;
+  std::vector<mojom::IceCandidatePtr> local_ice_candidates_;
 
   base::OnceCallback<void(SharingWebRtcConnection*)> on_disconnect_;
   SharingWebRtcTimingRecorder timing_recorder_;

chrome/services/sharing/webrtc/sharing_webrtc_connection_unittest.cc

@@ -105,11 +105,18 @@ class MockSessionDescriptionInterface
   MOCK_CONST_METHOD1(ToString, bool(std::string* out));
 };
 
+std::unique_ptr<webrtc::IceCandidateInterface> CreateIceCandidate(int index) {
+  const int base_port = 1234;
+  rtc::SocketAddress address("127.0.0.1", base_port + index);
+  cricket::Candidate candidate(cricket::ICE_CANDIDATE_COMPONENT_RTP, "udp",
+                               address, 1, "", "", "local", 0, "1");
+  return webrtc::CreateIceCandidate("mid", index, candidate);
+}
+
 std::vector<sharing::mojom::IceCandidatePtr> GenerateIceCandidates(
     int invalid_candidates,
     int valid_candidates) {
   std::vector<sharing::mojom::IceCandidatePtr> ice_candidates;
-  int port = 1234;
 
   // Add invalid ice candidate first;
   for (int i = 0; i < invalid_candidates; i++) {
@@ -123,12 +130,7 @@ std::vector<sharing::mojom::IceCandidatePtr> GenerateIceCandidates(
 
   // Add valid ice candidates;
   for (int i = 0; i < valid_candidates; i++) {
-    rtc::SocketAddress address("127.0.0.1", port++);
-    cricket::Candidate candidate(cricket::ICE_CANDIDATE_COMPONENT_RTP, "udp",
-                                 address, 1, "", "", "local", 0, "1");
-    std::unique_ptr<webrtc::IceCandidateInterface> ice_candidate =
-        webrtc::CreateIceCandidate("mid", i, candidate);
-
+    auto ice_candidate = CreateIceCandidate(i);
     sharing::mojom::IceCandidatePtr ice_candidate_ptr(
         sharing::mojom::IceCandidate::New());
     ice_candidate->ToString(&ice_candidate_ptr->candidate);
@@ -297,4 +299,73 @@ TEST_F(SharingWebRtcConnectionTest, AddIceCandidates_RTCError) {
       valid_ice_candidates_count - valid_ice_candidates_internal_error);
 }
 
+TEST_F(SharingWebRtcConnectionTest, AddIceCandidates_BeforeSignaling) {
+  // Local description not ready
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(nullptr));
+
+  EXPECT_CALL(*mock_webrtc_pc_, AddIceCandidate).Times(0);
+  connection_->OnIceCandidatesReceived(GenerateIceCandidates(
+      /*invalid_count=*/0, /*valid_count=*/1));
+
+  // Signaling state not ready
+  MockSessionDescriptionInterface local_description;
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(&local_description));
+  EXPECT_CALL(*mock_webrtc_pc_, signaling_state())
+      .WillOnce(testing::Return(
+          webrtc::PeerConnectionInterface::SignalingState::kHaveRemoteOffer));
+
+  EXPECT_CALL(*mock_webrtc_pc_, AddIceCandidate).Times(0);
+  connection_->OnIceCandidatesReceived(GenerateIceCandidates(
+      /*invalid_count=*/0, /*valid_count=*/1));
+
+  // Local description and signaling state ready
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(&local_description));
+  EXPECT_CALL(*mock_webrtc_pc_, signaling_state())
+      .WillOnce(testing::Return(
+          webrtc::PeerConnectionInterface::SignalingState::kStable));
+
+  EXPECT_CALL(*mock_webrtc_pc_, AddIceCandidate).Times(3);
+  connection_->OnIceCandidatesReceived(GenerateIceCandidates(
+      /*invalid_count=*/0, /*valid_count=*/1));
+}
+
+TEST_F(SharingWebRtcConnectionTest, OnIceCandidate_BeforeSignaling) {
+  auto ice_candidate = CreateIceCandidate(/*index=*/0);
+
+  // Local description not ready
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(nullptr));
+
+  EXPECT_CALL(connection_host_, SendIceCandidates(testing::_)).Times(0);
+  connection_->OnIceCandidate(ice_candidate.get());
+
+  // Signaling state not ready
+  MockSessionDescriptionInterface local_description;
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(&local_description));
+  EXPECT_CALL(*mock_webrtc_pc_, signaling_state())
+      .WillOnce(testing::Return(
+          webrtc::PeerConnectionInterface::SignalingState::kHaveRemoteOffer));
+
+  EXPECT_CALL(connection_host_, SendIceCandidates(testing::_)).Times(0);
+  connection_->OnIceCandidate(ice_candidate.get());
+
+  // Local description and signaling state ready
+  EXPECT_CALL(*mock_webrtc_pc_, local_description())
+      .WillOnce(testing::Return(&local_description));
+  EXPECT_CALL(*mock_webrtc_pc_, signaling_state())
+      .WillOnce(testing::Return(
+          webrtc::PeerConnectionInterface::SignalingState::kStable));
+
+  EXPECT_CALL(connection_host_, SendIceCandidates(testing::_))
+      .WillOnce(testing::Invoke(
+          [](std::vector<mojom::IceCandidatePtr> ice_candidates) {
+            EXPECT_EQ(3u, ice_candidates.size());
+          }));
+  connection_->OnIceCandidate(ice_candidate.get());
+}
+
 }  // namespace sharing

chrome/test/BUILD.gn

@@ -3592,6 +3592,7 @@ test("unit_tests") {
   if (!is_android) {
     sources += [
       "../browser/profiles/profile_avatar_icon_util_unittest.cc",
+      "../browser/sharing/webrtc/sharing_service_host_unittest.cc",
       "../browser/sharing/webrtc/sharing_webrtc_connection_host_unittest.cc",
       "../browser/sharing/webrtc/webrtc_signalling_host_fcm_unittest.cc",
       "../browser/storage/storage_notification_service_unittest.cc",