Add the UtilityProcessMojoClient class and convert SafeJsonParser

components/safe_json/safe_json_parser_impl.cc

 // Copyright 2013 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 "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,
                                        const SuccessCallback& success_callback,
                                        const ErrorCallback& error_callback)
     : unsafe_json_(unsafe_json),
       success_callback_(success_callback),
       error_callback_(error_callback) {
   io_thread_checker_.DetachFromThread();
 }
 
-SafeJsonParserImpl::~SafeJsonParserImpl() {
-}
-
-void SafeJsonParserImpl::StartWorkOnIOThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  // 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;
-  }
-
-  content::ServiceRegistry* service_registry =
-      utility_process_host_->GetServiceRegistry();
-  service_registry->ConnectToRemoteService(mojo::GetProxy(&service_));
-  service_.set_connection_error_handler(
-      base::Bind(&SafeJsonParserImpl::ReportResults, this));
-
-  service_->Parse(unsafe_json_,
-                  base::Bind(&SafeJsonParserImpl::OnParseDone, this));
-}
-
-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.
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  io_thread_checker_.DetachFromThread();
-
-  // 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));
-}
-
-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;
-}
-
-void SafeJsonParserImpl::OnParseDone(const base::ListValue& wrapper,
-                                     mojo::String error) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
-  if (!wrapper.empty()) {
-    const base::Value* value = NULL;
-    CHECK(wrapper.Get(0, &value));
-    parsed_json_.reset(value->DeepCopy());
-  } else {
-    error_ = error.get();
-  }
-  ReportResults();
-}
+SafeJsonParserImpl::~SafeJsonParserImpl() = default;
 
 void SafeJsonParserImpl::Start() {
   caller_task_runner_ = base::SequencedTaskRunnerHandle::Get();
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::Bind(&SafeJsonParserImpl::StartWorkOnIOThread, this));
+  content::BrowserThread::PostTask(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&SafeJsonParserImpl::StartOnIoThread, this));
+}
+
+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.

+  mojo_json_parser_.reset(
+      new content::UtilityProcessMojoClient<mojom::SafeJsonParser>(
+          l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_JSON_PARSER_NAME)));
+
+  mojo_json_parser_->set_error_handler(
+      base::Bind(&SafeJsonParserImpl::OnConnectionError, this));
+  mojo_json_parser_->Start();
+
+  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::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.

+
+  caller_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SafeJsonParserImpl::ReportResults, this, nullptr,
+                            "Json error"));
+
+  // 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,
+                                     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());
+  } else {
+    error_message = error.get();
+  }
+
+  // Call ReportResults() on caller's thread.
+  caller_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SafeJsonParserImpl::ReportResults, this,
+                            base::Passed(&parsed_json), error_message));
+}
+
+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