Make JSONParser a pure interface.

This allows stubbing it out in tests, and using platform-specific implementations in the future. As a side effect, the fact that the default implementation is refcounted is not exposed to clients anymore. Instead, the (newly added) factory method returns a raw pointer, and the object will delete itself after either of the callbacks has been called. In practice, this should not make a difference, as no client currently requires the SafeJsonParser to live longer than that, and in one case it actually simplifies things for the client by breaking a reference cycle. TBR=benwells@chromium.org,brettw@chromium.org BUG=501333 Review URL: https://codereview.chromium.org/1214903010 Cr-Commit-Position: refs/heads/master@{#337459}
marcosholgado · Jul 6, 2015 · 64c5a38 · 64c5a38
1 parent f4401a5
commit 64c5a38
Show file tree

Hide file tree

Showing 20 changed files with 373 additions and 212 deletions.
diff --git a/chrome/browser/extensions/api/management/chrome_management_api_delegate.cc b/chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
@@ -184,18 +184,16 @@ void ChromeManagementAPIDelegate::
     GetPermissionWarningsByManifestFunctionDelegate(
         extensions::ManagementGetPermissionWarningsByManifestFunction* function,
         const std::string& manifest_str) const {
-  scoped_refptr<safe_json::SafeJsonParser> parser(
-      new safe_json::SafeJsonParser(
-          manifest_str,
-          base::Bind(
-              &extensions::ManagementGetPermissionWarningsByManifestFunction::
-                  OnParseSuccess,
-              function),
-          base::Bind(
-              &extensions::ManagementGetPermissionWarningsByManifestFunction::
-                  OnParseFailure,
-              function)));
-  parser->Start();
+  safe_json::SafeJsonParser::Parse(
+      manifest_str,
+      base::Bind(
+          &extensions::ManagementGetPermissionWarningsByManifestFunction::
+              OnParseSuccess,
+          function),
+      base::Bind(
+          &extensions::ManagementGetPermissionWarningsByManifestFunction::
+              OnParseFailure,
+          function));
 }
 
 scoped_ptr<extensions::InstallPromptDelegate>

diff --git a/chrome/browser/extensions/webstore_data_fetcher.cc b/chrome/browser/extensions/webstore_data_fetcher.cc
@@ -82,13 +82,11 @@ void WebstoreDataFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   std::string webstore_json_data;
   fetcher->GetResponseAsString(&webstore_json_data);
 
-  scoped_refptr<safe_json::SafeJsonParser> parser =
-      new safe_json::SafeJsonParser(
-          webstore_json_data,
-          base::Bind(&WebstoreDataFetcher::OnJsonParseSuccess, AsWeakPtr()),
-          base::Bind(&WebstoreDataFetcher::OnJsonParseFailure, AsWeakPtr()));
   // The parser will call us back via one of the callbacks.
-  parser->Start();
+  safe_json::SafeJsonParser::Parse(
+      webstore_json_data,
+      base::Bind(&WebstoreDataFetcher::OnJsonParseSuccess, AsWeakPtr()),
+      base::Bind(&WebstoreDataFetcher::OnJsonParseFailure, AsWeakPtr()));
 }
 
 }  // namespace extensions
diff --git a/chrome/browser/extensions/webstore_install_helper.cc b/chrome/browser/extensions/webstore_install_helper.cc
@@ -43,19 +43,9 @@ WebstoreInstallHelper::~WebstoreInstallHelper() {}
 void WebstoreInstallHelper::Start() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // No existing |json_parser_| to avoid unbalanced AddRef().
-  CHECK(!json_parser_.get());
-  AddRef();  // Balanced in OnJSONParseSucceeded()/OnJSONParseFailed().
-  // Use base::Unretained so that base::Bind won't AddRef() on us. Otherwise,
-  // we'll have two callbacks holding references to us, only one of which will
-  // ever be called.
-  json_parser_ = new safe_json::SafeJsonParser(
-      manifest_,
-      base::Bind(&WebstoreInstallHelper::OnJSONParseSucceeded,
-                 base::Unretained(this)),
-      base::Bind(&WebstoreInstallHelper::OnJSONParseFailed,
-                 base::Unretained(this)));
-  json_parser_->Start();
+  safe_json::SafeJsonParser::Parse(
+      manifest_, base::Bind(&WebstoreInstallHelper::OnJSONParseSucceeded, this),
+      base::Bind(&WebstoreInstallHelper::OnJSONParseFailed, this));
 
   if (icon_url_.is_empty()) {
     icon_decode_complete_ = true;
@@ -103,7 +93,6 @@ void WebstoreInstallHelper::OnJSONParseSucceeded(
     parse_error_ = Delegate::MANIFEST_ERROR;
 
   ReportResultsIfComplete();
-  Release();  // Balanced in Start().
 }
 
 void WebstoreInstallHelper::OnJSONParseFailed(
@@ -113,7 +102,6 @@ void WebstoreInstallHelper::OnJSONParseFailed(
   error_ = error_message;
   parse_error_ = Delegate::MANIFEST_ERROR;
   ReportResultsIfComplete();
-  Release();  // Balanced in Start().
 }
 
 void WebstoreInstallHelper::ReportResultsIfComplete() {

diff --git a/chrome/browser/extensions/webstore_install_helper.h b/chrome/browser/extensions/webstore_install_helper.h
@@ -96,8 +96,6 @@ class WebstoreInstallHelper : public base::RefCounted<WebstoreInstallHelper>,
   // The manifest to parse.
   std::string manifest_;
 
-  scoped_refptr<safe_json::SafeJsonParser> json_parser_;
-
   // If |icon_url_| is non-empty, it needs to be fetched and decoded into an
   // SkBitmap.
   GURL icon_url_;

diff --git a/chrome/browser/safe_json_parser_browsertest.cc b/chrome/browser/safe_json_parser_browsertest.cc
@@ -54,9 +54,7 @@ class SafeJsonParserTest : public InProcessBrowserTest {
       error_callback = base::Bind(&SafeJsonParserTest::ExpectError,
                                   base::Unretained(this), error);
     }
-    scoped_refptr<SafeJsonParser> parser =
-        new SafeJsonParser(json, success_callback, error_callback);
-    parser->Start();
+    SafeJsonParser::Parse(json, success_callback, error_callback);
 
     message_loop_runner_->Run();
     message_loop_runner_ = nullptr;

diff --git a/chrome/browser/supervised_user/supervised_user_site_list.cc b/chrome/browser/supervised_user/supervised_user_site_list.cc
@@ -25,8 +25,6 @@ const char kWhitelistKey[] = "whitelist";
 
 namespace {
 
-bool g_load_in_process = false;
-
 std::string ReadFileOnBlockingThread(const base::FilePath& path) {
   SCOPED_UMA_HISTOGRAM_TIMER("ManagedUsers.Whitelist.ReadDuration");
   std::string contents;
@@ -114,11 +112,6 @@ void SupervisedUserSiteList::Load(const base::FilePath& path,
       base::Bind(&SupervisedUserSiteList::ParseJson, path, callback));
 }
 
-// static
-void SupervisedUserSiteList::SetLoadInProcessForTesting(bool in_process) {
-  g_load_in_process = in_process;
-}
-
 SupervisedUserSiteList::SupervisedUserSiteList(const base::ListValue& sites) {
   for (const base::Value* site : sites) {
     const base::DictionaryValue* entry = nullptr;
@@ -142,26 +135,12 @@ void SupervisedUserSiteList::ParseJson(
     const base::FilePath& path,
     const SupervisedUserSiteList::LoadedCallback& callback,
     const std::string& json) {
-  if (g_load_in_process) {
-    JSONFileValueDeserializer deserializer(path);
-    std::string error;
-    scoped_ptr<base::Value> value(deserializer.Deserialize(nullptr, &error));
-    if (!value) {
-      HandleError(path, error);
-      return;
-    }
-
-    OnJsonParseSucceeded(path, base::TimeTicks(), callback, value.Pass());
-    return;
-  }
-
-  // TODO(bauerb): Use batch mode to load multiple whitelists?
-  scoped_refptr<safe_json::SafeJsonParser> parser(
-      new safe_json::SafeJsonParser(
-          json, base::Bind(&SupervisedUserSiteList::OnJsonParseSucceeded, path,
-                           base::TimeTicks::Now(), callback),
-          base::Bind(&HandleError, path)));
-  parser->Start();
+  // TODO(bauerb): Use JSONSanitizer to sanitize whitelists on installation
+  // instead of using the expensive SafeJsonParser on every load.
+  safe_json::SafeJsonParser::Parse(
+      json, base::Bind(&SupervisedUserSiteList::OnJsonParseSucceeded, path,
+                       base::TimeTicks::Now(), callback),
+      base::Bind(&HandleError, path));
 }
 
 // static

diff --git a/chrome/browser/supervised_user/supervised_user_site_list.h b/chrome/browser/supervised_user/supervised_user_site_list.h
@@ -60,11 +60,6 @@ class SupervisedUserSiteList
   // the newly created object.
   static void Load(const base::FilePath& file, const LoadedCallback& callback);
 
-  // Sets whether the site list should be loaded in-process or out-of-process.
-  // In-process loading should only be used in tests (to avoid having to set up
-  // child process handling).
-  static void SetLoadInProcessForTesting(bool in_process);
-
   // Returns a list of all sites in this site list.
   const std::vector<Site>& sites() const { return sites_; }
 

diff --git a/chrome/browser/supervised_user/supervised_user_whitelist_service_unittest.cc b/chrome/browser/supervised_user/supervised_user_whitelist_service_unittest.cc
@@ -21,6 +21,7 @@
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
+#include "components/safe_json/testing_json_parser.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_error_factory.h"
@@ -90,11 +91,6 @@ class SupervisedUserWhitelistServiceTest : public testing::Test {
     service_->AddSiteListsChangedCallback(
         base::Bind(&SupervisedUserWhitelistServiceTest::OnSiteListsChanged,
                    base::Unretained(this)));
-    SupervisedUserSiteList::SetLoadInProcessForTesting(true);
-  }
-
-  ~SupervisedUserWhitelistServiceTest() override {
-    SupervisedUserSiteList::SetLoadInProcessForTesting(false);
   }
 
  protected:
@@ -156,6 +152,8 @@ class SupervisedUserWhitelistServiceTest : public testing::Test {
   content::TestBrowserThreadBundle thread_bundle_;
   TestingProfile profile_;
 
+  safe_json::TestingJsonParser::ScopedFactoryOverride factory_override_;
+
   scoped_ptr<MockSupervisedUserWhitelistInstaller> installer_;
   scoped_ptr<SupervisedUserWhitelistService> service_;
 

diff --git a/chrome/browser/ui/app_list/search/common/json_response_fetcher.cc b/chrome/browser/ui/app_list/search/common/json_response_fetcher.cc
@@ -72,14 +72,12 @@ void JSONResponseFetcher::OnURLFetchComplete(
   std::string json_data;
   fetcher->GetResponseAsString(&json_data);
 
-  scoped_refptr<safe_json::SafeJsonParser> parser =
-      new safe_json::SafeJsonParser(
-          json_data, base::Bind(&JSONResponseFetcher::OnJsonParseSuccess,
-                                weak_factory_.GetWeakPtr()),
-          base::Bind(&JSONResponseFetcher::OnJsonParseError,
-                     weak_factory_.GetWeakPtr()));
   // The parser will call us back via one of the callbacks.
-  parser->Start();
+  safe_json::SafeJsonParser::Parse(
+      json_data, base::Bind(&JSONResponseFetcher::OnJsonParseSuccess,
+                            weak_factory_.GetWeakPtr()),
+      base::Bind(&JSONResponseFetcher::OnJsonParseError,
+                 weak_factory_.GetWeakPtr()));
 }
 
 }  // namespace app_list
diff --git a/chrome/browser/web_resource/chrome_web_resource_service.cc b/chrome/browser/web_resource/chrome_web_resource_service.cc
@@ -38,9 +38,5 @@ void ChromeWebResourceService::ParseJSON(
     const std::string& data,
     const SuccessCallback& success_callback,
     const ErrorCallback& error_callback) {
-  // SafeJsonParser releases itself on completion.
-  scoped_refptr<safe_json::SafeJsonParser> json_parser(
-      new safe_json::SafeJsonParser(data, success_callback,
-                                           error_callback));
-  json_parser->Start();
+  safe_json::SafeJsonParser::Parse(data, success_callback, error_callback);
 }
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
@@ -2221,6 +2221,7 @@
             '../components/components.gyp:autofill_content_test_support',
             '../components/components.gyp:component_metrics_proto',
             '../components/components.gyp:data_reduction_proxy_test_support',
+            '../components/components.gyp:safe_json_test_support',
             '../components/components.gyp:webdata_services_test_support',
             '../components/components_strings.gyp:components_strings',
             '../content/app/resources/content_resources.gyp:content_resources',

diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
@@ -1274,6 +1274,7 @@ if (!is_android) {
         "//components/autofill/content/renderer:test_support",
         "//components/metrics/proto",
         "//components/data_reduction_proxy/core/browser:test_support",
+        "//components/safe_json:test_support",
         "//components/webdata_services:test_support",
         "//components/strings",
         "//device/bluetooth:mocks",

diff --git a/components/safe_json.gypi b/components/safe_json.gypi
@@ -25,6 +25,23 @@
       'sources': [
         'safe_json/safe_json_parser.cc',
         'safe_json/safe_json_parser.h',
+        'safe_json/safe_json_parser_impl.cc',
+        'safe_json/safe_json_parser_impl.h',
+      ],
+    },
+    {
+      'target_name': 'safe_json_test_support',
+      'type': 'static_library',
+      'dependencies': [
+        '../base/base.gyp:base',
+        ':safe_json',
+      ],
+      'include_dirs': [
+        '..',
+      ],
+      'sources': [
+        'safe_json/testing_json_parser.cc',
+        'safe_json/testing_json_parser.h',
       ],
     },
     {

diff --git a/components/safe_json/BUILD.gn b/components/safe_json/BUILD.gn
@@ -7,6 +7,8 @@ static_library("safe_json") {
   sources = [
     "safe_json_parser.cc",
     "safe_json_parser.h",
+    "safe_json_parser_impl.cc",
+    "safe_json_parser_impl.h",
   ]
 
   defines = [ "SAFE_JSON_IMPLEMENTATION" ]
@@ -20,6 +22,21 @@ static_library("safe_json") {
   ]
 }
 
+# GYP version: components/safe_json.gypi:safe_json_test_support
+source_set("test_support") {
+  testonly = true
+  sources = [
+    "testing_json_parser.cc",
+    "testing_json_parser.h",
+  ]
+
+  deps = [
+    ":safe_json",
+    "//base",
+  ]
+}
+
+# GYP version: components/safe_json.gypi:safe_json_parser_message_filter
 static_library("safe_json_parser_message_filter") {
   sources = [
     "safe_json_parser_message_filter.cc",