Fix cache bugs in CastMediaSinkServiceImpl

This change fixes two bugs with caching of media sinks and a potential
initialization race that could also affect the cache's performance.

The first fix is to allow network ID changes of network1->network2 to
cache sinks for network1 rather than only caching for
network1->disconnected/unknown.  The cache originally used the latter
logic because net::NetworkChangeNotifier broadcasts a disconnect between
any two network changes.  However, DiscoveryNetworkMonitor doesn't
necessarily forward this behavior to its observers because it actually
calculates the current network ID on every net::NetworkChangeNotifier
notification.  Not caching in the former case could account for some of
the cache's low utilization currently seen in metrics.

The second fix is to add CastMediaSinkServiceImpl as a
DiscoveryNetworkMonitor::Observer from the correct sequence so it
receives notifications on the correct sequence.  This in turn
exacerbates a race between the first network ID update and
CastMediaSinkServiceImpl's AddObserver call.  The race was therefore
also addressed in this change.

Bug: 782788, 782859
Change-Id: I064bd184508e66f1806e55dc5afe1eea907836ea
Reviewed-on: https://chromium-review.googlesource.com/760667
Commit-Queue: Brandon Tolsch <btolsch@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517134}

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

@@ -181,7 +181,6 @@ CastMediaSinkServiceImpl::CastMediaSinkServiceImpl(
   DETACH_FROM_SEQUENCE(sequence_checker_);
   DCHECK(cast_socket_service_);
   DCHECK(network_monitor_);
-  network_monitor_->AddObserver(this);
   cast_socket_service_->AddObserver(this);
 
   retry_params_ = RetryParams::GetFromFieldTrialParam();
@@ -224,7 +223,6 @@ CastMediaSinkServiceImpl::CastMediaSinkServiceImpl(
 CastMediaSinkServiceImpl::~CastMediaSinkServiceImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   cast_channel::CastSocketService::GetInstance()->RemoveObserver(this);
-  network_monitor_->RemoveObserver(this);
 }
 
 void CastMediaSinkServiceImpl::SetTaskRunnerForTest(
@@ -241,11 +239,18 @@ void CastMediaSinkServiceImpl::SetClockForTest(
 void CastMediaSinkServiceImpl::Start() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   MediaSinkServiceBase::StartTimer();
+  // This call to |GetNetworkId| ensures that we get the current network ID at
+  // least once during startup in case |AddObserver| occurs after the first
+  // round of notifications has already been dispatched.
+  network_monitor_->GetNetworkId(base::BindOnce(
+      &CastMediaSinkServiceImpl::OnNetworksChanged, AsWeakPtr()));
+  network_monitor_->AddObserver(this);
 }
 
 void CastMediaSinkServiceImpl::Stop() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   MediaSinkServiceBase::StopTimer();
+  network_monitor_->RemoveObserver(this);
 }
 
 void CastMediaSinkServiceImpl::OnFetchCompleted() {
@@ -338,21 +343,28 @@ void CastMediaSinkServiceImpl::OnMessage(
 
 void CastMediaSinkServiceImpl::OnNetworksChanged(
     const std::string& network_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Although DiscoveryNetworkMonitor guarantees this condition won't be true
+  // from its Observer interface, the callback from |AddNetworkChangeObserver|
+  // could cause this to happen.
+  if (network_id == current_network_id_) {
+    return;
+  }
   std::string last_network_id = current_network_id_;
   current_network_id_ = network_id;
   dial_sink_failure_count_.clear();
-  if (IsNetworkIdUnknownOrDisconnected(network_id)) {
-    if (!IsNetworkIdUnknownOrDisconnected(last_network_id)) {
-      // Collect current sinks even if OnFetchCompleted hasn't collected the
-      // latest sinks.
-      std::vector<MediaSinkInternal> current_sinks;
-      for (const auto& sink_it : current_sinks_map_) {
-        current_sinks.push_back(sink_it.second);
-      }
-      sink_cache_[last_network_id] = std::move(current_sinks);
+  if (!IsNetworkIdUnknownOrDisconnected(last_network_id)) {
+    // Collect current sinks even if OnFetchCompleted hasn't collected the
+    // latest sinks.
+    std::vector<MediaSinkInternal> current_sinks;
+    for (const auto& sink_it : current_sinks_map_) {
+      current_sinks.push_back(sink_it.second);
     }
-    return;
+    sink_cache_[last_network_id] = std::move(current_sinks);
   }
+  if (IsNetworkIdUnknownOrDisconnected(network_id))
+    return;
+
   auto cache_entry = sink_cache_.find(network_id);
   // Check if we have any cached sinks for this network ID.
   if (cache_entry == sink_cache_.end())

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

@@ -113,6 +113,8 @@ class CastMediaSinkServiceImpl
                            CacheDialDiscoveredSinks);
   FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
                            DualDiscoveryDoesntDuplicateCacheItems);
+  FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
+                           CacheSinksForDirectNetworkChange);
   FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest, TestAttemptConnection);
   FRIEND_TEST_ALL_PREFIXES(CastMediaSinkServiceImplTest,
                            TestInitRetryParametersWithFeatureDisabled);