Remove PopularSites' dependencies on //chrome/....

chrome/browser/android/ntp/popular_sites.cc

 // Copyright 2015 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 "chrome/browser/android/ntp/popular_sites.h"
 
 #include <stddef.h>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
-#include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
-#include "chrome/common/chrome_paths.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/ntp_tiles/pref_names.h"
 #include "components/ntp_tiles/switches.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
+#include "components/safe_json/json_sanitizer.h"
 #include "components/search_engines/search_engine_type.h"
 #include "components/search_engines/template_url_prepopulate_data.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/variations/service/variations_service.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_status_code.h"
 
 using content::BrowserThread;
+using net::URLFetcher;
 using variations::VariationsService;
 
 namespace {
 
 const char kPopularSitesURLFormat[] =
     "https://www.gstatic.com/chrome/ntp/suggested_sites_%s_%s.json";
 const char kPopularSitesDefaultCountryCode[] = "DEFAULT";
 const char kPopularSitesDefaultVersion[] = "5";
 const char kPopularSitesLocalFilename[] = "suggested_sites.json";
 const int kPopularSitesRedownloadIntervalHours = 24;
@@ skipping 73 matching lines @@
 
   if (version.empty())
     version = variation_param_version;
 
   if (version.empty())
     version = kPopularSitesDefaultVersion;
 
   return version;
 }
 
-base::FilePath GetPopularSitesPath() {
-  base::FilePath dir;
-  PathService::Get(chrome::DIR_USER_DATA, &dir);
-  return dir.AppendASCII(kPopularSitesLocalFilename);
-}
-
-std::unique_ptr<std::vector<PopularSites::Site>> ReadAndParseJsonFile(
-    const base::FilePath& path) {
-  std::string json;
-  if (!base::ReadFileToString(path, &json)) {
-    DLOG(WARNING) << "Failed reading file";
-    return nullptr;
-  }
-
+std::unique_ptr<std::vector<PopularSites::Site>> ParseJson(
+    const std::string& json) {
   std::unique_ptr<base::Value> value =
       base::JSONReader::Read(json, base::JSON_ALLOW_TRAILING_COMMAS);
   base::ListValue* list;
   if (!value || !value->GetAsList(&list)) {
     DLOG(WARNING) << "Failed parsing json";
     return nullptr;
   }
 
   std::unique_ptr<std::vector<PopularSites::Site>> sites(
       new std::vector<PopularSites::Site>);
@@ skipping 13 matching lines @@
     item->GetString("large_icon_url", &large_icon_url);
 
     sites->push_back(PopularSites::Site(title, GURL(url), GURL(favicon_url),
                                         GURL(large_icon_url),
                                         GURL(thumbnail_url)));
   }
 
   return sites;
 }
 
+// Write |data| to a temporary file, then move it to |path|, thereby ensuring
+// that a partial file cannot be written.
+//
+// TODO(sfiera): is there some helper for this somewhere?
+bool WriteStringToFileAtomic(const std::string& data,
+                             const base::FilePath& path) {
+  base::FilePath temporary_path;
+  return base::CreateTemporaryFile(&temporary_path) &&
+         base::WriteFile(temporary_path, data.data(), data.size()) &&
+         base::Move(temporary_path, path);
+}
+
+// Run |call| on the thread pool for blocking file operations, then call |done|
+// with its result on the original thread.
+template <typename T>
+void RunBlockingTaskAndReply(const tracked_objects::Location& from_here,
+                             const base::Callback<T()>& call,
+                             const base::Callback<void(T)>& done) {
+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool()
+          ->GetTaskRunnerWithShutdownBehavior(
+              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
+          .get(),
+      from_here, call, done);
+}
+
 }  // namespace
 
 PopularSites::Site::Site(const base::string16& title,
                          const GURL& url,
                          const GURL& favicon_url,
                          const GURL& large_icon_url,
                          const GURL& thumbnail_url)
     : title(title),
       url(url),
       favicon_url(favicon_url),
       large_icon_url(large_icon_url),
       thumbnail_url(thumbnail_url) {}
 
 PopularSites::Site::Site(const Site& other) = default;
 
 PopularSites::Site::~Site() {}
 
 PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            VariationsService* variations_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const std::string& variation_param_country,
                            const std::string& variation_param_version,
                            bool force_download,
                            const FinishedCallback& callback)
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    GetCountryToUse(prefs,
                                    template_url_service,
                                    variations_service,
                                    variation_param_country),
                    GetVersionToUse(prefs, variation_param_version),
                    GURL(),
                    force_download,
                    callback) {}
 
 PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const GURL& url,
                            const FinishedCallback& callback)
     : PopularSites(prefs,
                    template_url_service,
                    download_context,
+                   directory,
                    std::string(),
                    std::string(),
                    url,
                    true,
                    callback) {}
 
 PopularSites::~PopularSites() {}
 
 std::string PopularSites::GetCountry() const {
   return prefs_->GetString(kPopularSitesCountryPref);
@@ skipping 12 matching lines @@
                                  std::string());
 
   user_prefs->RegisterInt64Pref(kPopularSitesLastDownloadPref, 0);
   user_prefs->RegisterStringPref(kPopularSitesCountryPref, std::string());
   user_prefs->RegisterStringPref(kPopularSitesVersionPref, std::string());
 }
 
 PopularSites::PopularSites(PrefService* prefs,
                            const TemplateURLService* template_url_service,
                            net::URLRequestContextGetter* download_context,
+                           const base::FilePath& directory,
                            const std::string& country,
                            const std::string& version,
                            const GURL& override_url,
                            bool force_download,
                            const FinishedCallback& callback)
     : callback_(callback),
+      is_fallback_(false),
       pending_country_(country),
       pending_version_(version),
-      local_path_(GetPopularSitesPath()),
+      local_path_(directory.empty()
+                      ? base::FilePath()
+                      : directory.AppendASCII(kPopularSitesLocalFilename)),
       prefs_(prefs),
       template_url_service_(template_url_service),
       download_context_(download_context),
       weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   const base::Time last_download_time = base::Time::FromInternalValue(
       prefs_->GetInt64(kPopularSitesLastDownloadPref));
   const base::TimeDelta time_since_last_download =
       base::Time::Now() - last_download_time;
   const base::TimeDelta redownload_interval =
       base::TimeDelta::FromHours(kPopularSitesRedownloadIntervalHours);
   const bool download_time_is_future = base::Time::Now() < last_download_time;
   const bool country_changed = GetCountry() != pending_country_;
   const bool version_changed = GetVersion() != pending_version_;
 
-  const bool should_redownload_if_exists =
-      force_download || download_time_is_future ||
+  const GURL url =
+      override_url.is_valid() ? override_url : GetPopularSitesURL();
+
+  // No valid path to save to. Immediately post failure.
+  if (local_path_.empty()) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(callback_, false));
+    return;
+  }
+
+  // Download forced, or we need to download a new file.
+  if (force_download || download_time_is_future ||
       (time_since_last_download > redownload_interval) || country_changed ||
-      version_changed;
+      version_changed) {
+    FetchPopularSites(url);
+    return;
+  }
 
-  FetchPopularSites(
-      override_url.is_valid() ? override_url : GetPopularSitesURL(),
-      should_redownload_if_exists, false /* is_fallback */);
+  std::unique_ptr<std::string> file_data(new std::string);
+  std::string* file_data_ptr = file_data.get();
+  RunBlockingTaskAndReply(
+      FROM_HERE,
+      base::Bind(&base::ReadFileToString, local_path_, file_data_ptr),
+      base::Bind(&PopularSites::OnReadFileDone, weak_ptr_factory_.GetWeakPtr(),
+                 url, base::Passed(std::move(file_data))));

[Marc Treib, 2016/05/11 10:20:29]

There's also base::Owned which takes ownership of a raw pointer, and is arguably
a bit more convenient. Not sure which way I'd prefer, it's kinda icky either
way.

[sfiera, 2016/05/11 14:23:53]

I like the current way because a reader of OnReadFileDone() doesn't have to
wonder about the lifetime of the pointer.

[Marc Treib, 2016/05/11 14:47:25]

Acknowledged.

 }
 
 GURL PopularSites::GetPopularSitesURL() const {
   return GURL(base::StringPrintf(kPopularSitesURLFormat,
                                  pending_country_.c_str(),
                                  pending_version_.c_str()));
 }
 
-void PopularSites::FetchPopularSites(const GURL& url,
-                                     bool force_download,
-                                     bool is_fallback) {
-  downloader_.reset(new FileDownloader(
-      url, local_path_, force_download, download_context_,
-      base::Bind(&PopularSites::OnDownloadDone, base::Unretained(this),
-                 is_fallback)));
+void PopularSites::FetchPopularSites(const GURL& url) {
+  fetcher_ = URLFetcher::Create(url, URLFetcher::GET, this);
+  fetcher_->SetRequestContext(download_context_);
+  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                         net::LOAD_DO_NOT_SAVE_COOKIES);
+  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
+  fetcher_->Start();
 }
 
-void PopularSites::OnDownloadDone(bool is_fallback,
-                                  FileDownloader::Result result) {
-  downloader_.reset();
-  switch (result) {
-    case FileDownloader::DOWNLOADED:
-      prefs_->SetInt64(kPopularSitesLastDownloadPref,
-                      base::Time::Now().ToInternalValue());
-      prefs_->SetString(kPopularSitesCountryPref, pending_country_);
-      prefs_->SetString(kPopularSitesVersionPref, pending_version_);
-      ParseSiteList(local_path_);
-      break;
-    case FileDownloader::EXISTS:
-      ParseSiteList(local_path_);
-      break;
-    case FileDownloader::FAILED:
-      if (!is_fallback) {
-        DLOG(WARNING) << "Download country site list failed";
-        pending_country_ = kPopularSitesDefaultCountryCode;
-        pending_version_ = kPopularSitesDefaultVersion;
-        // It is fine to force the download as Fallback is only triggered after
-        // a failed download.
-        FetchPopularSites(GetPopularSitesURL(), true /* force_download */,
-                          true /* is_fallback */);
-      } else {
-        DLOG(WARNING) << "Download fallback site list failed";
-        callback_.Run(false);
-      }
-      break;
+void PopularSites::OnDownloadFailed() {
+  if (!is_fallback_) {
+    DLOG(WARNING) << "Download country site list failed";
+    is_fallback_ = true;
+    pending_country_ = kPopularSitesDefaultCountryCode;
+    pending_version_ = kPopularSitesDefaultVersion;
+    // It is fine to force the download as Fallback is only triggered after
+    // a failed download.

[Marc Treib, 2016/05/11 10:20:29]

This comment is kinda obsolete now.

[sfiera, 2016/05/11 14:23:53]

Done.

+    FetchPopularSites(GetPopularSitesURL());
+  } else {
+    DLOG(WARNING) << "Download fallback site list failed";
+    callback_.Run(false);
   }
 }
 
-void PopularSites::ParseSiteList(const base::FilePath& path) {
-  base::PostTaskAndReplyWithResult(
-      BrowserThread::GetBlockingPool()
-          ->GetTaskRunnerWithShutdownBehavior(
-              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN)
-          .get(),
-      FROM_HERE, base::Bind(&ReadAndParseJsonFile, path),
+void PopularSites::OnReadFileDone(const GURL& url,
+                                  std::unique_ptr<std::string> data,
+                                  bool success) {
+  if (success) {
+    ParseSiteList(*data);
+  } else {
+    FetchPopularSites(url);

[Marc Treib, 2016/05/11 10:20:29]

Add a comment? I guess this usually means the file didn't exist?

[sfiera, 2016/05/11 14:23:53]

Done.

+  }
+}
+
+void PopularSites::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK_EQ(fetcher_.get(), source);
+  fetcher_.reset();

[Marc Treib, 2016/05/11 10:20:29]

I think this may be why stuff fails: This deletes source, which you use later.
One pattern to solve this is to have a local unique_ptr<URLFetcher> and move
fetcher_ in there.

[sfiera, 2016/05/11 11:06:02]

Oh yeah, obviously. Now I wonder why it doesn't fail locally...

I know there are ways to build like is_asan to check things like this. Are there
others and do they generally work on Android?

[Marc Treib, 2016/05/11 11:35:28]

I think there's a few more, maybe "is_msan" or something. It's been a while
since I used any of those locally. Not sure if they work on Android.
I'd recommend just running the corresponding trybots when you're worried about
something. Doing this locally was rather painful.

[sfiera, 2016/05/11 14:23:53]

Acknowledged.

+
+  std::string sketchy_json;
+  if (!(source->GetStatus().is_success() &&
+        source->GetResponseCode() == net::HTTP_OK &&
+        source->GetResponseAsString(&sketchy_json))) {
+    OnDownloadFailed();
+    return;
+  }
+
+  safe_json::JsonSanitizer::Sanitize(
+      sketchy_json, base::Bind(&PopularSites::OnJsonSanitized,
+                               weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&PopularSites::OnJsonSanitizationFailed,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void PopularSites::OnJsonSanitized(const std::string& valid_minified_json) {
+  RunBlockingTaskAndReply(
+      FROM_HERE,
+      base::Bind(&WriteStringToFileAtomic, valid_minified_json, local_path_),
+      base::Bind(&PopularSites::OnFileWriteDone, weak_ptr_factory_.GetWeakPtr(),
+                 valid_minified_json));
+}
+
+void PopularSites::OnJsonSanitizationFailed(const std::string& error_message) {
+  DLOG(WARNING) << "JSON sanitization failed: " << error_message;
+  OnDownloadFailed();
+}
+
+void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
+  if (success) {
+    prefs_->SetInt64(kPopularSitesLastDownloadPref,
+                     base::Time::Now().ToInternalValue());
+    prefs_->SetString(kPopularSitesCountryPref, pending_country_);
+    prefs_->SetString(kPopularSitesVersionPref, pending_version_);
+    ParseSiteList(json);
+  } else {
+    DLOG(WARNING) << "Could not write file to "
+                  << local_path_.LossyDisplayName();
+    OnDownloadFailed();
+  }
+}
+
+void PopularSites::ParseSiteList(const std::string& json) {
+  RunBlockingTaskAndReply(
+      FROM_HERE, base::Bind(&ParseJson, json),
       base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
 }
 
 void PopularSites::OnJsonParsed(std::unique_ptr<std::vector<Site>> sites) {
   if (sites)
     sites_.swap(*sites);
   else
     sites_.clear();
   callback_.Run(!!sites);
 }