Reland "Variations service now checks explicitely if previous attempt was HTTP."

This reverts commit 8d41f8c9.

Reason for revert: This CL reverts the revert, and fixes the flakiness in the test by allowing one or two requests in the test.

Bug: 904108, 902727

Original change's description:
> Revert "Variations service now checks explicitely if previous attempt was HTTP."
>
> This reverts commit 91d24688.
>
> Reason for revert: This CL made ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange flaky (crbug.com/904108) reverting while investigating why.
>
> Original change's description:
> > Variations service now checks explicitely if previous attempt was HTTP.
> >
> > Variations service now sets a flag if the last request was an HTTP
> > retry, and uses it to decide whether to retry again, instead of
> > relying on the scheme of the final URL. Also, retries are now disabled
> > if the fallback url is HTTPS.
> >
> > Bug: 902727
> > Change-Id: Ibd6df4a22bc6302b231aff0ae32f8e1c8a1f277c
> > Reviewed-on: https://chromium-review.googlesource.com/c/1325035
> > Commit-Queue: Carlos IL <carlosil@chromium.org>
> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#607022}
>
> TBR=asvitkine@chromium.org,carlosil@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 902727
> Change-Id: I67036ea20097f889e71de5179a2e4510a201388d
> Reviewed-on: https://chromium-review.googlesource.com/c/1334421
> Reviewed-by: Carlos IL <carlosil@chromium.org>
> Commit-Queue: Carlos IL <carlosil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607882}

Change-Id: I5618dd7c48849ddf1fc8cd8f613ba7d83ebc66c0
Reviewed-on: https://chromium-review.googlesource.com/c/1335666
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611152}

chrome/browser/chrome_browser_main_browsertest.cc

@@ -9,6 +9,7 @@
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.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"
@@ -130,6 +131,10 @@ class ChromeBrowserMainBrowserTest : public InProcessBrowserTest {
  public:
   ChromeBrowserMainBrowserTest() {
     net::NetworkChangeNotifier::SetTestNotificationsOnly(true);
+    // Since the test currently performs an actual request to localhost (which
+    // is expected to fail since no variations server is running), retries are
+    // disabled to prevent race conditions from causing flakiness in tests.
+    scoped_feature_list_.InitAndDisableFeature(variations::kHttpRetryFeature);
   }
 
  protected:
@@ -159,12 +164,15 @@ class ChromeBrowserMainBrowserTest : public InProcessBrowserTest {
   ChromeBrowserMainExtraPartsNetFactoryInstaller* extra_parts_ = nullptr;
 
  private:
+  base::test::ScopedFeatureList scoped_feature_list_;
 
   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.
+// TODO(crbug.com/905714): This test should use a mock variations server
+// instead of performing an actual request.
 IN_PROC_BROWSER_TEST_F(ChromeBrowserMainBrowserTest,
                        VariationsServiceStartsRequestOnNetworkChange) {
   const int initial_request_count =

components/variations/service/variations_service.cc

@@ -64,11 +64,12 @@
 #endif  // OS_ANDROID
 
 namespace variations {
-namespace {
 
 const base::Feature kHttpRetryFeature{"VariationsHttpRetry",
                                       base::FEATURE_ENABLED_BY_DEFAULT};
 
+namespace {
+
 // Constants used for encrypting the if-none-match header if we are retrieving a
 // seed over http.
 const char kEncryptedMessageLabel[] = "chrome variations";
@@ -296,6 +297,7 @@ VariationsService::VariationsService(
                                MaybeImportFirstRunSeed(local_state),
                                base::BindOnce(&OnInitialSeedStored)),
                            ui_string_overrider),
+      last_request_was_http_retry_(false),
       weak_ptr_factory_(this) {
   DCHECK(client_);
   DCHECK(resource_request_allowed_notifier_);
@@ -482,49 +484,6 @@ void VariationsService::DoActualFetch() {
   DoFetchFromURL(variations_server_url_, false);
 }
 
-bool VariationsService::StoreSeed(const std::string& seed_data,
-                                  const std::string& seed_signature,
-                                  const std::string& country_code,
-                                  base::Time date_fetched,
-                                  bool is_delta_compressed,
-                                  bool is_gzip_compressed,
-                                  bool fetched_insecurely) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
-  std::unique_ptr<VariationsSeed> seed(new VariationsSeed);
-  if (!field_trial_creator_.seed_store()->StoreSeedData(
-          seed_data, seed_signature, country_code, date_fetched,
-          is_delta_compressed, is_gzip_compressed, fetched_insecurely,
-          seed.get())) {
-    return false;
-  }
-
-  RecordSuccessfulFetch();
-
-  // Now, do simulation to determine if there are any kill-switches that were
-  // activated by this seed. To do this, first get the Chrome version to do a
-  // simulation with, which must be done on a background thread, and then do the
-  // actual simulation on the UI thread.
-  base::PostTaskWithTraitsAndReplyWithResult(
-      FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
-      client_->GetVersionForSimulationCallback(),
-      base::Bind(&VariationsService::PerformSimulationWithVersion,
-                 weak_ptr_factory_.GetWeakPtr(), base::Passed(&seed)));
-  return true;
-}
-
-std::unique_ptr<const base::FieldTrial::EntropyProvider>
-VariationsService::CreateLowEntropyProvider() {
-  return state_manager_->CreateLowEntropyProvider();
-}
-
-void VariationsService::InitResourceRequestedAllowedNotifier() {
-  // ResourceRequestAllowedNotifier does not install an observer if there is no
-  // NetworkChangeNotifier, which results in never being notified of changes to
-  // network status.
-  resource_request_allowed_notifier_->Init(this, false /* leaky */);
-}
-
 bool VariationsService::DoFetchFromURL(const GURL& url, bool is_http_retry) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(IsFetchingEnabled());
@@ -539,6 +498,8 @@ bool VariationsService::DoFetchFromURL(const GURL& url, bool is_http_retry) {
   if (pending_seed_request_)
     return false;
 
+  last_request_was_http_retry_ = is_http_retry;
+
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("chrome_variations_service", R"(
         semantics {
@@ -609,6 +570,49 @@ bool VariationsService::DoFetchFromURL(const GURL& url, bool is_http_retry) {
   return true;
 }
 
+bool VariationsService::StoreSeed(const std::string& seed_data,
+                                  const std::string& seed_signature,
+                                  const std::string& country_code,
+                                  base::Time date_fetched,
+                                  bool is_delta_compressed,
+                                  bool is_gzip_compressed,
+                                  bool fetched_insecurely) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  std::unique_ptr<VariationsSeed> seed(new VariationsSeed);
+  if (!field_trial_creator_.seed_store()->StoreSeedData(
+          seed_data, seed_signature, country_code, date_fetched,
+          is_delta_compressed, is_gzip_compressed, fetched_insecurely,
+          seed.get())) {
+    return false;
+  }
+
+  RecordSuccessfulFetch();
+
+  // Now, do simulation to determine if there are any kill-switches that were
+  // activated by this seed. To do this, first get the Chrome version to do a
+  // simulation with, which must be done on a background thread, and then do the
+  // actual simulation on the UI thread.
+  base::PostTaskWithTraitsAndReplyWithResult(
+      FROM_HERE, {base::MayBlock(), base::TaskPriority::BEST_EFFORT},
+      client_->GetVersionForSimulationCallback(),
+      base::Bind(&VariationsService::PerformSimulationWithVersion,
+                 weak_ptr_factory_.GetWeakPtr(), base::Passed(&seed)));
+  return true;
+}
+
+std::unique_ptr<const base::FieldTrial::EntropyProvider>
+VariationsService::CreateLowEntropyProvider() {
+  return state_manager_->CreateLowEntropyProvider();
+}
+
+void VariationsService::InitResourceRequestedAllowedNotifier() {
+  // ResourceRequestAllowedNotifier does not install an observer if there is no
+  // NetworkChangeNotifier, which results in never being notified of changes to
+  // network status.
+  resource_request_allowed_notifier_->Init(this, false /* leaky */);
+}
+
 void VariationsService::StartRepeatedVariationsSeedFetch() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -672,10 +676,13 @@ void VariationsService::OnSimpleLoaderComplete(
     response_code = headers->response_code();
   }
   bool is_success = headers && (net_error == net::OK);
-  bool was_https =
+  // We use the final URL HTTPS/HTTP value to pass to StoreSeed, since
+  // signature validation should be forced on for any HTTP fetch, including
+  // redirects from HTTPS to HTTP.
+  bool final_url_was_https =
       pending_seed_request_->GetFinalURL().SchemeIs(url::kHttpsScheme);
   pending_seed_request_.reset();
-  if (was_https) {
+  if (last_request_was_http_retry_) {
     base::UmaHistogramSparse("Variations.SeedFetchResponseOrErrorCode",
                              is_success ? response_code : net_error);
   } else {
@@ -694,17 +701,12 @@ void VariationsService::OnSimpleLoaderComplete(
       return;
     }
 
-    // If the current fetch attempt was over an HTTPS connection, retry the
-    // fetch immediately over an HTTP connection.
-    // Currently we only do this if if the 'VariationsHttpRetry' feature is
-    // enabled.
-    if (was_https && !insecure_variations_server_url_.is_empty()) {
-      // When re-trying via HTTP, return immediately. OnURLFetchComplete() will
-      // be called with the result of that retry.
-      if (DoFetchFromURL(insecure_variations_server_url_, true))
-        return;
+    if (MaybeRetryOverHTTP()) {
+      // If the retry was successfully started, return immediately,
+      // OnSimpleLoaderComplete will be called again when the new fetch
+      // finishes.
+      return;
     }
-    return;
   }
 
   const base::TimeDelta latency =
@@ -756,7 +758,7 @@ void VariationsService::OnSimpleLoaderComplete(
   const std::string country_code = GetHeaderValue(headers.get(), "X-Country");
   const bool store_success =
       StoreSeed(*response_body, signature, country_code, response_date,
-                is_delta_compressed, is_gzip_compressed, !was_https);
+                is_delta_compressed, is_gzip_compressed, !final_url_was_https);
   if (!store_success && is_delta_compressed) {
     disable_deltas_for_next_request_ = true;
     // |request_scheduler_| will be null during unit tests.
@@ -765,6 +767,18 @@ void VariationsService::OnSimpleLoaderComplete(
   }
 }
 
+bool VariationsService::MaybeRetryOverHTTP() {
+  // If the current fetch attempt was over an HTTPS connection, retry the
+  // fetch immediately over an HTTP connection. We only do this if an insecure
+  // variations URL is set and its scheme is HTTP.
+  if (!last_request_was_http_retry_ &&
+      !insecure_variations_server_url_.is_empty() &&
+      insecure_variations_server_url_.SchemeIs(url::kHttpScheme)) {
+    return DoFetchFromURL(insecure_variations_server_url_, true);
+  }
+  return false;
+}
+
 void VariationsService::OnResourceRequestsAllowed() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ -816,6 +830,10 @@ void VariationsService::PerformSimulationWithVersion(
   NotifyObservers(result);
 }
 
+bool VariationsService::CallMaybeRetryOverHTTPForTesting() {
+  return MaybeRetryOverHTTP();
+}
+
 void VariationsService::RecordSuccessfulFetch() {
   field_trial_creator_.seed_store()->RecordLastFetchTime();
   safe_seed_manager_.RecordSuccessfulFetch(field_trial_creator_.seed_store());

components/variations/service/variations_service.h

@@ -55,6 +55,10 @@ class VariationsSeed;
 
 namespace variations {
 
+// If enabled, seed fetches will be retried over HTTP after an HTTPS request
+// fails.
+extern const base::Feature kHttpRetryFeature;
+
 // Used to setup field trials based on stored variations seed data, and fetch
 // new seed data from the variations server.
 class VariationsService
@@ -189,6 +193,11 @@ class VariationsService
   // the response. This calls DoFetchToURL with the set url.
   virtual void DoActualFetch();
 
+  // Attempts a seed fetch from the set |url|. Returns true if the fetch was
+  // started successfully, false otherwise. |is_http_retry| should be true if
+  // this is a retry over HTTP, false otherwise.
+  virtual bool DoFetchFromURL(const GURL& url, bool is_http_retry);
+
   // Stores the seed to prefs. Set as virtual and protected so that it can be
   // overridden by tests.
   virtual bool StoreSeed(const std::string& seed_data,
@@ -221,10 +230,24 @@ class VariationsService
     variations_server_url_ = url;
   }
 
+  // Sets the URL for querying the variations server when doing HTTP retries.
+  // Used for testing.
+  void set_insecure_variations_server_url(const GURL& url) {
+    insecure_variations_server_url_ = url;
+  }
+
+  // Sets the |last_request_was_http_retry_| flag. Used for testing.
+  void set_last_request_was_http_retry(bool was_http_retry) {
+    last_request_was_http_retry_ = was_http_retry;
+  }
+
   // The client that provides access to the embedder's environment.
   // Protected so testing subclasses can access it.
   VariationsServiceClient* client() { return client_.get(); }
 
+  // Exposes MaybeRetryOverHTTP for testing.
+  bool CallMaybeRetryOverHTTPForTesting();
+
   // Records a successful fetch:
   //   (1) Resets failure streaks for Safe Mode.
   //   (2) Records the time of this fetch as the most recent successful fetch.
@@ -250,6 +273,9 @@ class VariationsService
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, InsecurelyFetchedSetWhenHTTP);
   FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest,
                            InsecurelyFetchedNotSetWhenHTTPS);
+  FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, DoNotRetryAfterARetry);
+  FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest,
+                           DoNotRetryIfInsecureURLIsHTTPS);
 
   // Set of different possible values to report for the
   // Variations.LoadPermanentConsistencyCountryResult histogram. This enum must
@@ -270,11 +296,6 @@ class VariationsService
 
   void InitResourceRequestedAllowedNotifier();
 
-  // Attempts a seed fetch from the set |url|. Returns true if the fetch was
-  // started successfully, false otherwise. |is_http_retry| should be true if
-  // this is a retry over HTTP, false otherwise.
-  bool DoFetchFromURL(const GURL& url, bool is_http_retry);
-
   // Calls FetchVariationsSeed once and repeats this periodically. See
   // implementation for details on the period.
   void StartRepeatedVariationsSeedFetch();
@@ -290,6 +311,11 @@ class VariationsService
   // Called by SimpleURLLoader when |pending_seed_request_| load completes.
   void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
 
+  // Retry the fetch over HTTP, called by OnSimpleLoaderComplete when a request
+  // fails. Returns true is the fetch was successfully started, this does not
+  // imply the actual fetch was successful.
+  bool MaybeRetryOverHTTP();
+
   // ResourceRequestAllowedNotifier::Observer implementation:
   void OnResourceRequestsAllowed() override;
 
@@ -377,6 +403,9 @@ class VariationsService
   // Member responsible for creating trials from a variations seed.
   VariationsFieldTrialCreator field_trial_creator_;
 
+  // True if the last request was a retry over http.
+  bool last_request_was_http_retry_;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   base::WeakPtrFactory<VariationsService> weak_ptr_factory_;

components/variations/service/variations_service_unittest.cc

@@ -140,6 +140,12 @@ class TestVariationsService : public VariationsService {
   void set_intercepts_fetch(bool value) {
     intercepts_fetch_ = value;
   }
+  void set_insecure_url(const GURL& url) {
+    set_insecure_variations_server_url(url);
+  }
+  void set_last_request_was_retry(bool was_retry) {
+    set_last_request_was_http_retry(was_retry);
+  }
   bool fetch_attempted() const { return fetch_attempted_; }
   bool seed_stored() const { return seed_stored_; }
   const std::string& stored_country() const { return stored_country_; }
@@ -147,6 +153,8 @@ class TestVariationsService : public VariationsService {
   bool gzip_compressed_seed() const { return gzip_compressed_seed_; }
   bool insecurely_fetched_seed() const { return insecurely_fetched_seed_; }
 
+  bool CallMaybeRetryOverHTTP() { return CallMaybeRetryOverHTTPForTesting(); }
+
   void DoActualFetch() override {
     if (intercepts_fetch_) {
       fetch_attempted_ = true;
@@ -157,6 +165,15 @@ class TestVariationsService : public VariationsService {
     base::RunLoop().RunUntilIdle();
   }
 
+  bool DoFetchFromURL(const GURL& url, bool is_http_retry) override {
+    if (intercepts_fetch_) {
+      fetch_attempted_ = true;
+      return true;
+    }
+
+    return VariationsService::DoFetchFromURL(url, is_http_retry);
+  }
+
   bool StoreSeed(const std::string& seed_data,
                  const std::string& seed_signature,
                  const std::string& country_code,
@@ -912,6 +929,42 @@ TEST_F(VariationsServiceTest, InsecurelyFetchedNotSetWhenHTTPS) {
   EXPECT_FALSE(service.insecurely_fetched_seed());
 }
 
+TEST_F(VariationsServiceTest, RetryOverHTTPIfURLisSet) {
+  TestVariationsService service(
+      std::make_unique<web_resource::TestRequestAllowedNotifier>(
+          &prefs_, network_tracker_),
+      &prefs_, GetMetricsStateManager(), true);
+  service.set_intercepts_fetch(true);
+  service.set_last_request_was_retry(false);
+  service.set_insecure_url(GURL("http://example.test"));
+  EXPECT_TRUE(service.CallMaybeRetryOverHTTP());
+  EXPECT_TRUE(service.fetch_attempted());
+}
+
+TEST_F(VariationsServiceTest, DoNotRetryAfterARetry) {
+  TestVariationsService service(
+      std::make_unique<web_resource::TestRequestAllowedNotifier>(
+          &prefs_, network_tracker_),
+      &prefs_, GetMetricsStateManager(), true);
+  service.set_intercepts_fetch(true);
+  service.set_last_request_was_retry(true);
+  service.set_insecure_url(GURL("http://example.test"));
+  EXPECT_FALSE(service.CallMaybeRetryOverHTTP());
+  EXPECT_FALSE(service.fetch_attempted());
+}
+
+TEST_F(VariationsServiceTest, DoNotRetryIfInsecureURLIsHTTPS) {
+  TestVariationsService service(
+      std::make_unique<web_resource::TestRequestAllowedNotifier>(
+          &prefs_, network_tracker_),
+      &prefs_, GetMetricsStateManager(), true);
+  service.set_intercepts_fetch(true);
+  service.set_last_request_was_retry(false);
+  service.set_insecure_url(GURL("https://example.test"));
+  EXPECT_FALSE(service.CallMaybeRetryOverHTTP());
+  EXPECT_FALSE(service.fetch_attempted());
+}
+
 // TODO(isherman): Add an integration test for saving and loading a safe seed,
 // once the loading functionality is implemented on the seed store.