Test to ensure VariationsService triggers fetch on network change

This test installs a MockNetworkChangeNotifier that starts out in the
disconnected state. It then switches to WIFI and ensures
VariationsService did a request.

BUG=826930
TEST=covered by test

Change-Id: I8d193647689e8f67dfc9674bd81bd0d6da9e056b
Reviewed-on: https://chromium-review.googlesource.com/990728
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547741}

chrome/browser/chrome_browser_main.cc

@@ -850,6 +850,8 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       shutdown_watcher_(new ShutdownWatcherHelper()),
       ui_thread_profiler_(ThreadProfiler::CreateAndStartOnMainThread(
           metrics::CallStackProfileParams::UI_THREAD)),
+      should_call_pre_main_loop_start_startup_on_variations_service_(
+          !parameters.ui_task),
       profile_(NULL),
       run_message_loop_(true) {
   // If we're running tests (ui_task is non-null).
@@ -2031,9 +2033,7 @@ int ChromeBrowserMainParts::PreMainMessageLoopRunImpl() {
     const base::TimeDelta delta = base::TimeTicks::Now() - browser_open_start;
     startup_metric_utils::RecordBrowserOpenTabsDelta(delta);
 
-    // If we're running tests (ui_task is non-null), then we don't want to
-    // call StartRepeatedVariationsSeedFetch
-    if (parameters().ui_task == NULL) {
+    if (should_call_pre_main_loop_start_startup_on_variations_service_) {
       // Request new variations seed information from server.
       variations::VariationsService* variations_service =
           browser_process_->variations_service();

chrome/browser/chrome_browser_main.h

@@ -103,6 +103,8 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
   Profile* profile() { return profile_; }
 
  private:
+  friend class ChromeBrowserMainPartsTestApi;
+
   // Sets up the field trials and related initialization. Call only after
   // about:flags have been converted to switches.
   void SetupFieldTrials();
@@ -168,6 +170,11 @@ class ChromeBrowserMainParts : public content::BrowserMainParts {
   // A profiler that periodically samples stack traces on the UI thread.
   std::unique_ptr<ThreadProfiler> ui_thread_profiler_;
 
+  // Whether PerformPreMainMessageLoopStartup() is called on VariationsService.
+  // Initialized to true if |MainFunctionParams::ui_task| is null (meaning not
+  // running browser_tests), but may be forced to true for tests.
+  bool should_call_pre_main_loop_start_startup_on_variations_service_;
+
   // Members initialized after / released before main_message_loop_ ------------
 
   std::unique_ptr<BrowserProcessImpl> browser_process_;

chrome/browser/chrome_browser_main_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 "chrome/browser/chrome_browser_main.h"
+
+#include <stddef.h>
+
+#include "base/command_line.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/chrome_browser_main_extra_parts.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "components/variations/service/variations_service.h"
+#include "components/variations/variations_switches.h"
+#include "net/base/mock_network_change_notifier.h"
+#include "net/base/network_change_notifier_factory.h"
+
+// Friend of ChromeBrowserMainPartsTestApi to poke at internal state.
+class ChromeBrowserMainPartsTestApi {
+ public:
+  explicit ChromeBrowserMainPartsTestApi(ChromeBrowserMainParts* main_parts)
+      : main_parts_(main_parts) {}
+  ~ChromeBrowserMainPartsTestApi() = default;
+
+  void EnableVariationsServiceInit() {
+    main_parts_
+        ->should_call_pre_main_loop_start_startup_on_variations_service_ = true;
+  }
+
+ private:
+  ChromeBrowserMainParts* main_parts_;
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainPartsTestApi);
+};
+
+namespace {
+
+// ChromeBrowserMainExtraParts used to install a MockNetworkChangeNotifier.
+class ChromeBrowserMainExtraPartsNetFactoryInstaller
+    : public ChromeBrowserMainExtraParts {
+ public:
+  ChromeBrowserMainExtraPartsNetFactoryInstaller() = default;
+  ~ChromeBrowserMainExtraPartsNetFactoryInstaller() override {
+    // |network_change_notifier_| needs to be destroyed before |net_installer_|.
+    network_change_notifier_.reset();
+  }
+
+  net::test::MockNetworkChangeNotifier* network_change_notifier() {
+    return network_change_notifier_.get();
+  }
+
+  // ChromeBrowserMainExtraParts:
+  void PreEarlyInitialization() override {}
+  void PostMainMessageLoopStart() override {
+    ASSERT_TRUE(net::NetworkChangeNotifier::HasNetworkChangeNotifier());
+    net_installer_ =
+        std::make_unique<net::NetworkChangeNotifier::DisableForTest>();
+    network_change_notifier_ =
+        std::make_unique<net::test::MockNetworkChangeNotifier>();
+    network_change_notifier_->SetConnectionType(
+        net::NetworkChangeNotifier::CONNECTION_NONE);
+  }
+
+ private:
+  std::unique_ptr<net::test::MockNetworkChangeNotifier>
+      network_change_notifier_;
+  std::unique_ptr<net::NetworkChangeNotifier::DisableForTest> net_installer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainExtraPartsNetFactoryInstaller);
+};
+
+class ChromeBrowserMainBrowserTest : public InProcessBrowserTest {
+ public:
+  ChromeBrowserMainBrowserTest() = default;
+  ~ChromeBrowserMainBrowserTest() override = default;
+
+ protected:
+  // InProcessBrowserTest:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    // Without this (and EnableFetchForTesting() below) VariationsService won't
+    // do requests in non-branded builds.
+    command_line->AppendSwitchASCII(variations::switches::kVariationsServerURL,
+                                    "http://localhost");
+  }
+
+  void CreatedBrowserMainParts(
+      content::BrowserMainParts* browser_main_parts) override {
+    variations::VariationsService::EnableFetchForTesting();
+    ChromeBrowserMainParts* chrome_browser_main_parts =
+        static_cast<ChromeBrowserMainParts*>(browser_main_parts);
+    ChromeBrowserMainPartsTestApi(chrome_browser_main_parts)
+        .EnableVariationsServiceInit();
+    extra_parts_ = new ChromeBrowserMainExtraPartsNetFactoryInstaller();
+    chrome_browser_main_parts->AddParts(extra_parts_);
+  }
+
+  ChromeBrowserMainExtraPartsNetFactoryInstaller* extra_parts_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeBrowserMainBrowserTest);
+};
+
+// Verifies VariationsService does a request when network status changes from
+// none to connected. This is a regression test for https://crbug.com/826930.
+IN_PROC_BROWSER_TEST_F(ChromeBrowserMainBrowserTest,
+                       VariationsServiceStartsRequestOnNetworkChange) {
+  const int initial_request_count =
+      g_browser_process->variations_service()->request_count();
+  ASSERT_TRUE(extra_parts_);
+  extra_parts_->network_change_notifier()->SetConnectionType(
+      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  net::NetworkChangeNotifier::NotifyObserversOfNetworkChangeForTests(
+      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  // NotifyObserversOfNetworkChangeForTests uses PostTask, so run the loop until
+  // idle to ensure VariationsService processes the network change.
+  base::RunLoop run_loop;
+  run_loop.RunUntilIdle();
+  const int final_request_count =
+      g_browser_process->variations_service()->request_count();
+  EXPECT_EQ(initial_request_count + 1, final_request_count);
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -491,6 +491,7 @@ test("browser_tests") {
       "../browser/browsing_data/counters/sync_aware_counter_browsertest.cc",
       "../browser/browsing_data/navigation_entry_remover_browsertest.cc",
       "../browser/budget_service/budget_manager_browsertest.cc",
+      "../browser/chrome_browser_main_browsertest.cc",
       "../browser/chrome_content_browser_client_browsertest.cc",
       "../browser/chrome_content_browser_client_browsertest_chromeos.cc",
       "../browser/chrome_do_not_track_browsertest.cc",

components/variations/service/variations_service.h

@@ -171,6 +171,8 @@ class VariationsService
                         std::unique_ptr<base::FeatureList> feature_list,
                         variations::PlatformFieldTrials* platform_field_trials);
 
+  int request_count() const { return request_count_; }
+
  protected:
   // Starts the fetching process once, where |OnURLFetchComplete| is called with
   // the response. This calls DoFetchToURL with the set url.