Add the UtilityProcessMojoClient class and convert SafeJsonParser

components/safe_json/safe_json_parser_impl.cc

Index: components/safe_json/safe_json_parser_impl.cc
diff --git a/components/safe_json/safe_json_parser_impl.cc b/components/safe_json/safe_json_parser_impl.cc
index 40edbc7e112a1a27d6bf6aaf408f2a8f5069d837..1df0de4746bb3a99e4533ae906a46a4a7cce7927 100644
--- a/components/safe_json/safe_json_parser_impl.cc
+++ b/components/safe_json/safe_json_parser_impl.cc
@@ -4,23 +4,14 @@
 
 #include "components/safe_json/safe_json_parser_impl.h"
 
-#include <utility>
-
+#include "base/callback.h"
 #include "base/sequenced_task_runner.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "base/tuple.h"
 #include "base/values.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/utility_process_host.h"
-#include "content/public/common/service_registry.h"
 #include "grit/components_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using content::BrowserThread;
-using content::UtilityProcessHost;
-
 namespace safe_json {
 
 SafeJsonParserImpl::SafeJsonParserImpl(const std::string& unsafe_json,
@@ -32,90 +23,78 @@ SafeJsonParserImpl::SafeJsonParserImpl(const std::string& unsafe_json,
   io_thread_checker_.DetachFromThread();
 }
 
-SafeJsonParserImpl::~SafeJsonParserImpl() {
+SafeJsonParserImpl::~SafeJsonParserImpl() = default;
+
+void SafeJsonParserImpl::Start() {
+  caller_task_runner_ = base::SequencedTaskRunnerHandle::Get();
+
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&SafeJsonParserImpl::StartOnIoThread, this));
 }
 
-void SafeJsonParserImpl::StartWorkOnIOThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+void SafeJsonParserImpl::StartOnIoThread() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   DCHECK(io_thread_checker_.CalledOnValidThread());

[grt (UTC plus 2), 2016/06/09 14:48:58]

what's the advantage of doing these two things? why not remove
io_thread_checker_ and use CURRENTLY_ON in the callback functions below?

[Patrick Monette, 2016/06/10 18:13:23]

There's a comment explaining this with the declaration of
io_thread_checker_ in the .h file. Should I move the comment to the
.cc file?

[grt (UTC plus 2), 2016/06/10 19:03:06]

Ah, I missed that. The problem is that CURRENTLY_ON doesn't work during
shutdown, but the correct thread is used nonetheless? Does the rest of Chrome
not have a problem with this?

[Patrick Monette, 2016/06/10 21:29:46]

Maybe amistry@ can answer that? :)

[Anand Mistry (off Chromium), 2016/06/14 00:32:29]

Um... yes... this is a problem with the rest of Chrome.... kinda...

It's only a problem with MessageLoop destruction observers. The reason it's a
problem here is because the connection error callback will be run during
MessageLoop shutdown.

-  // TODO(amistry): This UtilityProcessHost dance is likely to be done multiple
-  // times as more tasks are migrated to Mojo. Create some sort of helper class
-  // to eliminate the code duplication.
-  utility_process_host_ = UtilityProcessHost::Create(
-      this, base::ThreadTaskRunnerHandle::Get().get())->AsWeakPtr();
-  utility_process_host_->SetName(
-      l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_JSON_PARSER_NAME));
-  if (!utility_process_host_->Start()) {
-    ReportResults();
-    return;
-  }
+  mojo_json_parser_.reset(
+      new content::UtilityProcessMojoClient<mojom::SafeJsonParser>(
+          l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_JSON_PARSER_NAME)));
 
-  content::ServiceRegistry* service_registry =
-      utility_process_host_->GetServiceRegistry();
-  service_registry->ConnectToRemoteService(mojo::GetProxy(&service_));
-  service_.set_connection_error_handler(
-      base::Bind(&SafeJsonParserImpl::ReportResults, this));
+  mojo_json_parser_->set_error_handler(
+      base::Bind(&SafeJsonParserImpl::OnConnectionError, this));
+  mojo_json_parser_->Start();
 
-  service_->Parse(unsafe_json_,
-                  base::Bind(&SafeJsonParserImpl::OnParseDone, this));
+  mojo_json_parser_->service()->Parse(
+      unsafe_json_, base::Bind(&SafeJsonParserImpl::OnParseDone, this));

[grt (UTC plus 2), 2016/06/09 14:48:58]

does this class support calling |Start| multiple times? if not, consider
std::moving unsafe_json_ into the Parse call so that the memory for the source
can be released once it has been sent along. might be worth it if the input
could be large.

[Patrick Monette, 2016/06/10 18:13:23]

Done.

 }
 
-void SafeJsonParserImpl::ReportResults() {
-  // There should be a DCHECK_CURRENTLY_ON(BrowserThread::IO) here. However, if
-  // the parser process is still alive on shutdown, this might run on the IO
-  // thread while the IO thread message loop is shutting down. This happens
-  // after the IO thread has unregistered from the BrowserThread list, causing
-  // the DCHECK to fail.
+void SafeJsonParserImpl::OnConnectionError() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   io_thread_checker_.DetachFromThread();

[Anand Mistry (off Chromium), 2016/06/14 00:32:29]

I know this was already there, but the DetachFromThread is only needed in the
constructor. You don't need it here or in OnParseDone.

[Patrick Monette, 2016/06/14 21:23:32]

Removed.

 
-  // Maintain a reference to |this| since either |utility_process_host_| or
-  // |service_| may have the last reference and destroying those might delete
-  // |this|.
-  scoped_refptr<SafeJsonParserImpl> ref(this);
-  // Shut down the utility process if it's still running.
-  delete utility_process_host_.get();
-  service_.reset();
-
   caller_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&SafeJsonParserImpl::ReportResultsOnOriginThread, this));
-}
+      FROM_HERE, base::Bind(&SafeJsonParserImpl::ReportResults, this, nullptr,
+                            "Json error"));
 
-void SafeJsonParserImpl::ReportResultsOnOriginThread() {
-  DCHECK(caller_task_runner_->RunsTasksOnCurrentThread());
-  if (error_.empty() && parsed_json_) {
-    if (!success_callback_.is_null())
-      success_callback_.Run(std::move(parsed_json_));
-  } else {
-    if (!error_callback_.is_null())
-      error_callback_.Run(error_);
-  }
-}
-
-bool SafeJsonParserImpl::OnMessageReceived(const IPC::Message& message) {
-  return false;
+  // Shut down the utility process. Doing this last to make sure the last
+  // reference to this is not released before binding with ReportResults().
+  mojo_json_parser_.reset();
 }
 
 void SafeJsonParserImpl::OnParseDone(const base::ListValue& wrapper,
-                                     mojo::String error) {
+                                     const mojo::String& error) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  io_thread_checker_.DetachFromThread();
+
+  // Shut down the utility process.
+  mojo_json_parser_.reset();
+
+  std::unique_ptr<base::Value> parsed_json;
+  std::string error_message;
   if (!wrapper.empty()) {
     const base::Value* value = NULL;
     CHECK(wrapper.Get(0, &value));
-    parsed_json_.reset(value->DeepCopy());
+    parsed_json.reset(value->DeepCopy());
   } else {
-    error_ = error.get();
+    error_message = error.get();
   }
-  ReportResults();
-}
 
-void SafeJsonParserImpl::Start() {
-  caller_task_runner_ = base::SequencedTaskRunnerHandle::Get();
+  // Call ReportResults() on caller's thread.
+  caller_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SafeJsonParserImpl::ReportResults, this,
+                            base::Passed(&parsed_json), error_message));
+}
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&SafeJsonParserImpl::StartWorkOnIOThread, this));
+void SafeJsonParserImpl::ReportResults(std::unique_ptr<base::Value> parsed_json,

[grt (UTC plus 2), 2016/06/09 14:48:58]

apologies if this is a silly question; i haven't looked at the details too
closely: is this method needed? can the spots above that post tasks to run this
instead directly post the proper callback and args?

[Patrick Monette, 2016/06/10 18:13:23]

It was an artifact of when this class was refcounted and needed to be destroyed
on the caller_task_runner. In the last patch set, it is a unique exit point
where the clean-up is done. I think it's useful.

+                                       const std::string& error) {
+  DCHECK(caller_task_runner_->RunsTasksOnCurrentThread());
+  if (error.empty() && parsed_json) {
+    if (!success_callback_.is_null())
+      success_callback_.Run(std::move(parsed_json));
+  } else {
+    if (!error_callback_.is_null())
+      error_callback_.Run(error);
+  }
 }
 
 }  // namespace safe_json