[CastMRP] Fix a couple of bugs in CMSSImpl.

- On channel error, erase the sink from |current_sinks_map|.
  crrev.com/541271 added a check in OpenChannel() to short circuit if
  a sink already exists on the map. In order for retry on error to kick
  in properly, we will need to erase the entry in OnError().
- In OnChannelOpenSucceeded, erase stale sink with the same IP. This is
  done to maintain the invariant of having at most 1 sink of a given ID
  in the map. Stale sink is defined as having the same sink ID but
  different IP endpoint than the current one.
- In OnChannelOpenFailed, when erasing the sink from
  |current_sinks_map_|, also verify the sink ID. It is possible that
  a different sink now occcupies the IP endpoint that it is trying to
  erase.

Change-Id: I12bfd5af5f664c4773e7587dc30c6709d2352021
Bug: 698940
Reviewed-on: https://chromium-review.googlesource.com/961555Reviewed-by: Bin Zhao <zhaobin@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543439}

chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.cc

@@ -350,6 +350,16 @@ void CastMediaSinkServiceImpl::OnError(const cast_channel::CastSocket& socket,
         base::BindOnce(&CastMediaSinkServiceImpl::OpenChannel, GetWeakPtr(),
                        ip_endpoint, cast_sink_it->second, nullptr,
                        SinkSource::kConnectionRetry));
+    // We erase the sink here so that OpenChannel would not find an existing
+    // sink.
+    // Note: a better longer term solution is to introduce a state field to the
+    // sink. We would set it to ERROR here. In OpenChannel(), we would check
+    // create a socket only if the state is not already CONNECTED.
+    if (observer_)
+      observer_->OnSinkRemoved(cast_sink_it->second);
+
+    current_sinks_map_.erase(cast_sink_it);
+    MediaSinkServiceBase::RestartTimer();
     return;
   }
 
@@ -382,6 +392,9 @@ void CastMediaSinkServiceImpl::OnNetworksChanged(
     }
     sink_cache_[last_network_id] = std::move(current_sinks);
   }
+
+  // TODO(imcheng): Maybe this should clear |current_sinks_map_| and call
+  // |RestartTimer()| so it is more responsive?
   if (IsNetworkIdUnknownOrDisconnected(network_id))
     return;
 
@@ -511,7 +524,7 @@ void CastMediaSinkServiceImpl::OnChannelErrorMayRetry(
              << ip_endpoint.ToString() << " [error_state]: "
              << cast_channel::ChannelErrorToString(error_state);
 
-    OnChannelOpenFailed(ip_endpoint);
+    OnChannelOpenFailed(ip_endpoint, cast_sink);
     CastAnalytics::RecordCastChannelConnectResult(
         MediaRouterChannelConnectResults::FAILURE);
     return;
@@ -572,6 +585,18 @@ void CastMediaSinkServiceImpl::OnChannelOpenSucceeded(
     sink_it->second = cast_sink;
   }
 
+  // If the sink was under a different IP address previously, remove it from
+  // |current_sinks_map_|.
+  auto old_sink_it = std::find_if(
+      current_sinks_map_.begin(), current_sinks_map_.end(),
+      [&cast_sink, &ip_endpoint](
+          const std::pair<net::IPEndPoint, MediaSinkInternal>& entry) {
+        return !(entry.first == ip_endpoint) &&
+               entry.second.sink().id() == cast_sink.sink().id();
+      });
+  if (old_sink_it != current_sinks_map_.end())
+    current_sinks_map_.erase(old_sink_it);
+
   if (observer_)
     observer_->OnSinkAddedOrUpdated(cast_sink, socket);
 
@@ -580,9 +605,14 @@ void CastMediaSinkServiceImpl::OnChannelOpenSucceeded(
 }
 
 void CastMediaSinkServiceImpl::OnChannelOpenFailed(
-    const net::IPEndPoint& ip_endpoint) {
+    const net::IPEndPoint& ip_endpoint,
+    const MediaSinkInternal& sink) {
+  // It is possible that the old sink in |current_sinks_map_| is replaced with
+  // a new sink if a network change happened. Check the sink ID to make sure
+  // this is the sink we want to erase.
   auto it = current_sinks_map_.find(ip_endpoint);
-  if (it == current_sinks_map_.end())
+  if (it == current_sinks_map_.end() ||
+      it->second.sink().id() != sink.sink().id())
     return;
 
   if (observer_)

chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl.h

@@ -169,6 +169,10 @@ class CastMediaSinkServiceImpl
                            TestInitRetryParametersWithDefaultValue);
   FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
                            TestOnDialSinkAddedSkipsIfNonCastDevice);
+  FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
+                           TestOnChannelErrorRetry);
+  FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
+                           OpenChannelNewIPSameSink);
 
   // Holds Finch field trial parameters controlling Cast channel retry strategy.
   struct RetryParams {
@@ -276,7 +280,7 @@ class CastMediaSinkServiceImpl
                               cast_channel::ChannelError error_state,
                               SinkSource sink_source);
 
-  // Invoked when opening cast channel on IO thread succeeds.
+  // Invoked when opening cast channel succeeds.
   // |cast_sink|: Cast sink created from mDNS service description or DIAL sink.
   // |socket|: raw pointer of newly created cast channel. Does not take
   // ownership of |socket|.
@@ -284,11 +288,12 @@ class CastMediaSinkServiceImpl
                               cast_channel::CastSocket* socket,
                               SinkSource sink_source);
 
-  // Invoked when opening cast channel on IO thread fails after all retry
+  // Invoked when opening cast channel fails after all retry
   // attempts.
   // |ip_endpoint|: ip endpoint of cast channel failing to connect to.
-  // |sink_source|: Method of sink discovery.
-  void OnChannelOpenFailed(const net::IPEndPoint& ip_endpoint);
+  // |sink|: The sink for which channel open failed.
+  void OnChannelOpenFailed(const net::IPEndPoint& ip_endpoint,
+                           const MediaSinkInternal& sink);
 
   // Returns whether the given DIAL-discovered |sink| is probably a non-Cast
   // device. This is heuristically determined by two things: |sink| has been

chrome/browser/media/router/discovery/mdns/cast_media_sink_service_impl_unittest.cc

@@ -403,11 +403,66 @@ TEST_F(CastMediaSinkServiceImplTest, TestMultipleOpenChannels) {
   media_sink_service_impl_.OnDiscoveryComplete();
 }
 
+TEST_F(CastMediaSinkServiceImplTest, OpenChannelNewIPSameSink) {
+  MediaSinkInternal cast_sink1 = CreateCastSink(1);
+  net::IPEndPoint ip_endpoint1 = cast_sink1.cast_data().ip_endpoint;
+
+  cast_channel::MockCastSocket socket;
+  socket.set_id(1);
+
+  base::SimpleTestClock clock;
+  base::Time start_time = base::Time::Now();
+  clock.SetNow(start_time);
+  media_sink_service_impl_.SetClockForTest(&clock);
+
+  EXPECT_CALL(*mock_cast_socket_service_,
+              OpenSocketInternal(ip_endpoint1, _, _))
+      .WillRepeatedly(
+          Invoke([&](const auto& ip_endpoint1, auto* net_log, auto open_cb) {
+            std::move(open_cb).Run(&socket);
+          }));
+  std::vector<MediaSinkInternal> sinks1 = {cast_sink1};
+  media_sink_service_impl_.OpenChannels(
+      sinks1, CastMediaSinkServiceImpl::SinkSource::kMdns);
+
+  mock_time_task_runner_->FastForwardUntilNoTasksRemain();
+  EXPECT_EQ(1u, media_sink_service_impl_.current_sinks_map_.size());
+
+  // |cast_sink1| changed IP address and is discovered by mdns before it is
+  // removed from |media_sink_service_impl_| first.
+  net::IPEndPoint ip_endpoint2 = CreateIPEndPoint(2);
+  CastSinkExtraData extra_data = cast_sink1.cast_data();
+  extra_data.ip_endpoint = ip_endpoint2;
+  cast_sink1.set_cast_data(extra_data);
+
+  EXPECT_CALL(*mock_cast_socket_service_,
+              OpenSocketInternal(ip_endpoint2, _, _))
+      .WillRepeatedly(
+          Invoke([&](const auto& ip_endpoint1, auto* net_log, auto open_cb) {
+            std::move(open_cb).Run(&socket);
+          }));
+
+  std::vector<MediaSinkInternal> updated_sinks1 = {cast_sink1};
+  media_sink_service_impl_.OpenChannels(
+      updated_sinks1, CastMediaSinkServiceImpl::SinkSource::kMdns);
+
+  // The entry under old IPEndPoint is removed and replaced with new IPEndPoint.
+  mock_time_task_runner_->FastForwardUntilNoTasksRemain();
+  const auto& current_sinks_map = media_sink_service_impl_.current_sinks_map_;
+  EXPECT_EQ(1u, current_sinks_map.size());
+  auto sink_it = current_sinks_map.find(ip_endpoint2);
+  ASSERT_TRUE(sink_it != current_sinks_map.end());
+  EXPECT_EQ(cast_sink1, sink_it->second);
+}
+
 TEST_F(CastMediaSinkServiceImplTest, TestOnChannelOpenFailed) {
   auto cast_sink = CreateCastSink(1);
   net::IPEndPoint ip_endpoint1 = CreateIPEndPoint(1);
   cast_channel::MockCastSocket socket;
   socket.set_id(1);
+  socket.SetIPEndpoint(ip_endpoint1);
+
+  auto cast_sink2 = CreateCastSink(2);
 
   EXPECT_CALL(observer_, OnSinkAddedOrUpdated(cast_sink, &socket));
   media_sink_service_impl_.OnChannelOpenSucceeded(
@@ -415,10 +470,47 @@ TEST_F(CastMediaSinkServiceImplTest, TestOnChannelOpenFailed) {
 
   EXPECT_EQ(1u, media_sink_service_impl_.current_sinks_map_.size());
 
+  // OnChannelOpenFailed called with mismatched sink: no-op.
+  EXPECT_CALL(observer_, OnSinkRemoved(_)).Times(0);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, cast_sink2);
+  EXPECT_FALSE(media_sink_service_impl_.current_sinks_map_.empty());
+
+  EXPECT_CALL(observer_, OnSinkRemoved(cast_sink));
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, cast_sink);
+  EXPECT_TRUE(media_sink_service_impl_.current_sinks_map_.empty());
+}
+
+TEST_F(CastMediaSinkServiceImplTest, TestOnChannelErrorRetry) {
+  auto cast_sink = CreateCastSink(1);
+  net::IPEndPoint ip_endpoint1 = CreateIPEndPoint(1);
+  cast_channel::MockCastSocket socket;
+  socket.set_id(1);
   socket.SetIPEndpoint(ip_endpoint1);
+  EXPECT_CALL(socket, ready_state())
+      .WillOnce(Return(cast_channel::ReadyState::OPEN));
+
+  EXPECT_CALL(observer_, OnSinkAddedOrUpdated(cast_sink, &socket));
+  media_sink_service_impl_.OnChannelOpenSucceeded(
+      cast_sink, &socket, CastMediaSinkServiceImpl::SinkSource::kMdns);
+
+  // Sink is removed on |OnError|, but we will retry.
   EXPECT_CALL(observer_, OnSinkRemoved(cast_sink));
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
+  EXPECT_CALL(*mock_cast_socket_service_,
+              OpenSocketInternal(ip_endpoint1, _, _))
+      .WillRepeatedly(
+          Invoke([&](const auto& ip_endpoint1, auto* net_log, auto open_cb) {
+            std::move(open_cb).Run(&socket);
+          }));
+  media_sink_service_impl_.OnError(socket,
+                                   cast_channel::ChannelError::PING_TIMEOUT);
+
   EXPECT_TRUE(media_sink_service_impl_.current_sinks_map_.empty());
+
+  // Retry succeeds and sink is added back.
+  EXPECT_CALL(observer_, OnSinkAddedOrUpdated(cast_sink, &socket));
+  mock_time_task_runner_->FastForwardUntilNoTasksRemain();
+
+  EXPECT_EQ(1u, media_sink_service_impl_.current_sinks_map_.size());
 }
 
 TEST_F(CastMediaSinkServiceImplTest,
@@ -666,8 +758,8 @@ TEST_F(CastMediaSinkServiceImplTest, CacheSinksForKnownNetwork) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2, sink2);
 
   MediaSinkInternal sink3 = CreateCastSink(3);
   net::IPEndPoint ip_endpoint3 = CreateIPEndPoint(3);
@@ -738,7 +830,7 @@ TEST_F(CastMediaSinkServiceImplTest, CacheContainsOnlyResolvedSinks) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
 
   MediaSinkInternal sink3 = CreateCastSink(3);
   net::IPEndPoint ip_endpoint3 = CreateIPEndPoint(3);
@@ -791,7 +883,7 @@ TEST_F(CastMediaSinkServiceImplTest, CacheUpdatedOnChannelOpenFailed) {
   ExpectOpenSocketInternal(&socket1);
   media_sink_service_impl_.OpenChannels(
       sink_list1, CastMediaSinkServiceImpl::SinkSource::kMdns);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
 
   // Connect to a new network with different sinks.
   fake_network_info_.clear();
@@ -872,8 +964,8 @@ TEST_F(CastMediaSinkServiceImplTest, UnknownNetworkNoCache) {
       net::NetworkChangeNotifier::CONNECTION_NONE);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2, sink2);
 
   MediaSinkInternal sink3 = CreateCastSink(3);
   net::IPEndPoint ip_endpoint3 = CreateIPEndPoint(3);
@@ -941,8 +1033,8 @@ TEST_F(CastMediaSinkServiceImplTest, CacheUpdatedForKnownNetwork) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2, sink2);
 
   MediaSinkInternal sink3 = CreateCastSink(3);
   net::IPEndPoint ip_endpoint3 = CreateIPEndPoint(3);
@@ -963,7 +1055,7 @@ TEST_F(CastMediaSinkServiceImplTest, CacheUpdatedForKnownNetwork) {
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
 
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint3);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint3, sink3);
 
   // Resolution will fail for cached sinks.
   socket1.SetErrorState(cast_channel::ChannelError::CONNECT_ERROR);
@@ -994,7 +1086,7 @@ TEST_F(CastMediaSinkServiceImplTest, CacheUpdatedForKnownNetwork) {
       net::NetworkChangeNotifier::CONNECTION_NONE);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint4);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint4, sink4);
 
   // Reconnect and expect only |sink4| to be cached.
   EXPECT_CALL(*mock_cast_socket_service_,
@@ -1034,6 +1126,12 @@ TEST_F(CastMediaSinkServiceImplTest, CacheDialDiscoveredSinks) {
       sink_list1, CastMediaSinkServiceImpl::SinkSource::kMdns);
   media_sink_service_impl_.OnDialSinkAdded(sink2_dial);
 
+  // CastMediaSinkServiceImpl generates a Cast sink based on |sink2_dial|.
+  auto sink2_it =
+      media_sink_service_impl_.current_sinks_map_.find(ip_endpoint2);
+  ASSERT_TRUE(sink2_it != media_sink_service_impl_.current_sinks_map_.end());
+  MediaSinkInternal sink2_cast_from_dial = sink2_it->second;
+
   // Connect to a new network with different sinks.
   fake_network_info_.clear();
   net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
@@ -1046,8 +1144,9 @@ TEST_F(CastMediaSinkServiceImplTest, CacheDialDiscoveredSinks) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1_cast);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2,
+                                               sink2_cast_from_dial);
 
   MediaSinkInternal sink3_cast = CreateCastSink(3);
   MediaSinkInternal sink4_dial = CreateDialSink(4);
@@ -1134,8 +1233,8 @@ TEST_F(CastMediaSinkServiceImplTest, DualDiscoveryDoesntDuplicateCacheItems) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1_cast);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1_dial);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1_cast, sink1_cast);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1_dial, sink1_dial);
 
   MediaSinkInternal sink2_cast = CreateCastSink(2);
   net::IPEndPoint ip_endpoint2 = CreateIPEndPoint(2);
@@ -1197,8 +1296,8 @@ TEST_F(CastMediaSinkServiceImplTest, CacheSinksForDirectNetworkChange) {
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   content::RunAllTasksUntilIdle();
   mock_time_task_runner_->FastForwardUntilNoTasksRemain();
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1);
-  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint1, sink1);
+  media_sink_service_impl_.OnChannelOpenFailed(ip_endpoint2, sink2);
 
   MediaSinkInternal sink3 = CreateCastSink(3);
   net::IPEndPoint ip_endpoint3 = CreateIPEndPoint(3);