Add LeakDetectorRemoteController as a Mojo service

Add LeakDetectorRemoteController as a Mojo service

New class LeakDetectorRemoteController that implements the Mojo
interface LeakDetectorRemote. This class provides a bridge between the
browser and renderer processes. It also allows a single instance of
LeakDetectorController to provide leak detector params to the renderer
process and receive leak reports from the renderer process.

The renderer process client will be implemented in a future CL.

BUG=chromium:615223
Committed: https://crrev.com/23bcb38936fddb66a15c02315d05d62e529756d4
Cr-Commit-Position: refs/heads/master@{#398775}

[Simon Que, 2016-06-03 18:07:53]

sque@chromium.org changed reviewers:
+ asvitkine@chromium.org, jochen@chromium.org

[Simon Que, 2016-06-03 18:07:53]

See this presentation for the proposed overall design.
https://docs.google.com/a/chromium.org/presentation/d/1b3vPQasyVVaSx7r4Y84YYC...

[Simon Que, 2016-06-07 09:19:29]

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

[Simon Que, 2016-06-07 09:19:30]

Adding isherman since asvitkine is slow to respond.

Some background: this is meant to implement the interface I added in
https://codereview.chromium.org/2018983004/.

[Ilya Sherman, 2016-06-07 22:44:18]

On 2016/06/07 09:19:30, Simon Que wrote:
> Adding isherman since asvitkine is slow to respond.
> 
> Some background: this is meant to implement the interface I added in
> https://codereview.chromium.org/2018983004/.

I think it probably makes sense to house this code under
chrome/browser/metrics/leak_detector/, with the same set of owners as
components/metrics/leak_detector.  (In fact, it might make sense for one owners
file to reference the other.)

[Ilya Sherman, 2016-06-07 23:03:38]

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:581: deps += [ "//components/metrics:interfaces" ]
Should this be listed in public_deps?  It seems to be referenced within header
files.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.h (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:38: //
LeakDetectorRemoteController::LocalController
nit: Please remove this line and the blank one above it (i.e. remove lines 37
and 38)

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.cc (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.cc:20: const
LeakDetectorRemote::GetParamsCallback& cb) {
nit: s/cb/callback

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.cc:22: // should not
initialize anything if the params are empty.
Would it be reasonable to instead just DCHECK that a controller has been set by
this point?  Or is desirable for it to be possible for the controller to be
unset?

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.cc:56: }  // namespace
metrics
It looks like this code could live within //components rather than within
//chrome -- is that true?  If so, why did you decide to implement it under
//chrome rather than //components?

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:29: virtual void
GetParams(MemoryLeakReportProto_Params* params) const = 0;
Why does this function fill a pointer, rather than returning a value (either by
copy or by const reference)?

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:41: //
LeakDetectorRemote
nit: Please add a trailing colon, ':'

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:44: //
LeakDetectorRemote
nit: Please remove this line and the one above (lines 43-44)

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:47: static void
SetLocalControllerInstance(LocalController* controller) {
nit: Please use hacker_case

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:56: static
LocalController* controller_;
Optional nit: I think it's slightly preferable to have a global pointer defined
in the anonymous namespace in the implementation file.  It's effectively
equivalent, but a bit clearer about what is happening.

[Simon Que, 2016-06-08 01:14:18]

I'll rebase this on top of the other CL (create leak_detector/ dir) once that
lands.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:581: deps += [ "//components/metrics:interfaces" ]
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> Should this be listed in public_deps?  It seems to be referenced within header
> files.

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.h (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:38: //
LeakDetectorRemoteController::LocalController
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> nit: Please remove this line and the blank one above it (i.e. remove lines 37
> and 38)

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.cc (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.cc:20: const
LeakDetectorRemote::GetParamsCallback& cb) {
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> nit: s/cb/callback

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.cc:22: // should not
initialize anything if the params are empty.
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> Would it be reasonable to instead just DCHECK that a controller has been set
by
> this point?  Or is desirable for it to be possible for the controller to be
> unset?

The intended operation is that the LocalController instance is set, but this
code allows the remote client to call this service smoothly even if this weren't
set. The remote client will see that |params.sampling_rate()| == 0 and refrain
from initializing the leak detector on the renderer process.

This allows us to check in all this code without adding any variation
config-based conditionals. Consider the alternative: There is no
LeakDetectorController / LocalController instance. The remote renderer process
doesn't know which variation is being used. It calls GetParams() and then this
function crashes.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:29: virtual void
GetParams(MemoryLeakReportProto_Params* params) const = 0;
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> Why does this function fill a pointer, rather than returning a value (either
by
> copy or by const reference)?

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:41: //
LeakDetectorRemote
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> nit: Please add a trailing colon, ':'

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:44: //
LeakDetectorRemote
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> nit: Please remove this line and the one above (lines 43-44)

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:47: static void
SetLocalControllerInstance(LocalController* controller) {
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> nit: Please use hacker_case

Done.

https://codereview.chromium.org/2030883004/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:56: static
LocalController* controller_;
On 2016/06/07 23:03:38, Ilya Sherman wrote:
> Optional nit: I think it's slightly preferable to have a global pointer
defined
> in the anonymous namespace in the implementation file.  It's effectively
> equivalent, but a bit clearer about what is happening.

Done.

[Ilya Sherman, 2016-06-08 02:49:21]

LGTM % nits -- thanks.

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.h (right):

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:36:
MemoryLeakReportProto_Params GetParams() const override;
Optional nit: I think you can also write MemoryLeakReportProto::Params, which
IMO is a bit cleaner.

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:47: static void
set_local_controller_instance(LocalController* controller);
Now that this is moved to the .cc file, please restore the CamelCase.  Sorry, I
should have been clearer about the interaction between those two comments.

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:54: static
LocalController* controller_;
nit: Please remove this.

[Simon Que, 2016-06-08 06:30:06]

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_controller.h (right):

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:36:
MemoryLeakReportProto_Params GetParams() const override;
On 2016/06/08 02:49:21, Ilya Sherman wrote:
> Optional nit: I think you can also write MemoryLeakReportProto::Params, which
> IMO is a bit cleaner.

Done.

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:47: static void
set_local_controller_instance(LocalController* controller);
On 2016/06/08 02:49:21, Ilya Sherman wrote:
> Now that this is moved to the .cc file, please restore the CamelCase.  Sorry,
I
> should have been clearer about the interaction between those two comments.

Done.

https://codereview.chromium.org/2030883004/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:54: static
LocalController* controller_;
On 2016/06/08 02:49:21, Ilya Sherman wrote:
> nit: Please remove this.

Done.

[jochen (gone - plz use gerrit), 2016-06-08 15:01:11]

lgtm with nit

https://codereview.chromium.org/2030883004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:25: class
LocalController {
virtual dtor please

[Simon Que, 2016-06-08 22:31:13]

https://codereview.chromium.org/2030883004/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/leak_detector_remote_controller.h (right):

https://codereview.chromium.org/2030883004/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_remote_controller.h:25: class
LocalController {
On 2016/06/08 15:01:11, jochen wrote:
> virtual dtor please

Done.

[Simon Que, 2016-06-09 03:45:15]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-06-09 03:45:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2030883004/#ps100001
(title: "Rebased")

[commit-bot: I haz the power, 2016-06-09 03:45:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2030883004/100001

[commit-bot: I haz the power, 2016-06-09 03:57:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-09 03:58:28]

Description was changed from

==========
Add LeakDetectorRemoteController as a Mojo service

New class LeakDetectorRemoteController that implements the Mojo
interface LeakDetectorRemote. This class provides a bridge between the
browser and renderer processes. It also allows a single instance of
LeakDetectorController to provide leak detector params to the renderer
process and receive leak reports from the renderer process.

The renderer process client will be implemented in a future CL.

BUG=chromium:615223
==========

to

==========
Add LeakDetectorRemoteController as a Mojo service

New class LeakDetectorRemoteController that implements the Mojo
interface LeakDetectorRemote. This class provides a bridge between the
browser and renderer processes. It also allows a single instance of
LeakDetectorController to provide leak detector params to the renderer
process and receive leak reports from the renderer process.

The renderer process client will be implemented in a future CL.

BUG=chromium:615223

Committed: https://crrev.com/23bcb38936fddb66a15c02315d05d62e529756d4
Cr-Commit-Position: refs/heads/master@{#398775}
==========

[commit-bot: I haz the power, 2016-06-09 03:58:29]

Patchset 6 (id:??) landed as
https://crrev.com/23bcb38936fddb66a15c02315d05d62e529756d4
Cr-Commit-Position: refs/heads/master@{#398775}

[Ilya Sherman, 2016-06-10 21:29:54]

https://codereview.chromium.org/2030883004/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc
(right):

https://codereview.chromium.org/2030883004/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc:52: }
Are these leak reports coming from the renderer process?  If so, the data should
be treated as untrusted/possibly corrupt, and sanity-checked.  (For IPC
messages, all IPC message files were set to have security reviewers as owners,
to make sure that this sort of check did not get forgotten.  Is there a similar
mechanism to ensure that Mojo messages are reviewed by the security team?)

[dcheng, 2016-06-10 21:36:27]

On 2016/06/10 21:29:54, Ilya Sherman wrote:
>
https://codereview.chromium.org/2030883004/diff/100001/chrome/browser/metrics...
> File chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc
> (right):
> 
>
https://codereview.chromium.org/2030883004/diff/100001/chrome/browser/metrics...
> chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc:52: }
> Are these leak reports coming from the renderer process?  If so, the data
should
> be treated as untrusted/possibly corrupt, and sanity-checked.  (For IPC
> messages, all IPC message files were set to have security reviewers as owners,
> to make sure that this sort of check did not get forgotten.  Is there a
similar
> mechanism to ensure that Mojo messages are reviewed by the security team?)

I've been trying to make sure OWNERS rules get updated where needed, but we've
missed some. We're close to getting a global mechanism for this ready.
However, as for this particular change, there are no mojom changes so OWNERS
files would have caught it. In general, this is why mojom changes should not be
landed without the corresponding implementation change.

[dcheng, 2016-06-10 21:37:15]

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

[dcheng, 2016-06-10 21:37:16]

Btw, we should probably just revert this CL and send it through proper security
review. Thanks.

[Simon Que, 2016-06-10 21:51:45]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2062483002/ by sque@chromium.org.

The reason for reverting is: Need to re-submit mojom interface and
implementation in single CL, and get security review from IPC owners..