Convert the utility process JSON parser into a Mojo service.

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/sequenced_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/tuple.h"
 #include "base/values.h"
-#include "components/safe_json/safe_json_parser_messages.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 "ipc/ipc_message_macros.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) {}
 
 SafeJsonParserImpl::~SafeJsonParserImpl() {
 }
 
 void SafeJsonParserImpl::StartWorkOnIOThread() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  UtilityProcessHost* host = UtilityProcessHost::Create(
-      this, base::ThreadTaskRunnerHandle::Get().get());
-  host->SetName(
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  utility_process_host_ = UtilityProcessHost::Create(
+      this, base::ThreadTaskRunnerHandle::Get().get())->AsWeakPtr();
+  utility_process_host_->SetName(
       l10n_util::GetStringUTF16(IDS_UTILITY_PROCESS_JSON_PARSER_NAME));
-  host->Send(new SafeJsonParserMsg_ParseJSON(unsafe_json_));
-}
+  if (!utility_process_host_->Start()) {
+    ReportResults();
+    return;
+  }
 
-void SafeJsonParserImpl::OnJSONParseSucceeded(const base::ListValue& wrapper) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  const base::Value* value = NULL;
-  CHECK(wrapper.Get(0, &value));
+  content::ServiceRegistry* service_registry =
+      utility_process_host_->GetServiceRegistry();
+  service_registry->ConnectToRemoteService(mojo::GetProxy(&service_));
+  service_.set_connection_error_handler(
+      base::Bind(&SafeJsonParserImpl::ReportResults, this));
 
-  parsed_json_.reset(value->DeepCopy());
-  ReportResults();
-}
-
-void SafeJsonParserImpl::OnJSONParseFailed(const std::string& error_message) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  error_ = error_message;
-  ReportResults();
+  service_->Parse(unsafe_json_,
+                  base::Bind(&SafeJsonParserImpl::OnParseDone, this));
 }
 
 void SafeJsonParserImpl::ReportResults() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // 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

[Bernhard Bauer, 2016/04/13 15:46:51]

Huh? Isn't the thread destroyed and cleaned up after it stopped running its
message loop
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

[Anand Mistry (off Chromium), 2016/04/14 00:59:49]

This case happens due to message loop destruction observers. Because I added the
Mojo connection error handler above, it's possible for this function to be run
via a destruction observer (because Mojo services are tied to a message loop).
Destruction observers are run inside the message loop destructor. And the
message loop is destroyed after the thread is unregistered from the
BrowserThread list (the last line of BrowserThread::Cleanup). Or, put another
way, Thread::Cleanup() runs before ~MessageLoop().

[Bernhard Bauer, 2016/04/14 14:11:38]

Hm, could we use a ThreadChecker in this case? That one looks at the actual
thread it's used on.

[Anand Mistry (off Chromium), 2016/04/18 05:06:45]

Done.

+  // after the IO thread has unregistered from the BrowserThread list, causing
+  // the DCHECK to fail.
+
+  // Maintain a reference to |this| since either |utility_process_host_| or
+  // |service_| may have to last reference and destroying those might delete
+  // |this|.
+  scoped_refptr<SafeJsonParserImpl> ref(this);
+  // Shut down the utility process if it's still running.

[Bernhard Bauer, 2016/04/13 15:46:51]

Shouldn't the utility process shut itself down when it has nothing to do? From
what I've seen with other uses of UtilityProcessHost, if we only need it for a
one-shot operation, we should be able to get away with not keeping a pointer to
it at all.

[Anand Mistry (off Chromium), 2016/04/14 00:59:49]

This approach doesn't work well for Mojo. If the process shuts itself down right
after sending the reply, there's no guarantee that the reply has been sent.
Also, if the process is responsible for shutting itself down, a compromised or
buggy process may not. So instead, I'm doing shutdown by the thing that starts
and "owns" the process so that there's some guarantee that the process is dead
and buried.

[Bernhard Bauer, 2016/04/14 14:11:38]

That seems... problematic. This change doesn't really extend the lifetime of the
utility process, so how would we guarantee that we get a result back then?
I get that, and I do like the idea, but this basically throws away any form of
ownership model: the utility process host might get deleted from somewhere else,
or we might delete it -- so who owns it?

Perhaps even more worryingly, how do we prevent double frees?
BrowserChildProcessHostImpl deletes its delegate (i.e. the
UtilityProcessHostImpl) without going through a weak pointer, so if this method
is called before that happens, we will delete it twice.

For that matter, if the UtilityProcessHost is destroyed, what would actually end
up killing a rogue utility process that has not shut itself down? I tried
following the various process hosts, but I couldn't find anything that would
actually kill the process.

[Anand Mistry (off Chromium), 2016/04/18 05:06:45]

Because inside the utility process, the implementation no longer calls
ReleaseProcessIfNeeded() when done.
UtilityProcessHost kinda has dual ownership. The two owners are:
1. The child process itself because if the child process shuts down, the
UtilityProcessHost gets deleted.
2. The user of the UtilityProcessHost, but via a WeakPtr.
Ownership #1 is kinda yuck. A child process shouldn't own its host. Ownership #2
is also yuck, but needs to be as a result of #1.
WeakPtr<>. As long as we're not re-entering the UtilityProcessHostImpl's
destructor (which doesn't happen), deleting WeakPtr<UtilityProcessHost>.get() is
correct behavior.
UtilityProcessHostImpl owns its BrowserChildProcessHostImpl via a unique_ptr.
Deleting the former will delete the latter without trying to delete the
UtilityProcessHostImpl.
Arg!!! Too levels of abstraction!!!
UtilityProcessHostImpl own a BrowserChildProcessHostImpl which owns a
ChildProcessLauncher. If a child process is still running when
ChildProcessLauncher is destroyed, the child process is force killed.
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch...)

[Bernhard Bauer, 2016/04/20 16:43:15]

Thanks for the explanation! I'm still somewhat concerned that we might end up
calling OnChannelError somehow during destruction, but I might just convince
myself that doesn't happen ☺

So, I realize this is not actually your fault, but from an outside perspective,
this switch to Mojo does not really seem to improve things. The ownership model
becomes even more complicated (if it can even still be called "ownership" at
this point -- it's more Wild West to me 😉), and if we assume that all usage of
utility process is eventually going to be converted to Mojo, we would have to do
this all over the place, where in the past we had an interface that was at least
fairly straightforward for clients; most of the time it was fire-and-forget.

I do like the idea of keeping the utility process around while it's needed, so
could we maybe move this into a dedicated class, something like
UtilityProcessHostHandle? That way, other clients that are converted to Mojo
could use that as well, and we'd at least keep the destruction shenanigans
fairly contained.
Oh, right. I didn't think to check ChildProcessLauncher, because I assumed it
would only deal with _launching_ the process... ☺

[Anand Mistry (off Chromium), 2016/04/21 07:54:12]

The good thing about UtilityProcessHost is that it call all it's delegate
functions via a post-task, so no reentrency.
So I've looked at a few utility process things since my goal is to do all of
them this quarter (probably won't happen, but it's my goal). One thing I've
noticed is that almost all of them get something wrong. Either they don't handle
startup failures, or crashes, or something else. The fire-and-forget model
doesn't really work (or is used) correctly right now.
I was thinking the same thing. However, I wanted to get a couple of conversions
out of the way so that I can see what exactly is common and refactor that later.
Right now, there is only one traditional use of the utility process with Mojo,
ImageDecoder. I want one or two more before I refactor that logic. And keep in
mind, I want to decouple all of this from "utility processes" as much as
possible to make it easy to migrate to a "mojo apps" world.

+  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) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(SafeJsonParserImpl, message)
-    IPC_MESSAGE_HANDLER(SafeJsonParserHostMsg_ParseJSON_Succeeded,
-                        OnJSONParseSucceeded)
-    IPC_MESSAGE_HANDLER(SafeJsonParserHostMsg_ParseJSON_Failed,
-                        OnJSONParseFailed)
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+  return false;
+}
+
+void SafeJsonParserImpl::OnParseDone(const base::ListValue& wrapper,

[Bernhard Bauer, 2016/04/13 15:46:51]

IIRC, this was a ListValue in the pre-Mojo version because of some weird issues
that wouldn't let us use a Value instead (see the comment in
safe_json_parser_messages.h). Maybe that doesn't apply anymore to Mojo?

[Anand Mistry (off Chromium), 2016/04/14 00:59:49]

That still applies because this change doesn't use a Mojo type for serialising
base::Values. It still uses the ChromeIPC ParamTraits<> for serialising which
requires exact types for C++ template matching (I think).

+                                     mojo::String error) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!wrapper.empty()) {
+    const base::Value* value = NULL;
+    CHECK(wrapper.Get(0, &value));
+    parsed_json_.reset(value->DeepCopy());
+  } else {
+    error_ = error.get();
+  }
+  ReportResults();
 }
 
 void SafeJsonParserImpl::Start() {
   caller_task_runner_ = base::SequencedTaskRunnerHandle::Get();
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&SafeJsonParserImpl::StartWorkOnIOThread, this));
 }
 
 }  // namespace safe_json