In PopularSites, parse (not sanitize) JSON safely.

chrome/browser/android/ntp/popular_sites.cc

Index: chrome/browser/android/ntp/popular_sites.cc
diff --git a/chrome/browser/android/ntp/popular_sites.cc b/chrome/browser/android/ntp/popular_sites.cc
index 98233ca12bd5e8f8660faf635a5219214f49f147..d1e6b9eb8964df3d90bde4b4b239152069b20c35 100644
--- a/chrome/browser/android/ntp/popular_sites.cc
+++ b/chrome/browser/android/ntp/popular_sites.cc
@@ -11,7 +11,7 @@
 #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"
@@ -24,7 +24,7 @@
 #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"
@@ -126,10 +126,8 @@ std::string GetVersionToUse(const PrefService* prefs,
   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";
@@ -317,7 +315,12 @@ 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);
@@ -337,42 +340,54 @@ 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();
@@ -380,13 +395,15 @@ void PopularSites::OnFileWriteDone(const std::string& json, bool success) {
   }
 }
 
-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