Convert the utility process JSON parser into a Mojo service.

Convert the utility process JSON parser into a Mojo service.

BUG=597124
Committed: https://crrev.com/f8ce981e73207dfb46f6ccbfe2cd29378b3c4baf
Cr-Commit-Position: refs/heads/master@{#391463}

[Anand Mistry (off Chromium), 2016-04-05 03:39:23]

Description was changed from

==========
Move JSON parser to Mojo.

BUG=597124
==========

to

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

[Anand Mistry (off Chromium), 2016-04-05 03:39:42]

Description was changed from

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

to

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

[Anand Mistry (off Chromium), 2016-04-13 01:24:30]

amistry@chromium.org changed reviewers:
+ bauerb@chromium.org

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:60: // thread while the IO thread
message loop is shutting down. This happens
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...

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
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.

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:92: void
SafeJsonParserImpl::OnParseDone(const base::ListValue& wrapper,
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?

[Bernhard Bauer, 2016-04-13 15:49:08]

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_mojo_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_mojo_impl.cc:5: #include
"components/safe_json/safe_json_parser_mojo_impl.h"
Technically, we should split this up into a browser part and a utility part, no?
We got around that somewhat by having this in chrome/utility previously (which
admittedly was kind of wrong), but I would be a bit worried about just putting
utility and browser parts into the same directory now.

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:60: // thread while the IO thread
message loop is shutting down. This happens
On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> 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...

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().

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> 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.

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.

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:92: void
SafeJsonParserImpl::OnParseDone(const base::ListValue& wrapper,
On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> 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?

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).

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_mojo_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_mojo_impl.cc:5: #include
"components/safe_json/safe_json_parser_mojo_impl.h"
On 2016/04/13 15:49:08, Bernhard Bauer wrote:
> Technically, we should split this up into a browser part and a utility part,
no?
> We got around that somewhat by having this in chrome/utility previously (which
> admittedly was kind of wrong), but I would be a bit worried about just putting
> utility and browser parts into the same directory now.

Well, the utility-side message filter was still in this directory (and the
wrapper was a layering quirk) so what I have now is no different than before.
Also, build rules keep the two sides separate.

This situation exists in quite a few places with Mojo and there isn't a great
solution for it yet. The only solid pattern we have so far is where the .mojom
files live (public/interfaces). One pattern is to have "client libraries"
(SafeJsonParserImpl in this case) in public/ or public/cpp/, but that isn't
followed in many places. If you have any ideas, I'd be happy to hear them.

Something else to consider is that in the mid-future, another change will likely
happen where the utility process is split into Mojo "apps" and the utility
process as a dumping ground goes away. This is longer term, but something to
keep in mind when considering layering and directory structure.

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:60: // thread while the IO thread
message loop is shutting down. This happens
On 2016/04/14 00:59:49, Anand Mistry wrote:
> On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > 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...
> 
> 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().

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
On 2016/04/14 00:59:49, Anand Mistry wrote:
> On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > 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.
> 
> 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.

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?

> 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.

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.

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_mojo_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_mojo_impl.cc:5: #include
"components/safe_json/safe_json_parser_mojo_impl.h"
On 2016/04/14 00:59:49, Anand Mistry wrote:
> On 2016/04/13 15:49:08, Bernhard Bauer wrote:
> > Technically, we should split this up into a browser part and a utility part,
> no?
> > We got around that somewhat by having this in chrome/utility previously
(which
> > admittedly was kind of wrong), but I would be a bit worried about just
putting
> > utility and browser parts into the same directory now.
> 
> Well, the utility-side message filter was still in this directory (and the
> wrapper was a layering quirk) so what I have now is no different than before.
> Also, build rules keep the two sides separate.

Sure, they get built as different targets, but there is nothing keeping the code
in this class from including e.g. content/public/browser/browser_thread.h.

> This situation exists in quite a few places with Mojo and there isn't a great
> solution for it yet. The only solid pattern we have so far is where the .mojom
> files live (public/interfaces). One pattern is to have "client libraries"
> (SafeJsonParserImpl in this case) in public/ or public/cpp/, but that isn't
> followed in many places. If you have any ideas, I'd be happy to hear them.

Could we just move the _mojo_impl.* files into a utility/ subdirectory, and the
other files into a browser/ subdirectory? That would match the layout for other
components that are split across multiple processes.

> Something else to consider is that in the mid-future, another change will
likely
> happen where the utility process is split into Mojo "apps" and the utility
> process as a dumping ground goes away. This is longer term, but something to
> keep in mind when considering layering and directory structure.

[Anand Mistry (off Chromium), 2016-04-18 05:06:46]

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:60: // thread while the IO thread
message loop is shutting down. This happens
On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> On 2016/04/14 00:59:49, Anand Mistry wrote:
> > On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > > 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...
> > 
> > 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().
> 
> Hm, could we use a ThreadChecker in this case? That one looks at the actual
> thread it's used on.

Done.

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> On 2016/04/14 00:59:49, Anand Mistry wrote:
> > On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > > 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.
> > 
> > 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.
> 
> 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?

Because inside the utility process, the implementation no longer calls
ReleaseProcessIfNeeded() when done.

> 
> > 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.
> 
> 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?

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.

> Perhaps even more worryingly, how do we prevent double frees?

WeakPtr<>. As long as we're not re-entering the UtilityProcessHostImpl's
destructor (which doesn't happen), deleting WeakPtr<UtilityProcessHost>.get() is
correct behavior.

> 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.

UtilityProcessHostImpl owns its BrowserChildProcessHostImpl via a unique_ptr.
Deleting the former will delete the latter without trying to delete the
UtilityProcessHostImpl.

> 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.

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...)

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_mojo_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_mojo_impl.cc:5: #include
"components/safe_json/safe_json_parser_mojo_impl.h"
On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> On 2016/04/14 00:59:49, Anand Mistry wrote:
> > On 2016/04/13 15:49:08, Bernhard Bauer wrote:
> > > Technically, we should split this up into a browser part and a utility
part,
> > no?
> > > We got around that somewhat by having this in chrome/utility previously
> (which
> > > admittedly was kind of wrong), but I would be a bit worried about just
> putting
> > > utility and browser parts into the same directory now.
> > 
> > Well, the utility-side message filter was still in this directory (and the
> > wrapper was a layering quirk) so what I have now is no different than
before.
> > Also, build rules keep the two sides separate.
> 
> Sure, they get built as different targets, but there is nothing keeping the
code
> in this class from including e.g. content/public/browser/browser_thread.h.
> 
> > This situation exists in quite a few places with Mojo and there isn't a
great
> > solution for it yet. The only solid pattern we have so far is where the
.mojom
> > files live (public/interfaces). One pattern is to have "client libraries"
> > (SafeJsonParserImpl in this case) in public/ or public/cpp/, but that isn't
> > followed in many places. If you have any ideas, I'd be happy to hear them.
> 
> Could we just move the _mojo_impl.* files into a utility/ subdirectory, and
the
> other files into a browser/ subdirectory? That would match the layout for
other
> components that are split across multiple processes.
> 
> > Something else to consider is that in the mid-future, another change will
> likely
> > happen where the utility process is split into Mojo "apps" and the utility
> > process as a dumping ground goes away. This is longer term, but something to
> > keep in mind when considering layering and directory structure.
> 

I've made the change for this latest patchset. However, I think it's somewhat
invasive and not worth it. I'd prefer to revert the browser/ change and keep the
utility/ side since that's quite small and isolated.

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
On 2016/04/18 05:06:45, Anand Mistry wrote:
> On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> > On 2016/04/14 00:59:49, Anand Mistry wrote:
> > > On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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?
> 
> Because inside the utility process, the implementation no longer calls
> ReleaseProcessIfNeeded() when done.
> 
> > 
> > > 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.
> > 
> > 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?
> 
> 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.
> 
> > Perhaps even more worryingly, how do we prevent double frees?
> 
> WeakPtr<>. As long as we're not re-entering the UtilityProcessHostImpl's
> destructor (which doesn't happen), deleting WeakPtr<UtilityProcessHost>.get()
is
> correct behavior.
> 
> > 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.
> 
> UtilityProcessHostImpl owns its BrowserChildProcessHostImpl via a unique_ptr.
> Deleting the former will delete the latter without trying to delete the
> UtilityProcessHostImpl.

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.

> > 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.
> 
> 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...)

Oh, right. I didn't think to check ChildProcessLauncher, because I assumed it
would only deal with _launching_ the process... ☺

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_mojo_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_mojo_impl.cc:5: #include
"components/safe_json/safe_json_parser_mojo_impl.h"
On 2016/04/18 05:06:45, Anand Mistry wrote:
> On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> > On 2016/04/14 00:59:49, Anand Mistry wrote:
> > > On 2016/04/13 15:49:08, Bernhard Bauer wrote:
> > > > Technically, we should split this up into a browser part and a utility
> part,
> > > no?
> > > > We got around that somewhat by having this in chrome/utility previously
> > (which
> > > > admittedly was kind of wrong), but I would be a bit worried about just
> > putting
> > > > utility and browser parts into the same directory now.
> > > 
> > > Well, the utility-side message filter was still in this directory (and the
> > > wrapper was a layering quirk) so what I have now is no different than
> before.
> > > Also, build rules keep the two sides separate.
> > 
> > Sure, they get built as different targets, but there is nothing keeping the
> code
> > in this class from including e.g. content/public/browser/browser_thread.h.
> > 
> > > This situation exists in quite a few places with Mojo and there isn't a
> great
> > > solution for it yet. The only solid pattern we have so far is where the
> .mojom
> > > files live (public/interfaces). One pattern is to have "client libraries"
> > > (SafeJsonParserImpl in this case) in public/ or public/cpp/, but that
isn't
> > > followed in many places. If you have any ideas, I'd be happy to hear them.
> > 
> > Could we just move the _mojo_impl.* files into a utility/ subdirectory, and
> the
> > other files into a browser/ subdirectory? That would match the layout for
> other
> > components that are split across multiple processes.
> > 
> > > Something else to consider is that in the mid-future, another change will
> > likely
> > > happen where the utility process is split into Mojo "apps" and the utility
> > > process as a dumping ground goes away. This is longer term, but something
to
> > > keep in mind when considering layering and directory structure.
> > 
> 
> I've made the change for this latest patchset. However, I think it's somewhat
> invasive and not worth it. I'd prefer to revert the browser/ change and keep
the
> utility/ side since that's quite small and isolated.

OK, fair enough. It is a rather big change, and initially I didn't see that the
handler already is in the same directory as the browser side today, so this
isn't actually a regression. It might still be a good thing to do though (in a
follow up, not necessarily for you).

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

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/80001/components/safe_json/sa...
components/safe_json/safe_json_parser_impl.cc:68: // Shut down the utility
process if it's still running.
On 2016/04/20 16:43:15, Bernhard Bauer wrote:
> On 2016/04/18 05:06:45, Anand Mistry wrote:
> > On 2016/04/14 14:11:38, Bernhard (OOO until Apr 18) wrote:
> > > On 2016/04/14 00:59:49, Anand Mistry wrote:
> > > > On 2016/04/13 15:46:51, Bernhard Bauer wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > Because inside the utility process, the implementation no longer calls
> > ReleaseProcessIfNeeded() when done.
> > 
> > > 
> > > > 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.
> > > 
> > > 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?
> > 
> > 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.
> > 
> > > Perhaps even more worryingly, how do we prevent double frees?
> > 
> > WeakPtr<>. As long as we're not re-entering the UtilityProcessHostImpl's
> > destructor (which doesn't happen), deleting
WeakPtr<UtilityProcessHost>.get()
> is
> > correct behavior.
> > 
> > > 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.
> > 
> > UtilityProcessHostImpl owns its BrowserChildProcessHostImpl via a
unique_ptr.
> > Deleting the former will delete the latter without trying to delete the
> > UtilityProcessHostImpl.
> 
> 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 ☺

The good thing about UtilityProcessHost is that it call all it's delegate
functions via a post-task, so no reentrency.

> 
> 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.

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 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.

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.

> 
> > > 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.
> > 
> > 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...)
> 
> 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-27 22:36:29]

Ping!

I know I still need other reviewers for the other changes, but I'd like to get
sign-off on safe_json before proceeding.

[Bernhard Bauer, 2016-04-28 16:12:41]

On 2016/04/27 22:36:29, Anand Mistry wrote:
> Ping!
> 
> I know I still need other reviewers for the other changes, but I'd like to get
> sign-off on safe_json before proceeding.

Eep, sorry for the delay!

LGTM, but it would be great to add a TODO or something to sort out this whole
UtilityProcessHost mess (whether with mojo apps or not).

https://codereview.chromium.org/1861573002/diff/180001/components/safe_json/s...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/180001/components/safe_json/s...
components/safe_json/safe_json_parser_impl.cc:70: // |service_| may have to last
reference and destroying those might delete
Nit: "the" last reference

[Anand Mistry (off Chromium), 2016-05-02 06:43:09]

amistry@chromium.org changed reviewers:
+ sdefresne@chromium.org, thakis@chromium.org

[Anand Mistry (off Chromium), 2016-05-02 06:43:10]

thakis@chromium.org: Please review changes in chrome/

sdefresne@chromium.org: Please review changes in components/typemaps.gni

[Anand Mistry (off Chromium), 2016-05-02 06:44:33]

Added a TODO.

https://codereview.chromium.org/1861573002/diff/180001/components/safe_json/s...
File components/safe_json/safe_json_parser_impl.cc (right):

https://codereview.chromium.org/1861573002/diff/180001/components/safe_json/s...
components/safe_json/safe_json_parser_impl.cc:70: // |service_| may have to last
reference and destroying those might delete
On 2016/04/28 16:12:41, Bernhard Bauer wrote:
> Nit: "the" last reference

Done.

[sdefresne, 2016-05-02 08:01:28]

rs lgtm

[Anand Mistry (off Chromium), 2016-05-02 11:22:41]

amistry@chromium.org changed reviewers:
+ tsepez@chromium.org

[Anand Mistry (off Chromium), 2016-05-02 11:22:42]

tsepez: Because of IPC message removal and mojom addition.

[Tom Sepez, 2016-05-02 15:51:53]

https://codereview.chromium.org/1861573002/diff/220001/components/safe_json/s...
File components/safe_json/safe_json_parser_messages.h (left):

https://codereview.chromium.org/1861573002/diff/220001/components/safe_json/s...
components/safe_json/safe_json_parser_messages.h:14: #define IPC_MESSAGE_START
SafeJsonParserMsgStart
We can remove SafeJsonParserMsgStart definition from ipc/ipc_message_start.h

[Anand Mistry (off Chromium), 2016-05-03 02:30:22]

https://codereview.chromium.org/1861573002/diff/220001/components/safe_json/s...
File components/safe_json/safe_json_parser_messages.h (left):

https://codereview.chromium.org/1861573002/diff/220001/components/safe_json/s...
components/safe_json/safe_json_parser_messages.h:14: #define IPC_MESSAGE_START
SafeJsonParserMsgStart
On 2016/05/02 15:51:53, Tom Sepez wrote:
> We can remove SafeJsonParserMsgStart definition from ipc/ipc_message_start.h

Yes. Yes we can.

[Nico, 2016-05-03 13:54:04]

chrome lgtm

[Tom Sepez, 2016-05-03 21:46:13]

lgtm

[Anand Mistry (off Chromium), 2016-05-04 04:57:19]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-05-04 04:57:20]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1861573002/#ps240001
(title: "Remove from IPC message start.")

[commit-bot: I haz the power, 2016-05-04 04:57:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861573002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861573002/240001

[commit-bot: I haz the power, 2016-05-04 05:21:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-04 05:21:22]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Anand Mistry (off Chromium), 2016-05-04 05:47:58]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-05-04 05:48:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1861573002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1861573002/240001

[commit-bot: I haz the power, 2016-05-04 08:55:34]

Description was changed from

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

to

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

[commit-bot: I haz the power, 2016-05-04 08:55:36]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-04 08:57:20]

Description was changed from

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124
==========

to

==========
Convert the utility process JSON parser into a Mojo service.

BUG=597124

Committed: https://crrev.com/f8ce981e73207dfb46f6ccbfe2cd29378b3c4baf
Cr-Commit-Position: refs/heads/master@{#391463}
==========

[commit-bot: I haz the power, 2016-05-04 08:57:21]

Patchset 13 (id:??) landed as
https://crrev.com/f8ce981e73207dfb46f6ccbfe2cd29378b3c4baf
Cr-Commit-Position: refs/heads/master@{#391463}

[Mathieu, 2016-05-04 12:12:19]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/1951863002/ by mathp@chromium.org.

The reason for reverting is: broke browsertest:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....