Introduce RenderProcessHostObserver, use it in its first client.

Introduce RenderProcessHostObserver, use it in its first client.

BUG=170921
TEST=everything still works

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235877

[Avi (use Gerrit), 2013-11-14 02:48:27]

WDYT? Do we want to do the auto deregister magic that WCO does?

[Avi (use Gerrit), 2013-11-14 03:32:26]

https://codereview.chromium.org/72203003/diff/1/content/browser/site_instance...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/72203003/diff/1/content/browser/site_instance...
content/browser/site_instance_impl.cc:131: process_->AddObserver(this);
Yeah, that's bogus. I'll fix that tomorrow.

[jam, 2013-11-14 18:46:59]

a few comments:

-do we know for sure if the interface will have multiple methods? i.e. if it's
one method only that's needed, then we can use a callback
-do we really need HasObserver? is that what you meant by the comment above?
-to be clear, we know for sure we'll need this outside content?

[Avi (use Gerrit), 2013-11-14 19:04:07]

On 2013/11/14 18:46:59, jam wrote:
> -do we know for sure if the interface will have multiple methods?

Absolutely.

I haven't figured out how to do "RenderProcessHost created" but there will be
three more callbacks:
- render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)
- render process will exit (≅NOTIFICATION_RENDERER_PROCESS_CLOSING)
- render process did exit (≅NOTIFICATION_RENDERER_PROCESS_CLOSED)
in addition to RenderProcessHostDestroyed
(≅NOTIFICATION_RENDERER_PROCESS_TERMINATED)

> -do we really need HasObserver? is that what you meant by the comment above?

We don't need it. I removed it in the latest patchset, but I started an upload a
few hours ago and forgot to give it a name :( It's uploaded now.

> -to be clear, we know for sure we'll need this outside content?

Yes, absolutely. Here's my plan:

Right now, RenderProcessHostImpl::Init calls
ContentBrowserClient::RenderProcessHostCreated. For users inside content, I want
to have a CallbackList so that they can register for all RenderProcessHostImpls
created. (Since this is inside content, this isn't a layering issue.) For users
outside content, I haven't fully figured it out yet. The cheap way out is to
just hook into ContentBrowserClient::RenderProcessHostCreated. I want to be more
clever, though, so that's TBD.

In either case, users discover newly created RenderProcessHost. If they care
about processes coming and going, they register as an observer for a single
object.

That's why I'm passing the RenderProcessHost as the parameter, as I expect for
most cases that the object will observe multiple RenderProcessHosts.

My question to you, do you want to automate RenderProcessHostObserver? What that
means would be that Add/RemoveObserver would move to the Impl, each
RenderProcessHostObserver would keep a std::set of RenderProcessHosts it's
watching, and it would automatically deregister if a RenderProcessHost went away
or if it goes away. Pros: -Automatic! Cons: -heavier-weight, allocates a
std::set always.

[Avi (use Gerrit), 2013-11-14 20:27:56]

This alternate approach is at http://crrev.com/73103002 . Notice that the user
of RenderProcessHostObserver is simpler, but we pay the costs inside the
observer itself.

[jam, 2013-11-15 02:42:33]

lgtm

On 2013/11/14 19:04:07, Avi wrote:
> On 2013/11/14 18:46:59, jam wrote:
> > -do we know for sure if the interface will have multiple methods?
> 
> Absolutely.
> 
> I haven't figured out how to do "RenderProcessHost created" but there will be
> three more callbacks:
> - render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)

to be clear, this won't be on this interface?

> - render process will exit (≅NOTIFICATION_RENDERER_PROCESS_CLOSING)
> - render process did exit (≅NOTIFICATION_RENDERER_PROCESS_CLOSED)
> in addition to RenderProcessHostDestroyed
> (≅NOTIFICATION_RENDERER_PROCESS_TERMINATED)
> 
> > -do we really need HasObserver? is that what you meant by the comment above?
> 
> We don't need it. I removed it in the latest patchset, but I started an upload
a
> few hours ago and forgot to give it a name :( It's uploaded now.
> 
> > -to be clear, we know for sure we'll need this outside content?
> 
> Yes, absolutely. Here's my plan:
> 
> Right now, RenderProcessHostImpl::Init calls
> ContentBrowserClient::RenderProcessHostCreated. For users inside content, I
want
> to have a CallbackList so that they can register for all
RenderProcessHostImpls
> created. (Since this is inside content, this isn't a layering issue.)

how many places will do this? We can also just have RPH's constructor call out
to these places directly.. 

> For users
> outside content, I haven't fully figured it out yet. The cheap way out is to
> just hook into ContentBrowserClient::RenderProcessHostCreated. I want to be
more
> clever, though, so that's TBD.

I wonder if code in chrome can do this after creating a WC? i.e. foo.cc creates
a WC, and right after it adds a RPHObserver to its process.

> 
> In either case, users discover newly created RenderProcessHost. If they care
> about processes coming and going, they register as an observer for a single
> object.
> 
> That's why I'm passing the RenderProcessHost as the parameter, as I expect for
> most cases that the object will observe multiple RenderProcessHosts.
> 
> My question to you, do you want to automate RenderProcessHostObserver? What
that
> means would be that Add/RemoveObserver would move to the Impl, each
> RenderProcessHostObserver would keep a std::set of RenderProcessHosts it's
> watching, and it would automatically deregister if a RenderProcessHost went
away
> or if it goes away. Pros: -Automatic! Cons: -heavier-weight, allocates a
> std::set always.

what do most observers look like now? are these per-profile classes that will
outlive all RPHs? If so i'm inclined not to put the boilerplate in the
interface.

https://codereview.chromium.org/72203003/diff/60001/content/browser/site_inst...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/72203003/diff/60001/content/browser/site_inst...
content/browser/site_instance_impl.cc:331: process_ = NULL;
nit: really you only need this line and not the first two right? The DCHECK is
unnecessary because this class only watches one RPH. The RemoveObserver isn't
necessary because RPHDestroyed should be the last notification that's sent out

[Avi (use Gerrit), 2013-11-15 04:22:06]

On 2013/11/15 02:42:33, jam wrote:
> > - render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)
> to be clear, this won't be on this interface?

Eeep, you are correct, no it will not.

> how many places will do this? We can also just have RPH's constructor call out
> to these places directly.. 

Unknown. More research is needed.

> I wonder if code in chrome can do this after creating a WC? i.e. foo.cc
creates
> a WC, and right after it adds a RPHObserver to its process.

WebContents switch RPHs as they browse around sites, so that's not going to
work.

> what do most observers look like now? are these per-profile classes that will
> outlive all RPHs? If so i'm inclined not to put the boilerplate in the
> interface.

Most of them are; they're singletons that essentially live forever. OK, gotcha.

> The DCHECK is
> unnecessary because this class only watches one RPH.

We're this paranoid in lots of other places; how many times is
NotificationObserver::Observe overridden with a function that DCHECKS that the
sole notification that was registered was the one that fired? If it bothers you
that much then I'll take it out.

> The RemoveObserver isn't
> necessary because RPHDestroyed should be the last notification that's sent out

True, though I'm being paranoid. :)

About your LG, I'm going to have to hack at this a bit more to get the tests
passing. I'll let you know if there's a significant enough change to warrant
another peek.

[Avi (use Gerrit), 2013-11-15 05:40:08]

On 2013/11/15 04:22:06, Avi wrote:
> On 2013/11/15 02:42:33, jam wrote:
> > > - render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)
> > to be clear, this won't be on this interface?
> 
> Eeep, you are correct, no it will not.

No, I was right.

That is the notification that the render process was created. I need to figure
out how to signal that the RenderProcessHost was created, but once someone is
watching, they'll want to know the process lifecycles.

[jam, 2013-11-15 08:21:44]

On 2013/11/15 05:40:08, Avi wrote:
> On 2013/11/15 04:22:06, Avi wrote:
> > On 2013/11/15 02:42:33, jam wrote:
> > > > - render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)
> > > to be clear, this won't be on this interface?
> > 
> > Eeep, you are correct, no it will not.
> 
> No, I was right.
> 
> That is the notification that the render process was created. I need to figure
> out how to signal that the RenderProcessHost was created, but once someone is
> watching, they'll want to know the process lifecycles.

ah, got it. the existing notification's name was confusing. Perhaps that one
could be something like ProcessLaunched?


On 2013/11/15 04:22:06, Avi wrote:
> On 2013/11/15 02:42:33, jam wrote:
> > > - render process created (≅NOTIFICATION_RENDERER_PROCESS_CREATED)
> > to be clear, this won't be on this interface?
> 
> Eeep, you are correct, no it will not.
> 
> > how many places will do this? We can also just have RPH's constructor call
out
> > to these places directly.. 
> 
> Unknown. More research is needed.
> 
> > I wonder if code in chrome can do this after creating a WC? i.e. foo.cc
> creates
> > a WC, and right after it adds a RPHObserver to its process.
> 
> WebContents switch RPHs as they browse around sites, so that's not going to
> work.
> 
> > what do most observers look like now? are these per-profile classes that
will
> > outlive all RPHs? If so i'm inclined not to put the boilerplate in the
> > interface.
> 
> Most of them are; they're singletons that essentially live forever. OK,
gotcha.
> 
> > The DCHECK is
> > unnecessary because this class only watches one RPH.
> 
> We're this paranoid in lots of other places; how many times is
> NotificationObserver::Observe overridden with a function that DCHECKS that the
> sole notification that was registered was the one that fired? If it bothers
you
> that much then I'll take it out.

I always thought those notifications were unnecessary as they're testing
notification service. but up to you if you want to keep them.

> 
> > The RemoveObserver isn't
> > necessary because RPHDestroyed should be the last notification that's sent
out
> 
> True, though I'm being paranoid. :)
> 
> About your LG, I'm going to have to hack at this a bit more to get the tests
> passing. I'll let you know if there's a significant enough change to warrant
> another peek.

[Avi (use Gerrit), 2013-11-15 15:13:37]

On 2013/11/15 08:21:44, jam wrote:
> ah, got it. the existing notification's name was confusing. Perhaps that one
> could be something like ProcessLaunched?

I'm going to use the four names that I was using in the other CL:

  // This method is invoked when a new render process is created.
  virtual void RenderProcessCreated(RenderProcessHost* host) {}

  // This method is invoked when a render process will no longer be used for
  // further navigations and will exit soon.
  virtual void RenderProcessWillExit(RenderProcessHost* host) {}

  // This method is invoked when a render process exited (either normally or
  // with a crash). To determine if the process closed normally or crashed,
  // examine the |status| parameter.
  virtual void RenderProcessExited(RenderProcessHost* host,
                                   base::ProcessHandle handle,
                                   base::TerminationStatus status,
                                   int exit_code) {}

  // This method is invoked when a RenderProcessHost itself is destroyed. It is
  // extremely unlikely that you will need to use this; normal code should only
  // need to know if render processes exited with RenderProcessExited, above.
  virtual void RenderProcessHostDestroyed(RenderProcessHost* host) {}

-------

> > > are these per-profile classes that will
> > > outlive all RPHs? If so i'm inclined not to put the boilerplate in the
> > > interface.
> > Most of them are; they're singletons that essentially live forever. OK,
gotcha.

Actually, the problem here is that I don't know. In general I believe that these
classes outlive all the RPHs, but this is pre-existing code and I don't Know For
Sure. Does that change the answer?

[jam, 2013-11-15 16:32:59]

On 2013/11/15 15:13:37, Avi wrote:
> On 2013/11/15 08:21:44, jam wrote:
> > ah, got it. the existing notification's name was confusing. Perhaps that one
> > could be something like ProcessLaunched?
> 
> I'm going to use the four names that I was using in the other CL:
> 
>   // This method is invoked when a new render process is created.
>   virtual void RenderProcessCreated(RenderProcessHost* host) {}

I find this name confusing. the subtlety of RPCreated vs CBC::RPHCreated is easy
to miss.
personally I would find this easier to understand if it said something related
to launching. For comparison, see BrowserChildProcessHost::OnProcessLaunched()

> 
>   // This method is invoked when a render process will no longer be used for
>   // further navigations and will exit soon.
>   virtual void RenderProcessWillExit(RenderProcessHost* host) {}
> 
>   // This method is invoked when a render process exited (either normally or
>   // with a crash). To determine if the process closed normally or crashed,
>   // examine the |status| parameter.
>   virtual void RenderProcessExited(RenderProcessHost* host,
>                                    base::ProcessHandle handle,
>                                    base::TerminationStatus status,
>                                    int exit_code) {}
> 
>   // This method is invoked when a RenderProcessHost itself is destroyed. It
is
>   // extremely unlikely that you will need to use this; normal code should
only
>   // need to know if render processes exited with RenderProcessExited, above.
>   virtual void RenderProcessHostDestroyed(RenderProcessHost* host) {}
> 
> -------
> 
> > > > are these per-profile classes that will
> > > > outlive all RPHs? If so i'm inclined not to put the boilerplate in the
> > > > interface.
> > > Most of them are; they're singletons that essentially live forever. OK,
> gotcha.
> 
> Actually, the problem here is that I don't know. In general I believe that
these
> classes outlive all the RPHs, but this is pre-existing code and I don't Know
For
> Sure. Does that change the answer?

Let's go with the simpler approach in this cl, and we can revisit later if we
find we need automatic unregistration.

[Avi (use Gerrit), 2013-11-15 16:36:39]

On 2013/11/15 16:32:59, jam wrote:
> to launching. For comparison, see BrowserChildProcessHost::OnProcessLaunched()

Once it's needed, I'm happy with calling it RenderProcessLaunched. You bring up
BrowserChildProcessHost, but the closest analog is BrowserChildProcessObserver,
which has BrowserChildProcessHostConnected... :\ Connected?

[Avi (use Gerrit), 2013-11-15 20:45:41]

New version up. This looks like it's pretty green across the tests.

One change that's simple is giving MockRenderProcessHost observers. I doubt that
is controversial.

The other is the deletion callback; search for deletion_callback_called_. It
turns out that the existing code, both regular and mock, didn't always bother to
inform of its deletion. That's kind of important to do if you have callbacks, so
I had to fix that. Please take another glance.

[jam, 2013-11-15 22:42:23]

https://codereview.chromium.org/72203003/diff/290001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/72203003/diff/290001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1384:
RenderProcessHostDestroyed(this));
why isn't this enough, i.e. why do we need to do this in the destructor as well?

[Avi (use Gerrit), 2013-11-18 18:44:38]

OK, this should be good.

[jam, 2013-11-18 19:16:19]

lgtm

https://codereview.chromium.org/72203003/diff/600001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/72203003/diff/600001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1545: switch
(g_all_hosts.Pointer()->size()) {
nit: isn't size() always == 1? can you just make this a check instead of the
switch statement?

https://codereview.chromium.org/72203003/diff/600001/content/public/browser/r...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/72203003/diff/600001/content/public/browser/r...
content/public/browser/render_process_host.h:229: static void
ShutDownInProcessRenderer();
nit: since this is only called inside content, put this on
RenderProcessHostImpl. then you can get rid of the "friend" in
render_process_host_impl.h

[Avi (use Gerrit), 2013-11-18 19:22:20]

https://codereview.chromium.org/72203003/diff/600001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/72203003/diff/600001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:1545: switch
(g_all_hosts.Pointer()->size()) {
On 2013/11/18 19:16:20, jam wrote:
> isn't size() always == 1?

Is it? It's valid to shut down even if there never was a renderer, right?

https://codereview.chromium.org/72203003/diff/600001/content/public/browser/r...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/72203003/diff/600001/content/public/browser/r...
content/public/browser/render_process_host.h:229: static void
ShutDownInProcessRenderer();
On 2013/11/18 19:16:20, jam wrote:
> nit: since this is only called inside content, put this on
> RenderProcessHostImpl. then you can get rid of the "friend" in
> render_process_host_impl.h

Right. I was thinking BrowserMainLoop was in chrome/.

[commit-bot: I haz the power, 2013-11-18 21:02:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/72203003/350002

[commit-bot: I haz the power, 2013-11-19 01:35:58]

Change committed as 235877