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

components/network_time/network_time_tracker.cc

Index: components/network_time/network_time_tracker.cc
diff --git a/components/network_time/network_time_tracker.cc b/components/network_time/network_time_tracker.cc
index d33de06568655a2f6a136e9a4ba7125385a8f8db..629016576a9fda82e8ea54ef43b7cc467c04740c 100644
--- a/components/network_time/network_time_tracker.cc
+++ b/components/network_time/network_time_tracker.cc
@@ -100,6 +100,9 @@ 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;
@@ -295,9 +298,8 @@ bool NetworkTimeTracker::QueryTimeServiceForTesting() {
 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 {
@@ -316,18 +318,16 @@ NetworkTimeTracker::NetworkTimeResult NetworkTimeTracker::GetNetworkTime(
       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 =
@@ -372,6 +372,40 @@ NetworkTimeTracker::NetworkTimeResult NetworkTimeTracker::GetNetworkTime(
   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());
 
@@ -490,8 +524,15 @@ void NetworkTimeTracker::OnURLFetchComplete(const net::URLFetcher* source) {
   }
   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) {