Add renderer-side client for LeakDetectorRemoteController

Add renderer-side client for LeakDetectorRemoteController

LeakDetectorRemoteController is a Mojo service that was introduced in
http://crrev.com/2030883004. It provides an interface for a client on
another process to initialize metrics::LeakDetector and send back memory
leak reports.

This patch implements such a client on the renderer process.

BUG=chromium:615223

[Simon Que, 2016-06-10 00:52:24]

Ilya, this is the corresponding CL for the renderer process (see
https://codereview.chromium.org/2030883004/). Do you think I should create a new
dir chrome/renderer/leak_detector/ and link leak detector OWNERS to it, just
like I did in the other CL?

[Ilya Sherman, 2016-06-10 21:21:20]

On 2016/06/10 00:52:24, Simon Que wrote:
> Ilya, this is the corresponding CL for the renderer process (see
> https://codereview.chromium.org/2030883004/). Do you think I should create a
new
> dir chrome/renderer/leak_detector/ and link leak detector OWNERS to it, just
> like I did in the other CL?

Yes, I do =)

[Ilya Sherman, 2016-06-10 21:27:34]

isherman@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2016-06-10 21:27:35]

FWIW, I'm not very well versed in Mojo, so it would be nice to have an
additional reviewer who is.

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/BUILD.gn#ne...
chrome/renderer/BUILD.gn:199: ]
I might have an oversimplified understanding of this, but I think that anything
that's #included in a header file should go in public_deps rather than deps. 
Otherwise, targets that import the header file will fail to compile in a
component build, because they'll be missing the needed dependencies to resolve
the includes.

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:21: base::Unretained(this)));
Why is base::Unretained safe?  Is it because |this| owns the remote_service_,
and therefore the service will be destroyed before |this| is?  If so, and this
is a common pattern, then it's probably fine not to document.  Otherwise, it
would be nice to document.

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:29: return;
Should this be some sort of DCHECK that either the result is empty, or
parseable?

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:32: detector->AddObserver(this);
Should you unregister as an observer somewhere?

[Simon Que, 2016-06-16 07:13:27]

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/BUILD.gn#ne...
chrome/renderer/BUILD.gn:199: ]
On 2016/06/10 21:27:35, Ilya Sherman wrote:
> I might have an oversimplified understanding of this, but I think that
anything
> that's #included in a header file should go in public_deps rather than deps. 
> Otherwise, targets that import the header file will fail to compile in a
> component build, because they'll be missing the needed dependencies to resolve
> the includes.

Done

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:21: base::Unretained(this)));
On 2016/06/10 21:27:35, Ilya Sherman wrote:
> Why is base::Unretained safe?  Is it because |this| owns the remote_service_,
> and therefore the service will be destroyed before |this| is?  If so, and this
> is a common pattern, then it's probably fine not to document.  Otherwise, it
> would be nice to document.

I'm actually not sure if this is the correct pattern. I found an example here
that binds a non-member function:
https://www.chromium.org/developers/design-documents/mojo#TOC-Responding-to-R...

However, I didn't find any clear examples of binding a class member function by
poking around the code base for a bit. I'll ask chromium-dev or mojo-dev about
this.

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:29: return;
On 2016/06/10 21:27:35, Ilya Sherman wrote:
> Should this be some sort of DCHECK that either the result is empty, or
> parseable?

Done. However, the interface might change to sending the protobuf fields
individually rather than as a serialized string.

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:32: detector->AddObserver(this);
On 2016/06/10 21:27:35, Ilya Sherman wrote:
> Should you unregister as an observer somewhere?

Done.

[Ilya Sherman, 2016-06-16 18:14:20]

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/leak_detect...
chrome/renderer/leak_detector_remote_client.cc:21: base::Unretained(this)));
On 2016/06/16 07:13:27, Simon Que wrote:
> On 2016/06/10 21:27:35, Ilya Sherman wrote:
> > Why is base::Unretained safe?  Is it because |this| owns the
remote_service_,
> > and therefore the service will be destroyed before |this| is?  If so, and
this
> > is a common pattern, then it's probably fine not to document.  Otherwise, it
> > would be nice to document.
> 
> I'm actually not sure if this is the correct pattern. I found an example here
> that binds a non-member function:
>
https://www.chromium.org/developers/design-documents/mojo#TOC-Responding-to-R...
> 
> However, I didn't find any clear examples of binding a class member function
by
> poking around the code base for a bit. I'll ask chromium-dev or mojo-dev about
> this.

Okay, sounds like the conclusion on that thread is that it's safe -- yay!  Since
both of us were not quite certain about it, though, it's probably useful to
document the reasoning here.  Maybe someday it'll become a common pattern that
doesn't need documentation, but for now it'll probably help.

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
chrome/renderer/leak_detector_remote_client.cc:25:
metrics::LeakDetector::GetInstance()->RemoveObserver(this);
Is it safe to call RemoveObserver() if AddObserver() was never called, i.e. if
OnParamsReceived() was never triggered?  (The most common pattern to solve this
problem is to assign a member variable within OnParamsReceived(), which
unregisters as an observer upon destruction.  That is, the Resource Acquisition
Is Initialization pattern.)

[Simon Que, 2016-06-17 01:30:38]

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
chrome/renderer/leak_detector_remote_client.cc:25:
metrics::LeakDetector::GetInstance()->RemoveObserver(this);
On 2016/06/16 18:14:20, Ilya Sherman wrote:
> Is it safe to call RemoveObserver() if AddObserver() was never called, i.e. if
> OnParamsReceived() was never triggered?  (The most common pattern to solve
this
> problem is to assign a member variable within OnParamsReceived(), which
> unregisters as an observer upon destruction.  That is, the Resource
Acquisition
> Is Initialization pattern.)

Yes, the observers are added to base::ObserverList, whose RemoveObserver() func
safely handles the case of an observer not being present in the list.

[Ilya Sherman, 2016-06-17 01:32:50]

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2056973002/diff/20001/chrome/renderer/leak_de...
chrome/renderer/leak_detector_remote_client.cc:25:
metrics::LeakDetector::GetInstance()->RemoveObserver(this);
On 2016/06/17 01:30:38, Simon Que wrote:
> On 2016/06/16 18:14:20, Ilya Sherman wrote:
> > Is it safe to call RemoveObserver() if AddObserver() was never called, i.e.
if
> > OnParamsReceived() was never triggered?  (The most common pattern to solve
> this
> > problem is to assign a member variable within OnParamsReceived(), which
> > unregisters as an observer upon destruction.  That is, the Resource
> Acquisition
> > Is Initialization pattern.)
> 
> Yes, the observers are added to base::ObserverList, whose RemoveObserver()
func
> safely handles the case of an observer not being present in the list.

Okay.  In that case, LGTM with the added documentation for use of
base::Unretained() above.

[Simon Que, 2016-06-17 01:33:56]

After discussion with wfh, I'm actually going to combine this with the browser
process-side CL: https://codereview.chromium.org/2064463002/