Quirks Client for downloading and providing display profiles

components/quirks/quirks_manager.cc

Index: components/quirks/quirks_manager.cc
diff --git a/components/quirks/quirks_manager.cc b/components/quirks/quirks_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b31405b5a30190fac51d0e9a3d01d327ede5f4b6
--- /dev/null
+++ b/components/quirks/quirks_manager.cc
@@ -0,0 +1,243 @@
+// Copyright 2016 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/quirks/quirks_manager.h"
+
+#include "base/command_line.h"
+#include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/path_service.h"
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/task_runner_util.h"
+#include "base/thread_task_runner_handle.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/scoped_user_pref_update.h"
+#include "components/quirks/pref_names.h"
+#include "components/quirks/switches.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
+#include "url/gurl.h"
+
+namespace quirks {
+
+namespace {
+
+QuirksManager* g_manager = nullptr;
+
+const char kIccExtension[] = ".icc";
+
+// How often we query Quirks Server.
+const int kDaysBetweenServerChecks = 30;
+
+// Check if file exists, VLOG results.
+bool CheckAndLogFile(const base::FilePath& path) {
+  const bool exists = base::PathExists(path);
+  VLOG(1) << (exists ? "File" : "No File") << " found at " << path.value();
+  // TODO(glevin): If file exists, do we want to implement a hash to verify that
+  // the file hasn't been corrupted or tampered with?
+  return exists;
+}
+
+base::FilePath CheckForIccFileAndNewDevice(
+    scoped_refptr<QuirksManager::Delegate> delegate,
+    int64_t product_id) {
+  // Record this on file thread for future use on task runner.
+  delegate->CheckDaysSinceOobe();

[stevenjb, 2016/02/29 20:22:29]

Calling into a delegate from the blocking pool is unusual and error prone. This
could be done before calling this.

[Greg Levin, 2016/03/02 00:00:26]

CheckDaysSinceOobe() calls StartupUtils::GetTimeSinceOobeFlagFileCreation(),
which has to run on a file thread.  And the delegate is currently the only way
to call this function since it lives in chrome/browser/etc.  Would deriving the
delegate from RefCountedThreadSafe be any better?

[Greg Levin, 2016/03/07 16:34:38]

UPDATE: After a small refactor, delegate::GetDaysSinceOobe() is called from ui
thread (with callback provided), then the delegate makes an internal call to
blocking pool for file access.  This is done later, only when it's clear that
device age will be needed.

+
+  // First, look for icc file in old read-only location.  If there, we don't use
+  // the Quirks server.
+  // TODO(glevin): Awaiting final decision on how to handle old read-only files.
+  base::FilePath path = delegate->GetBuiltInDisplayProfileDirectory().Append(

[stevenjb, 2016/02/29 20:22:29]

delegate->GetBuiltInDisplayProfileDirectory() could be passed to this.

[Greg Levin, 2016/03/02 00:00:26]

Yeah, I had briefly done it that way.  But I couldn't figure out any other way
of calling StartupUtils::GetTimeSinceOobeFlagFileCreation() except by passing
the delegate into the blocking pool, so I just left these next two as they are.

[Greg Levin, 2016/03/07 16:34:38]

UPDATE: Now both file paths (including file name) are passed into
CheckForIccFile().

+      IdToFileName(product_id));
+  if (CheckAndLogFile(path))
+    return path;
+
+  // If experimental Quirks flag isn't set, no other icc file is available.
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableDisplayQuirksClient)) {
+    VLOG(1) << "Quirks Client disabled, no built-in icc file available.";
+    return base::FilePath();
+  }
+
+  // Check if QuirksClient has already downloaded icc file from server.
+  path = delegate->GetDownloadDisplayProfileDirectory().Append(

[stevenjb, 2016/02/29 20:22:29]

delegate->GetDownloadDisplayProfileDirectory() could be passed to this.

[Greg Levin, 2016/03/02 00:00:26]

Acknowledged.

+      IdToFileName(product_id));
+  if (CheckAndLogFile(path))
+    return path;
+
+  return base::FilePath();
+}
+
+}  // namespace
+
+std::string IdToHexString(int64_t product_id) {
+  return base::StringPrintf("%08" PRIx64, product_id);
+}
+
+std::string IdToFileName(int64_t product_id) {
+  return IdToHexString(product_id).append(kIccExtension);
+}
+
+void RequestIccProfilePath(int64_t product_id,
+                           const RequestFinishedCallback& on_request_finished) {
+  if (g_manager)
+    g_manager->RequestIccFilePath(product_id, on_request_finished);
+  else
+    LOG(ERROR) << "Quirks Client Manager not initialized; can't get icc file.";
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// QuirksManager
+
+QuirksManager::QuirksManager(
+    scoped_refptr<Delegate> delegate,
+    scoped_refptr<base::SequencedWorkerPool> blocking_pool,
+    PrefService* local_state,
+    scoped_refptr<net::URLRequestContextGetter> url_context_getter)
+    : delegate_(delegate),
+      blocking_pool_(blocking_pool),
+      local_state_(local_state),
+      url_context_getter_(url_context_getter),
+      weak_ptr_factory_(this) {}
+
+QuirksManager::~QuirksManager() {
+  clients_.clear();
+  g_manager = nullptr;
+}
+
+// static
+void QuirksManager::Initialize(
+    scoped_refptr<Delegate> delegate,
+    scoped_refptr<base::SequencedWorkerPool> blocking_pool,
+    PrefService* local_state,
+    scoped_refptr<net::URLRequestContextGetter> url_context_getter) {
+  g_manager = new QuirksManager(delegate, blocking_pool, local_state,
+                                url_context_getter);
+}
+
+// static
+void QuirksManager::Shutdown() {
+  delete g_manager;
+}
+
+// static
+QuirksManager* QuirksManager::Get() {
+  DCHECK(g_manager);
+  return g_manager;
+}
+
+// static
+void QuirksManager::RegisterPrefs(PrefRegistrySimple* registry) {
+  registry->RegisterDictionaryPref(prefs::kQuirksClientLastServerCheck);
+}
+
+void QuirksManager::RequestIccFilePath(
+    int64_t product_id,
+    const RequestFinishedCallback& on_request_finished) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+
+  base::PostTaskAndReplyWithResult(
+      blocking_pool_.get(), FROM_HERE,
+      base::Bind(&CheckForIccFileAndNewDevice, delegate_, product_id),
+      base::Bind(&QuirksManager::OnIccFilePathRequestCompleted,
+                 weak_ptr_factory_.GetWeakPtr(), product_id,
+                 on_request_finished));
+}
+
+void QuirksManager::OnIccFilePathRequestCompleted(
+    int64_t product_id,
+    const RequestFinishedCallback& on_request_finished,
+    base::FilePath path) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+
+  // If we found a file or it's not time to check server, inform requester.
+  if (!path.empty() || !NeedToCheckServer(product_id)) {
+    on_request_finished.Run(path, false);
+    // TODO(glevin): Eventually we'll want to check the server for updates even
+    // if the file exists.
+    return;
+  }
+
+  // Create and start a client to download file.
+  QuirksClient* client =
+      new QuirksClient(product_id, on_request_finished, this);
+  clients_.insert(make_scoped_ptr(client));
+  client->StartDownload();
+}
+
+void QuirksManager::ClientFinished(QuirksClient* client) {
+  SetLastServerCheck(client->product_id(), base::Time::Now());
+  auto it = std::find_if(clients_.begin(), clients_.end(),
+                         [client](const scoped_ptr<QuirksClient>& c) {
+                           return c.get() == client;
+                         });
+  DCHECK(it != clients_.end());
+  if (it != clients_.end())

[stevenjb, 2016/02/29 20:22:29]

Use CHECK or DCHECK when something should be logically impossible, and then skip
any logical test. In this case, since accessing an invalid iterator could be
dangerous, use CHECK and remove the if().

[Greg Levin, 2016/03/02 00:00:26]

Done.

+    clients_.erase(it);
+}
+
+// TODO(glevin): Add code to record UMA stats here.  Also need to set
+// request_reason_ in QuirksClient.
+void QuirksManager::RecordReasonUmaStat(QuirksClient::RequestReason reason) {}
+
+void QuirksManager::RecordFileFoundUmaStat(bool success) {}
+
+scoped_ptr<net::URLFetcher> QuirksManager::CreateURLFetcher(
+    const std::string& url,
+    net::URLFetcherDelegate* delegate) {
+  if (!fake_quirks_fetcher_creator_.is_null())
+    return fake_quirks_fetcher_creator_.Run(GURL(url), delegate);
+
+  return net::URLFetcher::Create(GURL(url), net::URLFetcher::GET, delegate);
+}
+
+bool QuirksManager::NeedToCheckServer(int64_t product_id) {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableDisplayQuirksClient))
+    return false;
+
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());

[stevenjb, 2016/02/29 20:22:29]

nit: Put this first so that it is easier to find.

[Greg Levin, 2016/03/02 00:00:26]

Done.

[Greg Levin, 2016/03/07 16:34:38]

UPDATE (FYI): NeedToCheckServer() is now gone, code has been split between
OnIccFilePathRequestCompleted() and OnDaysSinceOobeGotten().

+  double last_check = 0.0;
+  local_state_->GetDictionary(prefs::kQuirksClientLastServerCheck)
+      ->GetDouble(IdToHexString(product_id), &last_check);
+
+  if (last_check != 0.0) {
+    const base::TimeDelta time_since =
+        base::Time::Now() - base::Time::FromDoubleT(last_check);
+
+    // Don't need server check if we've checked within last 30 days.
+    if (time_since < base::TimeDelta::FromDays(kDaysBetweenServerChecks)) {
+      VLOG(2) << time_since.InDays()
+              << " days since last Quirks Server check for display "
+              << IdToHexString(product_id);
+      return false;
+    }
+
+    return true;
+  }
+
+  // On new devices, we want to check server immediately.
+  if (delegate_->GetDaysSinceOobe() <= kDaysBetweenServerChecks)

[stevenjb, 2016/02/29 20:22:29]

This is running on the ui thread, not the blocking pool, so we would be calling
into the delegate from multiple threads, which could be a problem if the member
was being written to from the blocking pool when this is called. It is
cleaner/safer to make all calls to a delegate_ from a single thread (usually the
construction thread where the delegate is passed in unless otherwise necessary
and clearly documented).

[Greg Levin, 2016/03/02 00:00:26]

As noted above, CheckDaysSinceOobe() can only run from the blocking pool.

CheckDaysSinceOobe() is only called from within CheckForIccFileAndNewDevice(),
GetDaysSinceOobe() is only called from within OnIccFilePathRequestCompleted(),
and calling order is enforced by
PostTaskAndReplyWithResult(
  &CheckForIccFileAndNewDevice, &OnIccFilePathRequestCompleted).
I don't believe there is any practical thread safety issue here (tho I could be
mistaken), but of course am open to suggestions for improvement!

I could return a custom struct with { path, days_since_oobe }, but caching
days_since_oobe seemed easier, and it could potentially be used by multiple
clients, and only needs to be read once.

[stevenjb, 2016/03/02 17:51:08]

This is similar to the problem we had before. We have an interface (the
delegate) where we are making assumptions about which methods get called from
which thread with no documentation. i.e. some methods are expected to be called
from the main/UI thread and some from a blocking pool.

And there is a thread safety issue here. It seems unlikely, but there isn't any
protection against CheckDaysSinceOobe() and GetDaysSinceOobe() from being called
at the same time on different threads, and they both access days_since_oobe_.

There are a couple of ways we can solve this:
1) Document the expectations of the delegate methods and make the delegate
itself tread safe (including using ThreadSafeRefCounted). I dislike this because
it would be easy to miss a thread safety issue or race, especially down the
road.

2) Change and simplify the delegate interface so that it can be called
exclusively from the main/UI thread:
 Get*Directory - these are trivial/safe from the Ui thread.
 Check/GetDaysSinceOobe() - Eliminate Check and make Get asynchronous. It posts
the file io test (if necessary) to a blocking pool and invokes the callback on
the main/UI thread. The callback handles the remaining logic int his function.

I realize it may seem more complex to add blocking pool logic to the delegate,
but keeping the delegate interface simple and robust is more important than
reducing the complexity of the delegate implementation.

[Greg Levin, 2016/03/07 16:34:38]

Done, went with (2).

+    return true;
+
+  // Otherwise, for the first check on an older device, we want to stagger
+  // it over 30 days, so artificially set last check accordingly.
+  const int rand_days = base::RandInt(0, kDaysBetweenServerChecks);
+  const base::Time fake_last_check =
+      base::Time::Now() - base::TimeDelta::FromDays(rand_days);
+  SetLastServerCheck(product_id, fake_last_check);
+  VLOG(2) << "Delaying first Quirks Server check by "
+          << kDaysBetweenServerChecks - rand_days << " days.";
+  return false;
+}
+
+void QuirksManager::SetLastServerCheck(int64_t product_id,
+                                       const base::Time& last_check) {
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  DictionaryPrefUpdate dict(local_state_, prefs::kQuirksClientLastServerCheck);
+  dict->SetDouble(IdToHexString(product_id), last_check.ToDoubleT());
+}
+
+}  // namespace quirks