Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries

components/network_time/network_time_tracker.cc

 // Copyright 2014 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 "components/network_time/network_time_tracker.h"
 
 #include <stdint.h>
 #include <string>
 #include <utility>
 
@@ skipping 82 matching lines @@
 // be that badly wrong, but all the same it's included here to document the very
 // rough nature of the time service provided by this class.)
 const uint32_t kTimeServerMaxSkewSeconds = 10;
 
 const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current";
 
 const char kVariationsServiceCheckTimeIntervalSeconds[] =
     "CheckTimeIntervalSeconds";
 const char kVariationsServiceRandomQueryProbability[] =
     "RandomQueryProbability";
+// This parameter must have the value "true" in order for
+// StartTimeFetch() to start time queries on demand.
+const char kVariationsServiceEnableFetchesOnDemand[] = "EnableFetchesOnDemand";
 
 // This is an ECDSA prime256v1 named-curve key.
 const int kKeyVersion = 1;
 const uint8_t kKeyPubBytes[] = {
     0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02,
     0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03,
     0x42, 0x00, 0x04, 0xeb, 0xd8, 0xad, 0x0b, 0x8f, 0x75, 0xe8, 0x84, 0x36,
     0x23, 0x48, 0x14, 0x24, 0xd3, 0x93, 0x42, 0x25, 0x43, 0xc1, 0xde, 0x36,
     0x29, 0xc6, 0x95, 0xca, 0xeb, 0x28, 0x85, 0xff, 0x09, 0xdc, 0x08, 0xec,
     0x45, 0x74, 0x6e, 0x4b, 0xc3, 0xa5, 0xfd, 0x8a, 0x2f, 0x02, 0xa0, 0x4b,
@@ skipping 175 matching lines @@
 }
 
 bool NetworkTimeTracker::QueryTimeServiceForTesting() {
   CheckTime();
   return time_fetcher_ != nullptr;
 }
 
 void NetworkTimeTracker::WaitForFetchForTesting(uint32_t nonce) {
   query_signer_->OverrideNonceForTesting(kKeyVersion, nonce);
   base::RunLoop run_loop;
-  run_loop_for_testing_ = &run_loop;
+  fetch_completion_callbacks_.push_back(run_loop.QuitClosure());
   run_loop.Run();
-  run_loop_for_testing_ = nullptr;
 }
 
 base::TimeDelta NetworkTimeTracker::GetTimerDelayForTesting() const {
   DCHECK(timer_.IsRunning());
   return timer_.GetCurrentDelay();
 }
 
 NetworkTimeTracker::NetworkTimeResult NetworkTimeTracker::GetNetworkTime(
     base::Time* network_time,
     base::TimeDelta* uncertainty) const {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(network_time);
   if (network_time_at_last_measurement_.is_null()) {
     if (time_query_completed_) {
       // Time query attempts have been made in the past and failed.
       if (time_fetcher_) {
         // A fetch (not the first attempt) is in progress.
         return NETWORK_TIME_SUBSEQUENT_SYNC_PENDING;
-      } else {
-        return NETWORK_TIME_NO_SUCCESSFUL_SYNC;
       }
-    } else {
-      // No time queries have happened yet.
-      if (time_fetcher_) {
-        return NETWORK_TIME_FIRST_SYNC_PENDING;
-      } else {
-        return NETWORK_TIME_NO_SYNC_ATTEMPT;
-      }
+      return NETWORK_TIME_NO_SUCCESSFUL_SYNC;

[estark, 2016/10/25 20:14:00]

unrelated cleanup, sorry. there was too much indentation.

     }
+    // No time queries have happened yet.
+    if (time_fetcher_) {
+      return NETWORK_TIME_FIRST_SYNC_PENDING;
+    }
+    return NETWORK_TIME_NO_SYNC_ATTEMPT;
   }
+
   DCHECK(!ticks_at_last_measurement_.is_null());
   DCHECK(!time_at_last_measurement_.is_null());
   base::TimeDelta tick_delta =
       tick_clock_->NowTicks() - ticks_at_last_measurement_;
   base::TimeDelta time_delta = clock_->Now() - time_at_last_measurement_;
   if (time_delta.InMilliseconds() < 0) {  // Has wall clock run backward?
     DVLOG(1) << "Discarding network time due to wall clock running backward";
     UMA_HISTOGRAM_CUSTOM_TIMES(
         "NetworkTimeTracker.WallClockRanBackwards", time_delta.magnitude(),
         base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(7), 50);
@@ skipping 24 matching lines @@
     network_time_at_last_measurement_ = base::Time();
     return NETWORK_TIME_SYNC_LOST;
   }
   *network_time = network_time_at_last_measurement_ + tick_delta;
   if (uncertainty) {
     *uncertainty = network_time_uncertainty_ + divergence;
   }
   return NETWORK_TIME_AVAILABLE;
 }
 
+bool NetworkTimeTracker::StartTimeFetch(const base::Closure& closure) {

[meacer, 2016/10/26 19:19:22]

Could you add a DCHECK(thread_checker_.CalledOnValidThread()) here?

[estark, 2016/10/26 20:35:13]

Done.

+  // Check if the user is opted in to on-demand time fetches.
+  const std::string param = variations::GetVariationParamValueByFeature(
+      kNetworkTimeServiceQuerying, kVariationsServiceEnableFetchesOnDemand);
+  if (param != "true") {
+    return false;
+  }
+
+  // Enqueue the callback before calling CheckTime(), so that if
+  // CheckTime() completes synchronously, the callback gets called.
+  fetch_completion_callbacks_.push_back(closure);
+
+  // If a time query is already in progress, do not start another one.
+  if (time_fetcher_) {
+    return true;
+  }
+
+  // Cancel any fetches that are scheduled for the future, and try to
+  // start one now.

[meacer, 2016/10/26 00:38:15]

For my own education: If two clients call StartTimeFetch one of the other, this
will result in a single CheckTime operation, but with both callbacks being
called.

Is my understanding correct here? If yes, what happens when 4 clients call
StartTimeFetch with 1 second intervals?
We'll have 4 callbacks, and a single fetch happening after 4 seconds. Is it
going to be a problem if the owner of the first callback disappears? (E.g. the 3
second interstitial wait expires, the interstitial is shown, ssl error handler
is deleted which owns the callback)

Let me know if none of my words make sense :)

[meacer, 2016/10/26 01:07:37]

"If two clients call StartTimeFetch one of the other" should be "If two clients
call StartTimeFetch one after the other".

To answer my own question, I think this might be problematic since
SSLErrorHandler is passed as Unretained:
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Bind...

You'll probably want to cancel the callback when the error handler goes away, or
make it a weak ptr as described in Binding-A-Class-Method-With-Weak-Pointers.

CheckSuggestedURL doesn't have that problem since the objects that run the
callbacks (timer_ and common_name_mismatch_handler_) are owned by
SSLErrorHandler, so when it's destroyed callbacks don't run.

+  timer_.Stop();
+  CheckTime();
+
+  // CheckTime() does not necessarily start a fetch; for example, time
+  // queries might be disabled or network time might already be
+  // available.
+  if (!time_fetcher_) {
+    // If no query is in progress, there should be no callbacks waiting for it
+    // to complete.

[meacer, 2016/10/26 19:19:22]

nit: I'm a bit confused by this comment, it sounds like no callbacks are
expected instead of having to clear existing callbacks. Perhaps you could reword
it to say something like "If no query is in progress, no callbacks need to be
called"?

[estark, 2016/10/26 20:35:13]

Oh, yes, what I wrote was confusing. Done.

+    fetch_completion_callbacks_.clear();
+    return false;
+  }
+  return true;
+}
+
 void NetworkTimeTracker::CheckTime() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // If NetworkTimeTracker is waking up after a backoff, this will reset the
   // timer to its default faster frequency.
   QueueCheckTime(CheckTimeInterval());
 
   if (!ShouldIssueTimeQuery()) {
     return;
   }
@@ skipping 98 matching lines @@
   // long time.
   if (!UpdateTimeFromResponse()) {  // On error, back off.
     if (backoff_ < base::TimeDelta::FromDays(2)) {
       backoff_ *= 2;
     }
   } else {
     backoff_ = base::TimeDelta::FromMinutes(kBackoffMinutes);
   }
   QueueCheckTime(backoff_);
   time_fetcher_.reset();
-  if (run_loop_for_testing_ != nullptr)
-    run_loop_for_testing_->QuitWhenIdle();
+
+  // Clear |fetch_completion_callbacks_| before running any of them,
+  // because a callback could call StartTimeFetch() to enqueue another
+  // callback.
+  std::vector<base::Closure> callbacks = fetch_completion_callbacks_;
+  fetch_completion_callbacks_.clear();
+  for (const auto& callback : callbacks) {
+    callback.Run();
+  }
 }
 
 void NetworkTimeTracker::QueueCheckTime(base::TimeDelta delay) {
   timer_.Start(FROM_HERE, delay, this, &NetworkTimeTracker::CheckTime);
 }
 
 bool NetworkTimeTracker::ShouldIssueTimeQuery() {
   // Do not query the time service if not enabled via Variations Service.
   if (!base::FeatureList::IsEnabled(kNetworkTimeServiceQuerying)) {
     return false;
   }
 
   // If GetNetworkTime() does not return NETWORK_TIME_AVAILABLE,
   // synchronization has been lost and a query is needed.
   base::Time network_time;
   if (GetNetworkTime(&network_time, nullptr) != NETWORK_TIME_AVAILABLE) {
     return true;
   }
 
   // Otherwise, make the decision at random.
   return base::RandDouble() < RandomQueryProbability();
 }
 
 }  // namespace network_time