Data decoder service: Allow process sharing in SafeJsonParser.

This CL adds SafeJsonParser::ParseBatch which also takes in a |batch_id|
parameter. This allows clients to potentially use the same process for multiple
usages of data decoder service.

BUG=852058

Change-Id: If23f0cad2e819a6d3a2c0a136aab57284937f20c
Reviewed-on: https://chromium-review.googlesource.com/1105565
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Jay Civelli <jcivelli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569048}

chrome/browser/safe_json_parser_browsertest.cc

@@ -7,7 +7,9 @@
 #include "base/bind.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
+#include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/test_service_manager_listener.h"
@@ -24,6 +26,8 @@ namespace {
 
 using data_decoder::SafeJsonParser;
 
+constexpr char kTestJson[] = "[\"awesome\", \"possum\"]";
+
 std::string MaybeToJson(const base::Value* value) {
   if (!value)
     return "(null)";
@@ -35,32 +39,22 @@ std::string MaybeToJson(const base::Value* value) {
   return json;
 }
 
-class ParseCallback {
+class SafeJsonParserTest : public InProcessBrowserTest {
  public:
-  explicit ParseCallback(base::Closure callback) : callback_(callback) {}
+  SafeJsonParserTest() = default;
 
-  void OnSuccess(std::unique_ptr<base::Value> value) {
-    success_ = true;
-    callback_.Run();
-  }
+ protected:
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
 
-  void OnError(const std::string& error) {
-    success_ = false;
-    callback_.Run();
+    listener_.Init();
   }
 
-  bool success() const { return success_; }
-
- private:
-  bool success_ = false;
-  base::Closure callback_;
-
-  DISALLOW_COPY_AND_ASSIGN(ParseCallback);
-};
-
-class SafeJsonParserTest : public InProcessBrowserTest {
- protected:
-  void TestParse(const std::string& json) {
+  // Tests SafeJsonParser::Parse/ParseBatch. Parses |json| using SafeJsonParser
+  // and verifies that the correct callbacks are called. If |batch_id| is not
+  // empty, uses SafeJsonParser::ParseBatch to batch multiple parse requests.
+  void Parse(const std::string& json,
+             const base::Optional<std::string>& batch_id = base::nullopt) {
     SCOPED_TRACE(json);
     DCHECK(!message_loop_runner_);
     message_loop_runner_ = new content::MessageLoopRunner;
@@ -83,14 +77,25 @@ class SafeJsonParserTest : public InProcessBrowserTest {
       error_callback = base::Bind(&SafeJsonParserTest::ExpectError,
                                   base::Unretained(this), error);
     }
-    SafeJsonParser::Parse(
-        content::ServiceManagerConnection::GetForProcess()->GetConnector(),
-        json, success_callback, error_callback);
+
+    if (batch_id) {
+      SafeJsonParser::ParseBatch(
+          content::ServiceManagerConnection::GetForProcess()->GetConnector(),
+          json, success_callback, error_callback, *batch_id);
+    } else {
+      SafeJsonParser::Parse(
+          content::ServiceManagerConnection::GetForProcess()->GetConnector(),
+          json, success_callback, error_callback);
+    }
 
     message_loop_runner_->Run();
     message_loop_runner_ = nullptr;
   }
 
+  uint32_t GetServiceStartCount(const std::string& service_name) const {
+    return listener_.GetServiceStartCount(service_name);
+  }
+
  private:
   void ExpectValue(std::unique_ptr<base::Value> expected_value,
                    std::unique_ptr<base::Value> actual_value) {
@@ -117,81 +122,49 @@ class SafeJsonParserTest : public InProcessBrowserTest {
   }
 
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
-};
-
-class SafeJsonParserImplTest : public InProcessBrowserTest {
- public:
-  SafeJsonParserImplTest() = default;
-
- protected:
-  // InProcessBrowserTest implementation:
-  void SetUpOnMainThread() override {
-    InProcessBrowserTest::SetUpOnMainThread();
-
-    // Initialize the TestServiceManagerListener so it starts listening for
-    // service activity.
-    listener_.Init();
-
-    // The data_decoder service will stop if no connection is bound to it after
-    // 5 seconds. We bind a connection to it for the duration of the test so it
-    // is guaranteed the service is always running.
-    connector()->BindInterface(data_decoder::mojom::kServiceName,
-                               &json_parser_ptr_);
-    listener_.WaitUntilServiceStarted(data_decoder::mojom::kServiceName);
-    EXPECT_EQ(
-        1U, listener_.GetServiceStartCount(data_decoder::mojom::kServiceName));
-  }
-
-  service_manager::Connector* connector() const {
-    return content::ServiceManagerConnection::GetForProcess()->GetConnector();
-  }
-
-  uint32_t GetServiceStartCount(const std::string& service_name) const {
-    return listener_.GetServiceStartCount(service_name);
-  }
-
- private:
-  data_decoder::mojom::JsonParserPtr json_parser_ptr_;
   TestServiceManagerListener listener_;
 
-  DISALLOW_COPY_AND_ASSIGN(SafeJsonParserImplTest);
+  DISALLOW_COPY_AND_ASSIGN(SafeJsonParserTest);
 };
 
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(SafeJsonParserTest, Parse) {
-  TestParse("{}");
-  TestParse("choke");
-  TestParse("{\"awesome\": true}");
-  TestParse("\"laser\"");
-  TestParse("false");
-  TestParse("null");
-  TestParse("3.14");
-  TestParse("[");
-  TestParse("\"");
-  TestParse(std::string());
-  TestParse("☃");
-  TestParse("\"☃\"");
-  TestParse("\"\\ufdd0\"");
-  TestParse("\"\\ufffe\"");
-  TestParse("\"\\ud83f\\udffe\"");
+  Parse("{}");
+  Parse("choke");
+  Parse("{\"awesome\": true}");
+  Parse("\"laser\"");
+  Parse("false");
+  Parse("null");
+  Parse("3.14");
+  Parse("[");
+  Parse("\"");
+  Parse(std::string());
+  Parse("☃");
+  Parse("\"☃\"");
+  Parse("\"\\ufdd0\"");
+  Parse("\"\\ufffe\"");
+  Parse("\"\\ud83f\\udffe\"");
 }
 
 // Tests that when calling SafeJsonParser::Parse() a new service is started
 // every time.
-IN_PROC_BROWSER_TEST_F(SafeJsonParserImplTest, Isolation) {
+IN_PROC_BROWSER_TEST_F(SafeJsonParserTest, Isolation) {
+  for (int i = 0; i < 5; i++) {
+    SCOPED_TRACE(base::StringPrintf("Testing iteration %d", i));
+    Parse(kTestJson);
+    EXPECT_EQ(1U + i, GetServiceStartCount(data_decoder::mojom::kServiceName));
+  }
+}
+
+// Tests that using a batch ID allows service reuse.
+IN_PROC_BROWSER_TEST_F(SafeJsonParserTest, IsolationWithGroups) {
+  constexpr char kBatchId1[] = "batch1";
+  constexpr char kBatchId2[] = "batch2";
   for (int i = 0; i < 5; i++) {
-    base::RunLoop run_loop;
-    ParseCallback parse_callback(run_loop.QuitClosure());
-    SafeJsonParser::Parse(
-        connector(), "[\"awesome\", \"possum\"]",
-        base::Bind(&ParseCallback::OnSuccess,
-                   base::Unretained(&parse_callback)),
-        base::Bind(&ParseCallback::OnError, base::Unretained(&parse_callback)));
-    run_loop.Run();
-    EXPECT_TRUE(parse_callback.success());
-    // 2 + i below because the data_decoder is already running and the index
-    // starts at 0.
-    EXPECT_EQ(2U + i, GetServiceStartCount(data_decoder::mojom::kServiceName));
+    SCOPED_TRACE(base::StringPrintf("Testing iteration %d", i));
+    Parse(kTestJson, kBatchId1);
+    Parse(kTestJson, kBatchId2);
   }
+  EXPECT_EQ(2U, GetServiceStartCount(data_decoder::mojom::kServiceName));
 }

services/data_decoder/public/cpp/safe_json_parser.cc

@@ -4,6 +4,7 @@
 
 #include "services/data_decoder/public/cpp/safe_json_parser.h"
 
+#include "base/optional.h"
 #include "build/build_config.h"
 
 #if defined(OS_ANDROID)
@@ -21,7 +22,8 @@ SafeJsonParser::Factory g_factory = nullptr;
 SafeJsonParser* Create(service_manager::Connector* connector,
                        const std::string& unsafe_json,
                        const SafeJsonParser::SuccessCallback& success_callback,
-                       const SafeJsonParser::ErrorCallback& error_callback) {
+                       const SafeJsonParser::ErrorCallback& error_callback,
+                       const base::Optional<std::string>& batch_id) {
   if (g_factory)
     return g_factory(unsafe_json, success_callback, error_callback);
 
@@ -30,7 +32,7 @@ SafeJsonParser* Create(service_manager::Connector* connector,
                                    error_callback);
 #else
   return new SafeJsonParserImpl(connector, unsafe_json, success_callback,
-                                error_callback);
+                                error_callback, batch_id);
 #endif
 }
 
@@ -46,8 +48,19 @@ void SafeJsonParser::Parse(service_manager::Connector* connector,
                            const std::string& unsafe_json,
                            const SuccessCallback& success_callback,
                            const ErrorCallback& error_callback) {
-  SafeJsonParser* parser =
-      Create(connector, unsafe_json, success_callback, error_callback);
+  SafeJsonParser* parser = Create(connector, unsafe_json, success_callback,
+                                  error_callback, base::nullopt);
+  parser->Start();
+}
+
+// static
+void SafeJsonParser::ParseBatch(service_manager::Connector* connector,
+                                const std::string& unsafe_json,
+                                const SuccessCallback& success_callback,
+                                const ErrorCallback& error_callback,
+                                const std::string& batch_id) {
+  SafeJsonParser* parser = Create(connector, unsafe_json, success_callback,
+                                  error_callback, batch_id);
   parser->Start();
 }
 

services/data_decoder/public/cpp/safe_json_parser.h

@@ -36,9 +36,9 @@ class SafeJsonParser {
 
   // Starts parsing the passed in |unsafe_json| and calls either
   // |success_callback| or |error_callback| when finished.
-  // |connector| is the connector provided by the service manager and is used
-  // to retrieve the JSON decoder service. It's commonly retrieved from a
-  // service manager connection context object that the embedder provides.
+  // |connector| is the connector provided by the service manager and is used to
+  // retrieve the JSON decoder service. It's commonly retrieved from a service
+  // manager connection context object that the embedder provides.
   // Note that on Android, this runs in process, the sanitizing of |unsafe_json|
   // being performed in Java for safety. On other platforms the parsing happens
   // in an isolated sandboxed utility process.
@@ -47,6 +47,17 @@ class SafeJsonParser {
                     const SuccessCallback& success_callback,
                     const ErrorCallback& error_callback);
 
+  // Same as Parse, but allows clients to provide a |batch_id|, which the system
+  // may use to batch this parse request with other parse requests using the
+  // same |batch_id| in an effort to amortize the overhead of a single request.
+  // The trade-off is that batch requests may not be well-isolated from each
+  // other, so this should be used with appropriate caution.
+  static void ParseBatch(service_manager::Connector* connector,
+                         const std::string& unsafe_json,
+                         const SuccessCallback& success_callback,
+                         const ErrorCallback& error_callback,
+                         const std::string& batch_id);
+
   static void SetFactoryForTesting(Factory factory);
 
  protected:

services/data_decoder/public/cpp/safe_json_parser_impl.cc

@@ -16,19 +16,20 @@
 
 namespace data_decoder {
 
-SafeJsonParserImpl::SafeJsonParserImpl(service_manager::Connector* connector,
-                                       const std::string& unsafe_json,
-                                       const SuccessCallback& success_callback,
-                                       const ErrorCallback& error_callback)
+SafeJsonParserImpl::SafeJsonParserImpl(
+    service_manager::Connector* connector,
+    const std::string& unsafe_json,
+    const SuccessCallback& success_callback,
+    const ErrorCallback& error_callback,
+    const base::Optional<std::string>& batch_id)
     : unsafe_json_(unsafe_json),
       success_callback_(success_callback),
       error_callback_(error_callback) {
-  // Use a random instance ID to guarantee the connection is to a new service
-  // (running in its own process).
-  base::UnguessableToken token = base::UnguessableToken::Create();
-  service_manager::Identity identity(mojom::kServiceName,
-                                     service_manager::mojom::kInheritUserID,
-                                     token.ToString());
+  // If no batch ID has been provided, use a random instance ID to guarantee the
+  // connection is to a new service running in its own process.
+  service_manager::Identity identity(
+      mojom::kServiceName, service_manager::mojom::kInheritUserID,
+      batch_id.value_or(base::UnguessableToken::Create().ToString()));
   connector->BindInterface(identity, &json_parser_ptr_);
 }
 

services/data_decoder/public/cpp/safe_json_parser_impl.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "base/values.h"
 #include "services/data_decoder/public/cpp/safe_json_parser.h"
@@ -27,7 +28,8 @@ class SafeJsonParserImpl : public SafeJsonParser {
   SafeJsonParserImpl(service_manager::Connector* connector,
                      const std::string& unsafe_json,
                      const SuccessCallback& success_callback,
-                     const ErrorCallback& error_callback);
+                     const ErrorCallback& error_callback,
+                     const base::Optional<std::string>& batch_id);
 
  private:
   ~SafeJsonParserImpl() override;