Mojo interface/service for Leak Detector on remote process

Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org

Committed: https://crrev.com/c21a6e92a02f064431b22aa52b136f4682778c06
Cr-Commit-Position: refs/heads/master@{#406466}

[Simon Que, 2016-06-11 03:40:59]

Description was changed from

==========
Mojo interface for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports in serialized proto
  format (strings) to the browser process, where they can be attached to
  the UMA protobuf.

Also added an implementation of the Mojo interface,
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
TEST=Build successfully
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports in serialized proto
  format (strings) to the browser process, where they can be attached to
  the UMA protobuf.

Also added an implementation of the Mojo interface,
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
TEST=Build successfully
R=wfh@chromium.org
==========

[Simon Que, 2016-06-13 13:55:19]

On 2016/06/11 03:41:08, Simon Que wrote:

Will: I'm thinking that I should add another function to the Mojo interface:
OnProcessShutdown(). This is so that the browser process can keep track of which
renderer processes have shut down. This will only be called by renderer
processes with leak detector enabled.

See this CL for the mechanism by which renderer processes will have leak
detector randomly enabled:
https://codereview.chromium.org/2062743002/

If the browser process can track which renderer processes have shut down, then
it will know whether it is safe to have a new renderer process run the leak
detector. This avoids the remote possibility that many renderer processes are
running the LD simultaneously, which can spike the CPU overhead.

Let me know what you think and I can add it to this CL.

[Simon Que, 2016-06-13 23:38:15]

Description was changed from

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports in serialized proto
  format (strings) to the browser process, where they can be attached to
  the UMA protobuf.

Also added an implementation of the Mojo interface,
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
TEST=Build successfully
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports in serialized proto
  format (strings) to the browser process, where they can be attached to
  the UMA protobuf.
- OnProcessShutdown(): Notifies the browser process that a remote
  process running LeakDetector is terminating.

Also added an implementation of the Mojo interface,
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
TEST=Build successfully
R=wfh@chromium.org
==========

[Will Harris, 2016-06-15 22:56:53]

I'm asking some other ipc reviewers whether it's okay to serialize a proto over
mojo, or whether you should be decomposing the proto into a mojo message and
using base mojo types...

https://codereview.chromium.org/2064463002/diff/80001/components/metrics/leak...
File components/metrics/leak_detector/OWNERS (right):

https://codereview.chromium.org/2064463002/diff/80001/components/metrics/leak...
components/metrics/leak_detector/OWNERS:6: per-file *.mojom=set noparent
I think this changing soon +dcheng

see https://codereview.chromium.org/2070483002/

[Will Harris, 2016-06-16 15:53:48]

You'll need to break out the proto into mojo fields.

see
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc

"don't implement your own serialization mechanism (std::vector<char>, protobufs)
on top of the Chrome IPC system. Break up your structs and use the primitives
provided by Chrome IPC."

[Simon Que, 2016-06-16 19:58:06]

On 2016/06/16 15:53:48, Will Harris wrote:
> You'll need to break out the proto into mojo fields.
> 
> see
> http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> 
> "don't implement your own serialization mechanism (std::vector<char>,
protobufs)
> on top of the Chrome IPC system. Break up your structs and use the primitives
> provided by Chrome IPC."

In that case, perhaps I should include the remote client code?
https://codereview.chromium.org/2056973002/

That way both the breaking down and re-composing of the protobuf fields can be
in the same CL, and thus be tested.

[Will Harris, 2016-06-16 20:00:55]

On 2016/06/16 19:58:06, Simon Que wrote:
> On 2016/06/16 15:53:48, Will Harris wrote:
> > You'll need to break out the proto into mojo fields.
> > 
> > see
> >
http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
> > 
> > "don't implement your own serialization mechanism (std::vector<char>,
> protobufs)
> > on top of the Chrome IPC system. Break up your structs and use the
primitives
> > provided by Chrome IPC."
> 
> In that case, perhaps I should include the remote client code?
> https://codereview.chromium.org/2056973002/
> 
> That way both the breaking down and re-composing of the protobuf fields can be
> in the same CL, and thus be tested.

yes, I agree. I haven't managed to find any examples of mojo test code though.
it would be in browser_tests though, since those are the multi process Chrome
tests.

[Simon Que, 2016-06-17 01:42:39]

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

[Simon Que, 2016-06-17 01:42:40]

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

https://codereview.chromium.org/2064463002/diff/120001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:190: deps += [
Ilya was thinking that this should be public_deps since the dependencies are
header files included in the header file here. See:
https://codereview.chromium.org/2056973002/diff/1/chrome/renderer/BUILD.gn

Will, is this correct? If so, I think we'd have to add a "public_deps = []" at
the start of this file and then a "public_deps += [ ... ]" since there is no
existing public_deps field.

[Simon Que, 2016-06-17 06:26:50]

Description was changed from

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports in serialized proto
  format (strings) to the browser process, where they can be attached to
  the UMA protobuf.
- OnProcessShutdown(): Notifies the browser process that a remote
  process running LeakDetector is terminating.

Also added an implementation of the Mojo interface,
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
TEST=Build successfully
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully
R=wfh@chromium.org
==========

[Simon Que, 2016-06-17 07:35:05]

Now I need to fill in the |process_type| field of the protobuf to indicate
whether it was a browser or a renderer process.

Ideally I'd like to have this filled in by LeakDetector itself. How can that
class determine whether it is running on browser or renderer process?

Alternatively we could have the higher level classes set the field. This is a
little trickier on the browser process, LeakDetectorController currently has no
way of knowing whether an incoming report is from the current (browser) process
or a remote process. We would have to refactor it to determine the source.

[Will Harris, 2016-06-17 17:53:40]

On 2016/06/17 07:35:05, Simon Que wrote:
> Now I need to fill in the |process_type| field of the protobuf to indicate
> whether it was a browser or a renderer process.
> 
> Ideally I'd like to have this filled in by LeakDetector itself. How can that
> class determine whether it is running on browser or renderer process?
> 
> Alternatively we could have the higher level classes set the field. This is a
> little trickier on the browser process, LeakDetectorController currently has
no
> way of knowing whether an incoming report is from the current (browser)
process
> or a remote process. We would have to refactor it to determine the source.

you can't trust the data from the renderer as it might just lie that it's a
browser process. perhaps you could consider passing the renderer a
renderer-specific mojo interface that it can call when it has leak reports, then
you can trust that mojo will enforce this.

[Ilya Sherman, 2016-06-17 22:18:20]

FYI, I'll be OOO for the next week, so you'll likely want to ask another metrics
team reviewer to look over any changes that Will doesn't cover.  Or, if you and
Will both agree that they're pretty trivial, feel free to TBR me.

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.h:36:
MemoryLeakReportProto::Params GetParams() override;
nit: Should this method be const?

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

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc:25: new
LeakDetectorRemoteController(std::move(request));
I really wish that Mojo didn't use a pattern that expects bare new's, without
any obvious ownership at the construction site :(

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

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
IMO it would be nice to immediately start out with a leak_detector/
subdirectory, in part so that you could move the BUILD configuration for the
leak_detector into there.

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/leak_d...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/leak_d...
chrome/renderer/leak_detector_remote_client.cc:19: // It is safe to use
base::Unretained(this). See example at:
nit: For the first sentence, I'd add ", because |this| owns the
|remote_service_|".

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/leak_d...
chrome/renderer/leak_detector_remote_client.cc:37:
result->call_stack_suspicion_threshold);
It's kind of a shame that Mojo can't pass protocol buffers directly.  Might be
worth filing a feature request for adding that support?

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/BUI...
components/metrics/BUILD.gn:317: if (is_chromeos) {
nit: Please combine the two ChromeOS stanzas.  Also, could all of the
leak-detector stuff live in a BUILD.gn file within the leak_detector directory?

[Ben Goodger (Google), 2016-06-17 22:22:01]

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

[Ben Goodger (Google), 2016-06-17 22:22:03]

¡¡mojom drive-by!!

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_remote.mojom (right):

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_remote.mojom:35: interface
LeakDetectorRemote {
Can we not use the *Remote suffix for something that might be in another
process? Most mojom interfaces are implemented in remote processes & we don't
use this convention for then.

[Simon Que, 2016-06-18 04:17:40]

On 2016/06/17 17:53:40, Will Harris wrote:
> On 2016/06/17 07:35:05, Simon Que wrote:
> > Now I need to fill in the |process_type| field of the protobuf to indicate
> > whether it was a browser or a renderer process.
> > 
> > Ideally I'd like to have this filled in by LeakDetector itself. How can that
> > class determine whether it is running on browser or renderer process?
> > 
> > Alternatively we could have the higher level classes set the field. This is
a
> > little trickier on the browser process, LeakDetectorController currently has
> no
> > way of knowing whether an incoming report is from the current (browser)
> process
> > or a remote process. We would have to refactor it to determine the source.
> 
> you can't trust the data from the renderer as it might just lie that it's a
> browser process. perhaps you could consider passing the renderer a
> renderer-specific mojo interface that it can call when it has leak reports,
then
> you can trust that mojo will enforce this.

Done. Went the refactoring route.

[Simon Que, 2016-06-18 04:17:53]

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.h:36:
MemoryLeakReportProto::Params GetParams() override;
On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> nit: Should this method be const?

No, in the next CL this will be used to determine which remote processes should
have leak detector enabled vs disabled (= empty params). This class will then
need to track how many remote processes have leak detector enabled. This is a
mechanism to limit the number of processes that are running leak detector at any
given time.

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

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> IMO it would be nice to immediately start out with a leak_detector/
> subdirectory, in part so that you could move the BUILD configuration for the
> leak_detector into there.

I can do that, but is it necessary for just two files?

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/leak_d...
File chrome/renderer/leak_detector_remote_client.cc (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/leak_d...
chrome/renderer/leak_detector_remote_client.cc:19: // It is safe to use
base::Unretained(this). See example at:
On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> nit: For the first sentence, I'd add ", because |this| owns the
> |remote_service_|".

Done.

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/BUI...
components/metrics/BUILD.gn:317: if (is_chromeos) {
On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> nit: Please combine the two ChromeOS stanzas.  Also, could all of the
> leak-detector stuff live in a BUILD.gn file within the leak_detector
directory?

Done.

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_remote.mojom (right):

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_remote.mojom:35: interface
LeakDetectorRemote {
On 2016/06/17 22:22:03, Ben Goodger (Google) wrote:
> Can we not use the *Remote suffix for something that might be in another
> process? Most mojom interfaces are implemented in remote processes & we don't
> use this convention for then.

I can't use "LeakDetector" because that name already exists in the metrics
namespace.

How about "LeakDetectorInterface"?

[dcheng, 2016-06-18 06:17:07]

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

[dcheng, 2016-06-18 06:17:08]

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector_remote.mojom (right):

https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
components/metrics/leak_detector/leak_detector_remote.mojom:35: interface
LeakDetectorRemote {
On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> On 2016/06/17 22:22:03, Ben Goodger (Google) wrote:
> > Can we not use the *Remote suffix for something that might be in another
> > process? Most mojom interfaces are implemented in remote processes & we
don't
> > use this convention for then.
> 
> I can't use "LeakDetector" because that name already exists in the metrics
> namespace.
> 
> How about "LeakDetectorInterface"?

The general convention is to define mojo interfaces in a nested mojom namespace,
which prevents this collision. For an example, look at //url/mojo (which also
had examples of unit testing).

[Simon Que, 2016-06-27 07:02:50]

On 2016/06/18 06:17:08, dcheng wrote:
>
https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
> File components/metrics/leak_detector/leak_detector_remote.mojom (right):
> 
>
https://codereview.chromium.org/2064463002/diff/140001/components/metrics/lea...
> components/metrics/leak_detector/leak_detector_remote.mojom:35: interface
> LeakDetectorRemote {
> On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> > On 2016/06/17 22:22:03, Ben Goodger (Google) wrote:
> > > Can we not use the *Remote suffix for something that might be in another
> > > process? Most mojom interfaces are implemented in remote processes & we
> don't
> > > use this convention for then.
> > 
> > I can't use "LeakDetector" because that name already exists in the metrics
> > namespace.
> > 
> > How about "LeakDetectorInterface"?
> 
> The general convention is to define mojo interfaces in a nested mojom
namespace,
> which prevents this collision. For an example, look at //url/mojo (which also
> had examples of unit testing).

Moved to nested mojom namespace.

[Simon Que, 2016-06-27 16:02:12]

I looked in //url/mojo and //services/shell/tests... the tests there seemed to
be for the Mojo interfaces rather than for the implementations of those
interfaces. Is that correct? Is it possible to unit test the implementations
themselves?

[Ben Goodger (Google), 2016-06-27 16:52:31]

On 2016/06/27 16:02:12, Simon Que (Away June 20-24) wrote:
> I looked in //url/mojo and //services/shell/tests... the tests there seemed to
> be for the Mojo interfaces rather than for the implementations of those
> interfaces. Is that correct? Is it possible to unit test the implementations
> themselves?

For testing you have two options:

- if the implementation is sufficiently modular, there may be sub-components
that can be individually unit tested. For example, for a hypothetical UI
service, there may be a geometry library whose operations can be unit tested.
This fits the intent of unit testing well - testing well scoped features with
fast tests.
- if you find yourself wanting to test the implementation as it appears to the
consumer, I argue that abstracting the Mojo layer just adds complexity to the
implementation side to support stubbing the various Mojo bindings with mocks.
Whether or not you're using Mojo, I argue that this is "integration testing" &
you should be running the system as close to production as you can, or refactor
the impl to be more easily unit testable.

Concrete example of the pitfalls: We currently have unit tests in Chrome that
attempt to mock out parts of content. I think this is a result of poor layering
in the things being unit tested (e.g. my tabstrip model is one case - it manages
WebContents, so in its unit test I need to be able to construct WebContents
absent all the rest of content stuff that WebContents requires,  which results
in a unit test environment where some WebContents methods mysteriously crash -
making the code quite hard to evolve).

[Ilya Sherman, 2016-06-28 21:33:10]

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.h:36:
MemoryLeakReportProto::Params GetParams() override;
On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> > nit: Should this method be const?
> 
> No, in the next CL this will be used to determine which remote processes
should
> have leak detector enabled vs disabled (= empty params). This class will then
> need to track how many remote processes have leak detector enabled. This is a
> mechanism to limit the number of processes that are running leak detector at
any
> given time.

That's pretty surprising behavior for a method simply named "GetParams()".  I
would maybe recommending marking the method const for now, and updating it in
the next CL, including definitely more comments, and possibly a different name
altogether.

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

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> > IMO it would be nice to immediately start out with a leak_detector/
> > subdirectory, in part so that you could move the BUILD configuration for the
> > leak_detector into there.
> 
> I can do that, but is it necessary for just two files?

Necessary?  No.  But it does have some nice advantages, including the
possibility of splitting off a BUILD.gn file for the directory.

https://codereview.chromium.org/2064463002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right):

https://codereview.chromium.org/2064463002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.cc:109: *reports =
std::move(stored_reports_);
I'm pretty sure that you need to follow this up with some code that clears the
|stored_reports_| vector.  Otherwise, multiple calls to this method in a row
might give surprising results.

[Simon Que, 2016-06-29 02:30:47]

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.h (right):

https://codereview.chromium.org/2064463002/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.h:36:
MemoryLeakReportProto::Params GetParams() override;
On 2016/06/28 21:33:10, Ilya Sherman wrote:
> On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> > On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> > > nit: Should this method be const?
> > 
> > No, in the next CL this will be used to determine which remote processes
> should
> > have leak detector enabled vs disabled (= empty params). This class will
then
> > need to track how many remote processes have leak detector enabled. This is
a
> > mechanism to limit the number of processes that are running leak detector at
> any
> > given time.
> 
> That's pretty surprising behavior for a method simply named "GetParams()".  I
> would maybe recommending marking the method const for now, and updating it in
> the next CL, including definitely more comments, and possibly a different name
> altogether.

Done.

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

https://codereview.chromium.org/2064463002/diff/140001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
On 2016/06/28 21:33:10, Ilya Sherman wrote:
> On 2016/06/18 04:17:53, Simon Que (Away June 20-24) wrote:
> > On 2016/06/17 22:18:20, Ilya Sherman (Away June 18-26) wrote:
> > > IMO it would be nice to immediately start out with a leak_detector/
> > > subdirectory, in part so that you could move the BUILD configuration for
the
> > > leak_detector into there.
> > 
> > I can do that, but is it necessary for just two files?
> 
> Necessary?  No.  But it does have some nice advantages, including the
> possibility of splitting off a BUILD.gn file for the directory.

Done in Patch Set 9.

https://codereview.chromium.org/2064463002/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_controller.cc (right):

https://codereview.chromium.org/2064463002/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_controller.cc:109: *reports =
std::move(stored_reports_);
On 2016/06/28 21:33:10, Ilya Sherman wrote:
> I'm pretty sure that you need to follow this up with some code that clears the
> |stored_reports_| vector.  Otherwise, multiple calls to this method in a row
> might give surprising results.

Done.

[Ilya Sherman, 2016-06-29 20:35:50]

LGTM % file placement nit, which is really not all that important -- mostly just
including it for clarity.

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

https://codereview.chromium.org/2064463002/diff/220001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
I meant that this makes sense to pull into a separate BUILD.gn file --
especially once we drop the GYP reference (once GYP is fully deprecated).

[Simon Que, 2016-06-30 03:15:01]

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

https://codereview.chromium.org/2064463002/diff/220001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
On 2016/06/29 20:35:50, Ilya Sherman wrote:
> I meant that this makes sense to pull into a separate BUILD.gn file --
> especially once we drop the GYP reference (once GYP is fully deprecated).

Is that necessary for just one header/source pair? I could do it...

... also, revert the other BUILD.gn change under src/components?

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

On 2016/06/27 16:52:31, Ben Goodger (Google) wrote:
> On 2016/06/27 16:02:12, Simon Que (Away June 20-24) wrote:
> > I looked in //url/mojo and //services/shell/tests... the tests there seemed
to
> > be for the Mojo interfaces rather than for the implementations of those
> > interfaces. Is that correct? Is it possible to unit test the implementations
> > themselves?
> 
> For testing you have two options:
> 
> - if the implementation is sufficiently modular, there may be sub-components
> that can be individually unit tested. For example, for a hypothetical UI
> service, there may be a geometry library whose operations can be unit tested.
> This fits the intent of unit testing well - testing well scoped features with
> fast tests.
> - if you find yourself wanting to test the implementation as it appears to the
> consumer, I argue that abstracting the Mojo layer just adds complexity to the
> implementation side to support stubbing the various Mojo bindings with mocks.
> Whether or not you're using Mojo, I argue that this is "integration testing" &
> you should be running the system as close to production as you can, or
refactor
> the impl to be more easily unit testable.
> 
> Concrete example of the pitfalls: We currently have unit tests in Chrome that
> attempt to mock out parts of content. I think this is a result of poor
layering
> in the things being unit tested (e.g. my tabstrip model is one case - it
manages
> WebContents, so in its unit test I need to be able to construct WebContents
> absent all the rest of content stuff that WebContents requires,  which results
> in a unit test environment where some WebContents methods mysteriously crash -
> making the code quite hard to evolve).

I'd like to test out the conversion of the protobufs to Mojo structs and vice
versa, in leak_detector_remote_controller and leak_detector_remote_client. I
would consider that a unit test.

How about: I extract the conversion functions into a class in
src/components/metrics/leak_detector, alongside the .mojom? Then, this class can
be tested with a test mojo interface. This avoids directly testing the actual
mojo implementation. This class's functions can be called from
leak_detector_remote_controller and leak_detector_remote_client.

[Simon Que, 2016-07-13 01:25:01]

On 2016/06/30 06:08:28, Simon Que wrote:
> How about: I extract the conversion functions into a class in
> src/components/metrics/leak_detector, alongside the .mojom? Then, this class
can
> be tested with a test mojo interface. This avoids directly testing the actual
> mojo implementation. This class's functions can be called from
> leak_detector_remote_controller and leak_detector_remote_client.

Done. Already discussed this with wfh.

[Will Harris, 2016-07-14 00:05:30]

lgtm just edit the TEST= line as you now have tests.

[Will Harris, 2016-07-14 00:06:12]

you just need a chrome/ owner now

[Will Harris, 2016-07-14 00:15:17]

hmm looks like this needs a rebase, I think
https://codereview.chromium.org/2099063002 changed
chrome/browser/chrome_content_browser_client.cc

[Simon Que, 2016-07-15 17:07:59]

Patchset #15 (id:280001) has been deleted

[Simon Que, 2016-07-15 22:44:15]

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

https://codereview.chromium.org/2064463002/diff/220001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:194: }
On 2016/06/30 03:15:01, Simon Que wrote:
> On 2016/06/29 20:35:50, Ilya Sherman wrote:
> > I meant that this makes sense to pull into a separate BUILD.gn file --
> > especially once we drop the GYP reference (once GYP is fully deprecated).
> 
> Is that necessary for just one header/source pair? I could do it...
> 
> ... also, revert the other BUILD.gn change under src/components?

Done.

[Simon Que, 2016-07-15 22:47:24]

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

[Simon Que, 2016-07-15 22:47:25]

+jochen to review chrome/

[jochen (gone - plz use gerrit), 2016-07-18 11:40:55]

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

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc:26: new
LeakDetectorRemoteController(std::move(request));
who owns this?

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_remote_controller.h
(right):

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.h:54:
static void SetLocalControllerInstance(LocalController* controller);
where is this called from?

https://codereview.chromium.org/2064463002/diff/320001/components/metrics/lea...
File components/metrics/leak_detector/protobuf_to_mojo_converter.h (right):

https://codereview.chromium.org/2064463002/diff/320001/components/metrics/lea...
components/metrics/leak_detector/protobuf_to_mojo_converter.h:14: class
ProtobufToMojoConverter {
we prefer namespaces over classes with only static methods:
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

[Simon Que, 2016-07-18 18:50:21]

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

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.cc:26: new
LeakDetectorRemoteController(std::move(request));
On 2016/07/18 11:40:55, jochen wrote:
> who owns this?

The Mojo system calls this function and takes ownership of the created object,
in response to a connection request from LeakDetectorRemoteClient. See:
http://dev.chromium.org/developers/design-documents/mojo#TOC-Exposing-Service...

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector/leak_detector_remote_controller.h
(right):

https://codereview.chromium.org/2064463002/diff/320001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector/leak_detector_remote_controller.h:54:
static void SetLocalControllerInstance(LocalController* controller);
On 2016/07/18 11:40:55, jochen wrote:
> where is this called from?

Currently not being called. In a subsequent CL, this will be called from
chrome/browser/chrome_content_browser_client.cc. I also plan to add some code to
randomly enable leak detection on the remote process (vs 100% of the time), to
avoid overloading the system with overhead. Since there's already so much in
this CL, I'd like to save that for the next one.

https://codereview.chromium.org/2064463002/diff/320001/components/metrics/lea...
File components/metrics/leak_detector/protobuf_to_mojo_converter.h (right):

https://codereview.chromium.org/2064463002/diff/320001/components/metrics/lea...
components/metrics/leak_detector/protobuf_to_mojo_converter.h:14: class
ProtobufToMojoConverter {
On 2016/07/18 11:40:55, jochen wrote:
> we prefer namespaces over classes with only static methods:
>
https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_a...

Done.

[Simon Que, 2016-07-18 21:53:09]

Description was changed from

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org
==========

[jochen (gone - plz use gerrit), 2016-07-19 12:21:25]

lgtm

[Simon Que, 2016-07-19 17:56:30]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-07-19 17:56:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, wfh@chromium.org
Link to the patchset: https://codereview.chromium.org/2064463002/#ps340001
(title: "Turn ProtobufToMojoConverter into namespace; Fix path in
chrome_renderer.gypi")

[commit-bot: I haz the power, 2016-07-19 17:57:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 18:02:51]

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

[commit-bot: I haz the power, 2016-07-19 18:02:53]

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

[Simon Que, 2016-07-19 22:55:52]

The CQ bit was checked by sque@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 22:56:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 01:47:19]

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

[commit-bot: I haz the power, 2016-07-20 01:47:22]

Dry run: This issue passed the CQ dry run.

[Simon Que, 2016-07-20 02:19:01]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-07-20 02:19:02]

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

[commit-bot: I haz the power, 2016-07-20 02:19:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 02:24:34]

Description was changed from

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org
==========

[commit-bot: I haz the power, 2016-07-20 02:24:36]

Committed patchset #20 (id:400001)

[commit-bot: I haz the power, 2016-07-20 02:24:53]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 02:27:39]

Description was changed from

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org
==========

to

==========
Mojo interface/service for Leak Detector on remote process

This interface allows an instance of the Runtime Memory Leak Detector
(components/metrics/leak_detector) to run on a non-browser process, such
as a tab (renderer) process. The interface provides two function APIs:

- GetParams(): Returns the profiling parameters used for initializing
  LeakDetector.
- SendLeakReports(): Passes an array of leak reports to the browser process,
  where they can be attached to the UMA protobuf.

Also added an implementation of the Mojo interface,
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.

BUG=chromium:615223
TEST=Build successfully, unit tests pass
R=wfh@chromium.org

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

[commit-bot: I haz the power, 2016-07-20 02:27:41]

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

[achuithb, 2016-07-20 07:23:49]

On 2016/07/20 02:27:41, commit-bot: I haz the power wrote:
> Patchset 20 (id:??) landed as
> https://crrev.com/c21a6e92a02f064431b22aa52b136f4682778c06
> Cr-Commit-Position: refs/heads/master@{#406466}

I believe this CL needs to be reverted:
https://uberchromegw.corp.google.com/i/chromiumos.chromium/builders/x86-gener...

crbug.com/629760

[achuithb, 2016-07-20 07:24:48]

A revert of this CL (patchset #20 id:400001) has been created in
https://codereview.chromium.org/2166553003/ by achuith@chromium.org.

The reason for reverting is: crbug.com/629760.