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/1334421Reviewed-by: Carlos IL <carlosil@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607882}

components/variations/service/variations_service.cc

@@ -291,7 +291,6 @@ 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_);
@@ -471,6 +470,49 @@ 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());
@@ -485,8 +527,6 @@ 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 {
@@ -557,49 +597,6 @@ 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_);
 
@@ -666,13 +663,10 @@ void VariationsService::OnSimpleLoaderComplete(
     response_code = headers->response_code();
   }
   bool is_success = headers && (net_error == net::OK);
-  // 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 =
+  bool was_https =
       pending_seed_request_->GetFinalURL().SchemeIs(url::kHttpsScheme);
   pending_seed_request_.reset();
-  if (last_request_was_http_retry_) {
+  if (was_https) {
     base::UmaHistogramSparse("Variations.SeedFetchResponseOrErrorCode",
                              is_success ? response_code : net_error);
   } else {
@@ -682,14 +676,16 @@ void VariationsService::OnSimpleLoaderComplete(
   if (!is_success) {
     DVLOG(1) << "Variations server request failed with error: " << net_error
              << ": " << net::ErrorToString(net_error);
-
-    if (MaybeRetryOverHTTP()) {
-      // If the retry was successfully started, return immediately,
-      // OnSimpleLoaderComplete will be called again when the new fetch
-      // finishes.
-      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;
     }
-
     // It's common for the very first fetch attempt to fail (e.g. the network
     // may not yet be available). In such a case, try again soon, rather than
     // waiting the full time interval.
@@ -748,7 +744,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, !final_url_was_https);
+                is_delta_compressed, is_gzip_compressed, !was_https);
   if (!store_success && is_delta_compressed) {
     disable_deltas_for_next_request_ = true;
     // |request_scheduler_| will be null during unit tests.
@@ -757,18 +753,6 @@ 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_);
 
@@ -820,10 +804,6 @@ 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

@@ -185,11 +185,6 @@ 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,
@@ -222,24 +217,10 @@ 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.
@@ -265,9 +246,6 @@ 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
@@ -288,6 +266,11 @@ 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();
@@ -303,11 +286,6 @@ 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;
 
@@ -395,9 +373,6 @@ 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,12 +140,6 @@ 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_; }
@@ -153,8 +147,6 @@ 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;
@@ -165,15 +157,6 @@ 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,
@@ -922,42 +905,6 @@ 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.