NQE: Read cache at startup only after pref service has been initialized

Currently, the pref delegate in network quality estimator (NQE) reads
network quality prefs right after startup. However, the underlying pref
service factory is initialized asynchronously by the network_context
(see the linked bug). This results in pref delegate read at startup
returning empty value, irrespective of the actual prefs.

This CL fixes the bug by changing pref delegate to read the pref only
after the pref service has been initialized. This ensures that
when the prefs are read, they actually return what's stored on the
disk, instead of empty values.

Change-Id: I9791c673c0300d6d896f2d8e556c97f01f7e1f52
Bug: 904465
Reviewed-on: https://chromium-review.googlesource.com/c/1333127
Commit-Queue: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608544}

chrome/browser/net/network_quality_estimator_prefs_browsertest.cc

+// Copyright 2018 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 <string>
+
+#include "base/command_line.h"
+#include "base/files/file_path.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/path_service.h"
+#include "base/process/memory.h"
+#include "base/run_loop.h"
+#include "base/task/post_task.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_task_environment.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_process_impl.h"
+#include "chrome/browser/chrome_content_browser_client.h"
+#include "chrome/browser/metrics/subprocess_metrics_provider.h"
+#include "chrome/browser/net/system_network_context_manager.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/prefs/json_pref_store.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_task_traits.h"
+#include "content/public/browser/network_service_instance.h"
+#include "content/public/browser/storage_partition.h"
+#include "content/public/common/service_manager_connection.h"
+#include "content/public/common/service_names.mojom.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_base.h"
+#include "content/public/test/browser_test_utils.h"
+#include "net/base/filename_util.h"
+#include "net/nqe/effective_connection_type.h"
+#include "net/nqe/network_quality_estimator.h"
+#include "net/nqe/network_quality_estimator_params.h"
+#include "services/network/network_service.h"
+#include "services/network/public/cpp/features.h"
+#include "services/network/public/cpp/network_quality_tracker.h"
+#include "services/network/public/mojom/network_service_test.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// Retries fetching |histogram_name| until it contains at least |count| samples.
+void RetryForHistogramUntilCountReached(base::HistogramTester* histogram_tester,
+                                        const std::string& histogram_name,
+                                        size_t count) {
+  while (true) {
+    const std::vector<base::Bucket> buckets =
+        histogram_tester->GetAllSamples(histogram_name);
+    size_t total_count = 0;
+    for (const auto& bucket : buckets)
+      total_count += bucket.count;
+    if (total_count >= count)
+      return;
+    content::FetchHistogramsFromChildProcesses();
+    SubprocessMetricsProvider::MergeHistogramDeltasForTesting();
+    base::RunLoop().RunUntilIdle();
+  }
+}
+
+int GetHistogramSamplesSum(base::HistogramTester* histogram_tester,
+                           const std::string& histogram_name) {
+  const std::vector<base::Bucket> buckets =
+      histogram_tester->GetAllSamples(histogram_name);
+  size_t sum = 0;
+  for (const auto& bucket : buckets)
+    sum += (bucket.count * bucket.min);
+  return sum;
+}
+
+class TestNetworkQualityObserver
+    : public network::NetworkQualityTracker::EffectiveConnectionTypeObserver {
+ public:
+  explicit TestNetworkQualityObserver(network::NetworkQualityTracker* tracker)
+      : run_loop_wait_effective_connection_type_(
+            net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN),
+        run_loop_(std::make_unique<base::RunLoop>()),
+        tracker_(tracker),
+        effective_connection_type_(net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN) {
+    tracker_->AddEffectiveConnectionTypeObserver(this);
+  }
+
+  ~TestNetworkQualityObserver() override {
+    tracker_->RemoveEffectiveConnectionTypeObserver(this);
+  }
+
+  // NetworkQualityTracker::EffectiveConnectionTypeObserver implementation:
+  void OnEffectiveConnectionTypeChanged(
+      net::EffectiveConnectionType type) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    net::EffectiveConnectionType queried_type =
+        tracker_->GetEffectiveConnectionType();
+    EXPECT_EQ(type, queried_type);
+
+    effective_connection_type_ = type;
+    if (effective_connection_type_ != run_loop_wait_effective_connection_type_)
+      return;
+    run_loop_->Quit();
+  }
+
+  void WaitForNotification(
+      net::EffectiveConnectionType run_loop_wait_effective_connection_type) {
+    if (effective_connection_type_ == run_loop_wait_effective_connection_type)
+      return;
+    ASSERT_NE(net::EFFECTIVE_CONNECTION_TYPE_UNKNOWN,
+              run_loop_wait_effective_connection_type);
+    run_loop_wait_effective_connection_type_ =
+        run_loop_wait_effective_connection_type;
+    run_loop_->Run();
+    run_loop_.reset(new base::RunLoop());
+  }
+
+ private:
+  net::EffectiveConnectionType run_loop_wait_effective_connection_type_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+  network::NetworkQualityTracker* tracker_;
+  net::EffectiveConnectionType effective_connection_type_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkQualityObserver);
+};
+
+}  // namespace
+
+class NetworkQualityEstimatorPrefsBrowserTest : public InProcessBrowserTest {
+ public:
+  NetworkQualityEstimatorPrefsBrowserTest()
+      : network_service_enabled_(
+            base::FeatureList::IsEnabled(network::features::kNetworkService)) {
+    EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
+  }
+
+  // Simulates a network quality change.
+  void SimulateNetworkQualityChange(net::EffectiveConnectionType type) {
+    DCHECK(network_service_enabled_);
+
+    mojo::ScopedAllowSyncCallForTesting allow_sync_call;
+    content::StoragePartition* partition =
+        content::BrowserContext::GetDefaultStoragePartition(
+            browser()->profile());
+    DCHECK(partition->GetNetworkContext());
+    DCHECK(content::GetNetworkService());
+
+    network::mojom::NetworkServiceTestPtr network_service_test;
+    content::ServiceManagerConnection::GetForProcess()
+        ->GetConnector()
+        ->BindInterface(content::mojom::kNetworkServiceName,
+                        &network_service_test);
+    base::RunLoop run_loop;
+    network_service_test->SimulateNetworkQualityChange(
+        type, base::BindOnce([](base::RunLoop* run_loop) { run_loop->Quit(); },
+                             base::Unretained(&run_loop)));
+    run_loop.Run();
+  }
+
+  bool network_service_enabled() const { return network_service_enabled_; }
+
+  base::FilePath GetTempDirectory() { return temp_dir_.GetPath(); }
+
+  base::HistogramTester histogram_tester;
+
+ private:
+  const bool network_service_enabled_;
+
+  base::ScopedTempDir temp_dir_;
+};
+
+// Verify that prefs are read at startup, and the read prefs are notified to the
+// network quality estimator.
+IN_PROC_BROWSER_TEST_F(NetworkQualityEstimatorPrefsBrowserTest,
+                       ReadPrefsAtStartup) {
+  // The check below ensures that "NQE.Prefs.ReadSize" contains at least one
+  // sample. This implies that NQE was notified of the read prefs.
+  RetryForHistogramUntilCountReached(&histogram_tester, "NQE.Prefs.ReadSize",
+                                     1);
+}
+
+// Verify that prefs are read at startup.
+IN_PROC_BROWSER_TEST_F(NetworkQualityEstimatorPrefsBrowserTest,
+                       ReadPrefsAtStartupCustomPrefFile) {
+  // The check below ensures that "NQE.Prefs.ReadSize" contains at least one
+  // sample. This implies that NQE was notified of the read prefs.
+  RetryForHistogramUntilCountReached(&histogram_tester, "NQE.Prefs.ReadSize",
+                                     1);
+
+  base::HistogramTester histogram_tester2;
+
+  // Create network context with JSON pref store pointing to the temp file.
+  network::mojom::NetworkContextPtr network_context;
+  network::mojom::NetworkContextParamsPtr context_params =
+      network::mojom::NetworkContextParams::New();
+  context_params->http_server_properties_path =
+      GetTempDirectory().Append(FILE_PATH_LITERAL("Network Persistent State"));
+
+  auto state = base::MakeRefCounted<JsonPrefStore>(
+      context_params->http_server_properties_path.value());
+
+  base::DictionaryValue pref_value;
+  base::Value value("2G");
+  pref_value.Set("network_id_foo",
+                 base::Value::ToUniquePtrValue(value.Clone()));
+  state->SetValue("net.network_qualities",
+                  base::Value::ToUniquePtrValue(pref_value.Clone()), 0);
+
+  // Wait for the pending commit to finish before creating the network context.
+  base::RunLoop loop;
+  state->CommitPendingWrite(
+      base::BindOnce([](base::RunLoop* loop) { loop->Quit(); }, &loop));
+  loop.Run();
+
+  content::GetNetworkService()->CreateNetworkContext(
+      mojo::MakeRequest(&network_context), std::move(context_params));
+
+  RetryForHistogramUntilCountReached(&histogram_tester2, "NQE.Prefs.ReadSize",
+                                     1);
+  // Pref value must be read from the temp file.
+  EXPECT_LE(1,
+            GetHistogramSamplesSum(&histogram_tester2, "NQE.Prefs.ReadSize"));
+}
+
+// Verify that prefs are read at startup, and written to later.
+IN_PROC_BROWSER_TEST_F(NetworkQualityEstimatorPrefsBrowserTest, PrefsWritten) {
+  // The check below ensures that "NQE.Prefs.ReadSize" contains at least one
+  // sample. This implies that NQE was notified of the read prefs.
+  RetryForHistogramUntilCountReached(&histogram_tester, "NQE.Prefs.ReadSize",
+                                     1);
+
+  if (!network_service_enabled())
+    return;
+
+  // Change in network quality is guaranteed to trigger a pref write.
+  SimulateNetworkQualityChange(net::EFFECTIVE_CONNECTION_TYPE_2G);
+  TestNetworkQualityObserver network_quality_observer(
+      g_browser_process->network_quality_tracker());
+  network_quality_observer.WaitForNotification(
+      net::EFFECTIVE_CONNECTION_TYPE_2G);
+
+  RetryForHistogramUntilCountReached(&histogram_tester, "NQE.Prefs.WriteCount",
+                                     1);
+}

chrome/test/BUILD.gn

@@ -690,6 +690,7 @@ test("browser_tests") {
       "../browser/net/netinfo_network_quality_estimator_holdback_browsertest.cc",
       "../browser/net/network_connection_tracker_browsertest.cc",
       "../browser/net/network_context_configuration_browsertest.cc",
+      "../browser/net/network_quality_estimator_prefs_browsertest.cc",
       "../browser/net/network_quality_netinfo_browsertest.cc",
       "../browser/net/network_quality_tracker_browsertest.cc",
       "../browser/net/network_request_metrics_browsertest.cc",

services/network/network_qualities_pref_delegate.cc

@@ -56,6 +56,12 @@ class PrefDelegateImpl
   DISALLOW_COPY_AND_ASSIGN(PrefDelegateImpl);
 };
 
+// Returns true if |pref_service| has been initialized.
+bool IsPrefServiceInitialized(PrefService* pref_service) {
+  return pref_service->GetInitializationStatus() !=
+         PrefService::INITIALIZATION_STATUS_WAITING;
+}
+
 }  // namespace
 
 namespace network {
@@ -63,16 +69,32 @@ namespace network {
 NetworkQualitiesPrefDelegate::NetworkQualitiesPrefDelegate(
     PrefService* pref_service,
     net::NetworkQualityEstimator* network_quality_estimator)
-    : prefs_manager_(std::make_unique<PrefDelegateImpl>(pref_service)) {
+    : prefs_manager_(std::make_unique<PrefDelegateImpl>(pref_service)),
+      network_quality_estimator_(network_quality_estimator),
+      weak_ptr_factory_(this) {
   DCHECK(pref_service);
-  DCHECK(network_quality_estimator);
-  prefs_manager_.InitializeOnNetworkThread(network_quality_estimator);
+  DCHECK(network_quality_estimator_);
+
+  if (IsPrefServiceInitialized(pref_service)) {
+    OnPrefServiceInitialized(true);
+  } else {
+    // Register for a callback that will be invoked when |pref_service| is
+    // initialized.
+    pref_service->AddPrefInitObserver(
+        base::BindOnce(&NetworkQualitiesPrefDelegate::OnPrefServiceInitialized,
+                       weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 NetworkQualitiesPrefDelegate::~NetworkQualitiesPrefDelegate() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
+void NetworkQualitiesPrefDelegate::OnPrefServiceInitialized(bool success) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  prefs_manager_.InitializeOnNetworkThread(network_quality_estimator_);
+}
+
 void NetworkQualitiesPrefDelegate::ClearPrefs() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   prefs_manager_.ClearPrefs();
@@ -80,7 +102,7 @@ void NetworkQualitiesPrefDelegate::ClearPrefs() {
 
 // static
 void NetworkQualitiesPrefDelegate::RegisterPrefs(PrefRegistrySimple* registry) {
-  registry->RegisterDictionaryPref(kNetworkQualities, PrefRegistry::LOSSY_PREF);
+  registry->RegisterDictionaryPref(kNetworkQualities);
 }
 
 std::map<net::nqe::internal::NetworkID,

services/network/network_qualities_pref_delegate.h

@@ -9,6 +9,7 @@
 
 #include "base/component_export.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "net/nqe/cached_network_quality.h"
 #include "net/nqe/network_id.h"
@@ -44,12 +45,20 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkQualitiesPrefDelegate {
   ForceReadPrefsForTesting() const;
 
  private:
+  // Called when pref service is initialized.
+  void OnPrefServiceInitialized(bool success);
+
   // Prefs manager that is owned by this service. Created on the UI thread, but
   // used and deleted on the IO thread.
   net::NetworkQualitiesPrefsManager prefs_manager_;
 
+  // Guaranteed to be non-null during the lifetime of |this|.
+  net::NetworkQualityEstimator* network_quality_estimator_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
+  base::WeakPtrFactory<NetworkQualitiesPrefDelegate> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkQualitiesPrefDelegate);
 };