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);
+std::unique_ptr<std::vector<PopularSites::Site>> UnmarshalJson(

[Marc Treib, 2016/06/02 08:32:11]

IMO "unmarshal" isn't the right term here - we're not converting from a byte
stream. Also, a base::Value isn't actually json (even if it's closely related).
But, see below: Probably this helper isn't even required, so maybe the naming
problem will just go away :)

[sfiera, 2016/06/02 11:07:59]

Yep, inlined.

+    std::unique_ptr<base::Value> value) {
   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;
@@ skipping 167 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);
+    safe_json::SafeJsonParser::Parse(
+        *data,
+        base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr(),
+                   false /* write_to_file */),
+        base::Bind(&PopularSites::OnJsonParseFailed,
+                   weak_ptr_factory_.GetWeakPtr()));
   } else {
     // File didn't exist, or couldn't be read for some other reason.
     FetchPopularSites(url);
   }
 }
 
 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) {
+  std::string json_string;

[Marc Treib, 2016/06/02 08:32:11]

Shouldn't this be inside the "if (write_to_file)"?

[sfiera, 2016/06/02 11:07:59]

Done. (actually, moved into the blocking thread pool)

+  if (!base::JSONWriter::Write(*json, &json_string)) {
+    // DO_NOT_SUBMIT: fail

[Marc Treib, 2016/06/02 08:32:11]

:)

[sfiera, 2016/06/02 11:07:59]

It was indeed a DO_NOT_SUBMIT-fail.

+  }
+  if (write_to_file) {
+    base::PostTaskAndReplyWithResult(
+        blocking_runner_.get(), FROM_HERE,
+        base::Bind(&base::ImportantFileWriter::WriteFileAtomically, local_path_,
+                   json_string),
+        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) {
+void PopularSites::ParseSiteList(std::unique_ptr<base::Value> json) {
   base::PostTaskAndReplyWithResult(
-      blocking_runner_.get(), FROM_HERE, base::Bind(&ParseJson, json),
-      base::Bind(&PopularSites::OnJsonParsed, weak_ptr_factory_.GetWeakPtr()));
+      blocking_runner_.get(), FROM_HERE,
+      base::Bind(&UnmarshalJson, base::Passed(std::move(json))),

[Marc Treib, 2016/06/02 08:32:11]

Any reason why this happens asynchronously? I think the conversion could just
happen inline here..?

[sfiera, 2016/06/02 11:07:59]

It was to keep ParseJson off the UI thread, but we now ensure that elsewhere
(although we do *serialize* JSON on the UI thread now; I've fixed that).

+      base::Bind(&PopularSites::OnJsonUnmarshaled,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PopularSites::OnJsonParsed(std::unique_ptr<std::vector<Site>> sites) {
+void PopularSites::OnJsonUnmarshaled(std::unique_ptr<std::vector<Site>> sites) {
   if (sites)
     sites_.swap(*sites);
   else
     sites_.clear();
   callback_.Run(!!sites);

[sfiera, 2016/06/02 08:49:28]

Whoa, wait, shouldn't this be !!sites_ (note the underscore).

[sfiera, 2016/06/02 11:07:59]

Fixed to be less confusing.

 }
 
 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);
   }
 }