In PopularSites, parse (not sanitize) JSON safely.

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/files/important_file_writer.h"
 #include "base/json/json_reader.h"
+#include "base/json/json_writer.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/safe_json/safe_json_parser.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 "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 
 using net::URLFetcher;
 using variations::VariationsService;
 
@@ skipping 81 matching lines @@
 
   if (version.empty())
     version = variation_param_version;
 
   if (version.empty())
     version = kPopularSitesDefaultVersion;
 
   return version;
 }
 
-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>);
-  for (size_t i = 0; i < list->GetSize(); i++) {
-    base::DictionaryValue* item;
-    if (!list->GetDictionary(i, &item))
-      continue;
-    base::string16 title;
-    std::string url;
-    if (!item->GetString("title", &title) || !item->GetString("url", &url))
-      continue;
-    std::string favicon_url;
-    item->GetString("favicon_url", &favicon_url);
-    std::string thumbnail_url;
-    item->GetString("thumbnail_url", &thumbnail_url);
-    std::string large_icon_url;
-    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;
+// Must run on the blocking thread pool.
+bool WriteJsonToFile(const base::FilePath& local_path,
+                     const base::Value* json) {
+  std::string json_string;
+  return base::JSONWriter::Write(*json, &json_string) &&
+         base::ImportantFileWriter::WriteFileAtomically(local_path,
+                                                        json_string);
 }
 
 }  // namespace
 
 base::FilePath ChromePopularSites::GetDirectory() {
   base::FilePath dir;
   PathService::Get(chrome::DIR_USER_DATA, &dir);
   return dir;  // empty if PathService::Get() failed.
 }
 
@@ skipping 138 matching lines @@
 GURL PopularSites::GetPopularSitesURL() const {
   return GURL(base::StringPrintf(kPopularSitesURLFormat,
                                  pending_country_.c_str(),
                                  pending_version_.c_str()));
 }
 
 void PopularSites::OnReadFileDone(const GURL& url,
                                   std::unique_ptr<std::string> data,
                                   bool success) {
   if (success) {
-    ParseSiteList(*data);
+    // Pick the correct overload of JSONReader::Read().
+    using JsonCall = std::unique_ptr<base::Value>(base::StringPiece, int);
+    base::PostTaskAndReplyWithResult(

[Marc Treib, 2016/06/02 13:00:39]

The goal here is to keep the parsing off the UI thread for performance reasons?
I don't think that's necessary; lots of other places use JSONReader on the UI
thread, and this is not a huge file.
Then again, just because other people are doing it doesn't mean it's right,
so... I don't know.

[sfiera, 2016/06/02 13:12:11]

ParseJson() was called on the blocking pool before this CL, so I'm preserving
that behavior. If you prefer the simpler code, say the word and I'll change it.

(or, ask me to refactor it into a helper--this is basically the
UnsafeJsonParser::Parse() that iOS needs)

[Marc Treib, 2016/06/02 13:25:20]

That seems to be a historical artifact - before
https://codereview.chromium.org/1957313003, the function read the file *and*
parsed the json, so it needed to be off-thread. Now that the file handling is
elsewhere, IMO we don't need the extra thread-hopping anymore.

[sfiera, 2016/06/02 13:31:05]

Ah, OK, UI thread it is. Starting a dry run now.

+        blocking_runner_.get(), FROM_HERE,
+        base::Bind<JsonCall*>(&base::JSONReader::Read, *data,
+                              base::JSON_ALLOW_TRAILING_COMMAS),
+        base::Bind(&PopularSites::OnUnsafeParseDone,
+                   weak_ptr_factory_.GetWeakPtr()));
   } else {
     // File didn't exist, or couldn't be read for some other reason.
     FetchPopularSites(url);
   }
 }
 
+void PopularSites::OnUnsafeParseDone(std::unique_ptr<base::Value> json) {
+  if (json) {
+    OnJsonParsed(false, std::move(json));

[Marc Treib, 2016/06/02 13:00:39]

Directly call ParseSiteList here, and remove the write_to_file param from
OnJsonParsed?

[sfiera, 2016/06/02 13:12:11]

Done.

+  } else {
+    OnJsonParseFailed("previously-fetched JSON was no longer parseable");
+  }
+}
+
 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::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK_EQ(fetcher_.get(), source);
   std::unique_ptr<net::URLFetcher> free_fetcher = std::move(fetcher_);
 
-  std::string sketchy_json;
+  std::string json_string;
   if (!(source->GetStatus().is_success() &&
         source->GetResponseCode() == net::HTTP_OK &&
-        source->GetResponseAsString(&sketchy_json))) {
+        source->GetResponseAsString(&json_string))) {
     OnDownloadFailed();
     return;
   }
 
-  safe_json::JsonSanitizer::Sanitize(
-      sketchy_json, base::Bind(&PopularSites::OnJsonSanitized,
-                               weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&PopularSites::OnJsonSanitizationFailed,
+  safe_json::SafeJsonParser::Parse(
+      json_string,
+      base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr(),
+                 true /* write_to_file */),
+      base::Bind(&PopularSites::OnJsonParseFailed,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PopularSites::OnJsonSanitized(const std::string& valid_minified_json) {
-  base::PostTaskAndReplyWithResult(
-      blocking_runner_.get(), FROM_HERE,
-      base::Bind(&base::ImportantFileWriter::WriteFileAtomically, local_path_,
-                 valid_minified_json),
-      base::Bind(&PopularSites::OnFileWriteDone, weak_ptr_factory_.GetWeakPtr(),
-                 valid_minified_json));
+void PopularSites::OnJsonParsed(bool write_to_file,
+                                std::unique_ptr<base::Value> json) {
+  if (write_to_file) {
+    const base::Value* json_ptr = json.get();
+    base::PostTaskAndReplyWithResult(
+        blocking_runner_.get(), FROM_HERE,
+        base::Bind(&WriteJsonToFile, local_path_, json_ptr),
+        base::Bind(&PopularSites::OnFileWriteDone,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::Passed(std::move(json))));
+  } else {
+    ParseSiteList(std::move(json));
+  }
 }
 
-void PopularSites::OnJsonSanitizationFailed(const std::string& error_message) {
-  DLOG(WARNING) << "JSON sanitization failed: " << error_message;
+void PopularSites::OnJsonParseFailed(const std::string& error_message) {
+  DLOG(WARNING) << "JSON parsing failed: " << error_message;
   OnDownloadFailed();
 }
 
-void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
+void PopularSites::OnFileWriteDone(std::unique_ptr<base::Value> json,
+                                   bool success) {
   if (success) {
     prefs_->SetInt64(kPopularSitesLastDownloadPref,
                      base::Time::Now().ToInternalValue());
     prefs_->SetString(kPopularSitesCountryPref, pending_country_);
     prefs_->SetString(kPopularSitesVersionPref, pending_version_);
-    ParseSiteList(json);
+    ParseSiteList(std::move(json));
   } else {
     DLOG(WARNING) << "Could not write file to "
                   << local_path_.LossyDisplayName();
     OnDownloadFailed();
   }
 }
 
-void PopularSites::ParseSiteList(const std::string& json) {
-  base::PostTaskAndReplyWithResult(
-      blocking_runner_.get(), FROM_HERE, base::Bind(&ParseJson, json),
-      base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
-}
+void PopularSites::ParseSiteList(std::unique_ptr<base::Value> json) {
+  base::ListValue* list = nullptr;
+  if (!json || !json->GetAsList(&list)) {
+    DLOG(WARNING) << "JSON is not a list";
+    sites_.clear();
+    callback_.Run(false);
+    return;
+  }
 
-void PopularSites::OnJsonParsed(std::unique_ptr<std::vector<Site>> sites) {
-  if (sites)
-    sites_.swap(*sites);
-  else
-    sites_.clear();
-  callback_.Run(!!sites);
+  std::vector<PopularSites::Site> sites;
+  for (size_t i = 0; i < list->GetSize(); i++) {
+    base::DictionaryValue* item;
+    if (!list->GetDictionary(i, &item))
+      continue;
+    base::string16 title;
+    std::string url;
+    if (!item->GetString("title", &title) || !item->GetString("url", &url))
+      continue;
+    std::string favicon_url;
+    item->GetString("favicon_url", &favicon_url);
+    std::string thumbnail_url;
+    item->GetString("thumbnail_url", &thumbnail_url);
+    std::string large_icon_url;
+    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)));
+  }
+
+  sites_.swap(sites);
+  callback_.Run(true);
 }
 
 void PopularSites::OnDownloadFailed() {
   if (!is_fallback_) {
     DLOG(WARNING) << "Download country site list failed";
     is_fallback_ = true;
     pending_country_ = kPopularSitesDefaultCountryCode;
     pending_version_ = kPopularSitesDefaultVersion;
     FetchPopularSites(GetPopularSitesURL());
   } else {
     DLOG(WARNING) << "Download fallback site list failed";
     callback_.Run(false);
   }
 }