metrics: Add leak detector controller in Chrome OS metrics system

metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests

Committed: https://crrev.com/82e4d1559b83487b7d75527a8d74bdb41b5f5472
Cr-Commit-Position: refs/heads/master@{#378011}

[Simon Que, 2016-02-10 04:25:03]

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

[Simon Que, 2016-02-10 04:25:04]

This patch is the workaround that I described to use in wfh's absence. It makes
more sense to check in the stub code under components/ in a separate CL, but I'm
including it here so you can see how LeakDetector, LeakDetectorController, and
ChromeOSMetricsProvider are connected. I can put the code under components/ in a
separate CL.

[Alexei Svitkine (slow), 2016-02-10 20:16:59]

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

https://codereview.chromium.org/1681263003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.cc:39:
stored_reports_.resize(stored_reports_.size() + 1);
I think this pattern results in bad growth (i.e. preventing the usual doubling
algorithm that STL has) - just do push_back().

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

https://codereview.chromium.org/1681263003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:18: 
Nit: No new line.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:25: return true;
If you always return true, why have a return value?

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:16: // The top level leak
detector is an interface layer that connects the allocator
I don't understand what "top level" means in this case. How about just
"LeakDetector is an interface layer that .."

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:21: // TODO(sque): Add the full
functionality and allow only one instance.
Nit: Put this whole comment above the class declaration.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:38: bool operator<(const
LeakReport& other) const {
Why is this needed?

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:94: std::list<Observer*>
observers_;
Consider using base/observer_list.h

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector_unittest.cc (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_unittest.cc:41: void SetUp()
override { detector_.reset(new LeakDetector); }
You can just put this in the ctor and avoid having SetUp() and TearDown()
methods.

A new instance of the Test object will be instantiated for each test. In fact,
you don't need to even make it a scoped_ptr - can just be a direct member.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_unittest.cc:48: protected:
private:

[Simon Que, 2016-02-11 01:45:38]

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

https://codereview.chromium.org/1681263003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.cc:39:
stored_reports_.resize(stored_reports_.size() + 1);
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> I think this pattern results in bad growth (i.e. preventing the usual doubling
> algorithm that STL has) - just do push_back().

Done.

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

https://codereview.chromium.org/1681263003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:18: 
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> Nit: No new line.

Done.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.cc:25: return true;
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> If you always return true, why have a return value?
> 

Changing them both to return void, esp in light of using ObserverList.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:16: // The top level leak
detector is an interface layer that connects the allocator
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> I don't understand what "top level" means in this case. How about just
> "LeakDetector is an interface layer that .."

Done.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:21: // TODO(sque): Add the full
functionality and allow only one instance.
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> Nit: Put this whole comment above the class declaration.

Done.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:38: bool operator<(const
LeakReport& other) const {
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> Why is this needed?

It's used in the unit test. I've moved it to there.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:94: std::list<Observer*>
observers_;
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> Consider using base/observer_list.h

Done.

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector_unittest.cc (right):

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_unittest.cc:41: void SetUp()
override { detector_.reset(new LeakDetector); }
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> You can just put this in the ctor and avoid having SetUp() and TearDown()
> methods.
> 
> A new instance of the Test object will be instantiated for each test. In fact,
> you don't need to even make it a scoped_ptr - can just be a direct member.

I actually call "detector_.reset()" in the second unit test.

In a future unit test, I will be instantiating the class with non-default
parameters. Making it a direct member doesn't work.

However, I can get rid of SetUp() and TearDown(). Note that in a future CL, I
plan to check the allocator hooks before and after each test, so I might still
need to have those functions (see http://crrev.com/1665553002/).

https://codereview.chromium.org/1681263003/diff/20001/components/metrics/leak...
components/metrics/leak_detector/leak_detector_unittest.cc:48: protected:
On 2016/02/10 20:16:58, Alexei Svitkine wrote:
> private:

Done.

[Alexei Svitkine (slow), 2016-02-12 16:17:42]

lgtm % remaining comments

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

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:23: // Stores a new leak
report in |stored_reports_|.
This comment should just be:

// LeakDetector::Observer:

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:24: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
Move this to private section.

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:86: void NotifyObservers(const
std::vector<LeakReport>& reports);
Does this need to be public? Kind of strange to have any public caller be able
to notify all the observers.

[Simon Que, 2016-02-12 19:00:07]

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

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:23: // Stores a new leak
report in |stored_reports_|.
On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> This comment should just be:
> 
> // LeakDetector::Observer:

Will make this change once I know what to do regarding the other comments.

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:24: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> Move this to private section.

What about the unit test? It calls this func. How about creating a derived class
that exposes these functions, like here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:86: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> Does this need to be public? Kind of strange to have any public caller be able
> to notify all the observers.

This will be called from the memory allocator hook function, which cannot be a
non-static class member. Either the hook func will have to be a static member of
this class, or this class will have to expose some interface to call this
function See here:
https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...

[Simon Que, 2016-02-17 01:55:43]

Ping. Waiting for input before proceeding.

[Alexei Svitkine (slow), 2016-02-17 16:06:30]

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

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:24: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
On 2016/02/12 19:00:07, Simon Que wrote:
> On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> > Move this to private section.
> 
> What about the unit test? It calls this func. How about creating a derived
class
> that exposes these functions, like here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...

Derived class that exposes these functions SGTM.

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:86: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/12 19:00:07, Simon Que wrote:
> On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> > Does this need to be public? Kind of strange to have any public caller be
able
> > to notify all the observers.
> 
> This will be called from the memory allocator hook function, which cannot be a
> non-static class member. Either the hook func will have to be a static member
of
> this class, or this class will have to expose some interface to call this
> function See here:
>
https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...

If having the hook be a static member of the class will work, then let's do
that.

However, how do you plan to for the hook to get at the LeakDetector instance to
call this on?

Regardless, I think it's fine to make it private for now and possibly move it in
the follow-up CL that adds the hook if necessary.

[Simon Que, 2016-02-17 20:49:33]

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

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:23: // Stores a new leak
report in |stored_reports_|.
On 2016/02/12 19:00:07, Simon Que wrote:
> On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> > This comment should just be:
> > 
> > // LeakDetector::Observer:
> 
> Will make this change once I know what to do regarding the other comments.

Done.

https://codereview.chromium.org/1681263003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/leak_detector_controller.h:24: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
On 2016/02/17 16:06:30, Alexei Svitkine wrote:
> On 2016/02/12 19:00:07, Simon Que wrote:
> > On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> > > Move this to private section.
> > 
> > What about the unit test? It calls this func. How about creating a derived
> class
> > that exposes these functions, like here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
> 
> Derived class that exposes these functions SGTM.

Done.

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/40001/components/metrics/leak...
components/metrics/leak_detector/leak_detector.h:86: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/17 16:06:30, Alexei Svitkine wrote:
> On 2016/02/12 19:00:07, Simon Que wrote:
> > On 2016/02/12 16:17:42, Alexei Svitkine wrote:
> > > Does this need to be public? Kind of strange to have any public caller be
> able
> > > to notify all the observers.
> > 
> > This will be called from the memory allocator hook function, which cannot be
a
> > non-static class member. Either the hook func will have to be a static
member
> of
> > this class, or this class will have to expose some interface to call this
> > function See here:
> >
>
https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...
> 
> If having the hook be a static member of the class will work, then let's do
> that.
> 
> However, how do you plan to for the hook to get at the LeakDetector instance
to
> call this on?
> 
> Regardless, I think it's fine to make it private for now and possibly move it
in
> the follow-up CL that adds the hook if necessary.

Done.

[Simon Que, 2016-02-17 20:52:26]

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

[Alexei Svitkine (slow), 2016-02-17 20:53:33]

lgtm

[Simon Que, 2016-02-17 21:08:00]

On 2016/02/17 20:53:33, Alexei Svitkine wrote:
> lgtm

Added thakis to review changes to build/common.gypi.

[brettw, 2016-02-18 04:30:21]

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

[brettw, 2016-02-18 04:30:21]

Sorry to come in late and add a lot of headache:

FOr new feature defines, we should be using the new buildflag system rather than
adding defines in common.gypi. This generates headers with the defines in it to
help modularlize the build, so we don't pass random defines to 20K files, most
of which don't need it.

There should be extensive documentation in build/buildflag_header.gypi and
build/buildflag_header.gni (for the two different builds).

This will localize the C define to a small area. Looking at your code, this
header would be generated by components/leak_detector.

Ideally the GN/GYP variable should also be localized as much as possible.
Whether this can also go into components/leak_detector or needs to be more
global (or something else) depends on the GYP code you added for
"fno-omit-frame-pointer" (for future reference, this kind of thing really needs
a comment about why we need it).

Is this flag necessary for the feature or does it just improve the data quality?
I would like to be very cautious about this kind of thing, and I'm also
suspicious about ChromeOS diverging from Chrome. in these areas. If it's
possible to land something mostly useful without this change, that would be nice
so we can talk about the flag with some Linux build experts separately.

[Will Harris, 2016-02-18 04:49:45]

https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.cc:38: const
LeakDetector::LeakReport& report) {
which threads are OnLeakFound and GetLeakReports being called on. Do you need a
thread checker here to avoid concurrent access to stored_reports_

[Simon Que, 2016-02-18 05:16:04]

https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.cc:38: const
LeakDetector::LeakReport& report) {
On 2016/02/18 04:49:45, Will Harris (ooo until 25 Feb) wrote:
> which threads are OnLeakFound and GetLeakReports being called on. Do you need
a
> thread checker here to avoid concurrent access to stored_reports_

OnLeakFound could be called from any thread but GetLeakReports will be called
from the main thread of the browser process, just like its counterpart
GetSampledProfiles() in perf_provider_chromeos.cc is currently being called.

Do you think a base::Lock() will suffice here?

[Will Harris, 2016-02-18 05:23:27]

On 2016/02/18 05:16:04, Simon Que wrote:
>
https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
> File chrome/browser/metrics/leak_detector_controller.cc (right):
> 
>
https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics...
> chrome/browser/metrics/leak_detector_controller.cc:38: const
> LeakDetector::LeakReport& report) {
> On 2016/02/18 04:49:45, Will Harris (ooo until 25 Feb) wrote:
> > which threads are OnLeakFound and GetLeakReports being called on. Do you
need
> a
> > thread checker here to avoid concurrent access to stored_reports_
> 
> OnLeakFound could be called from any thread but GetLeakReports will be called
> from the main thread of the browser process, just like its counterpart
> GetSampledProfiles() in perf_provider_chromeos.cc is currently being called.
> 
> Do you think a base::Lock() will suffice here?

It's more chromium style to post the message back to the UI thread, and make the
class check it's thread safety with DCHECK_CURRENTLY_ON, rather than using locks
- e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/us...

asvitkine may have input on this as this in chrome/browser/metrics

[Alexei Svitkine (slow), 2016-02-18 15:55:58]

Agree with Will. OnLeakFound() should only be called on the UI thread and
it makes sense to have it assert this by checking the thread. I suggest
using a ThreadChecker class instance rather than DCHECK_CURRENTLY_ON().

You can make the callback that's coming from another thread post the task
back to the UI thread as Will suggests.

On Thu, Feb 18, 2016 at 12:23 AM, <wfh@chromium.org> wrote:

>
> On 2016/02/18 05:16:04, Simon Que wrote:
>
>https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics/leak_detector_controller.cc
> > File chrome/browser/metrics/leak_detector_controller.cc (right):
> >
>
>https://codereview.chromium.org/1681263003/diff/120001/chrome/browser/metrics/leak_detector_controller.cc#newcode38
> > chrome/browser/metrics/leak_detector_controller.cc:38: const
> > LeakDetector::LeakReport& report) {
> > On 2016/02/18 04:49:45, Will Harris (ooo until 25 Feb) wrote:
> > > which threads are OnLeakFound and GetLeakReports being called on. Do you
> need
> > a
> > > thread checker here to avoid concurrent access to stored_reports_
> >
> > OnLeakFound could be called from any thread but GetLeakReports will be
called
> > from the main thread of the browser process, just like its counterpart
> > GetSampledProfiles() in perf_provider_chromeos.cc is currently being
called.
> >
> > Do you think a base::Lock() will suffice here?
>
> It's more chromium style to post the message back to the UI thread, and make
the
> class check it's thread safety with DCHECK_CURRENTLY_ON, rather than using
locks
> -
e.g.https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/user_metrics.cc&sq=package:chromium&type=cs&l=16
>
> asvitkine may have input on this as this in chrome/browser/metrics
> https://codereview.chromium.org/1681263003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2016-02-18 19:14:59]

Where should the post-back-to-UI-thread take place? In OnLeakFound() or in the
caller of OnLeakFound()?

If the latter, the current call is using the ObserverList macro:

    FOR_EACH_OBSERVER(Observer, observers_, OnLeakFound(report));

Should this be unrolled into an explicit for loop containing a PostTask() call?

[Simon Que, 2016-02-18 20:33:02]

On 2016/02/18 04:30:21, brettw wrote:
> There should be extensive documentation in build/buildflag_header.gypi and
> build/buildflag_header.gni (for the two different builds).
> 
> This will localize the C define to a small area. Looking at your code, this
> header would be generated by components/leak_detector.

The components/metrics/leak_detector code is included in the build in
components/metrics/BUILD.gn. So I've attempted to add a metrics_flags.h file
using buildflag_header in that file. But it's not being generated. Can you see
what I've done wrong?

> Ideally the GN/GYP variable should also be localized as much as possible.
> Whether this can also go into components/leak_detector or needs to be more
> global (or something else) depends on the GYP code you added for
> "fno-omit-frame-pointer" (for future reference, this kind of thing really
needs
> a comment about why we need it).

The GN variable is already in allocator.gni, which is not global. I'll look into
it for GYP.

> Is this flag necessary for the feature or does it just improve the data
quality?
> I would like to be very cautious about this kind of thing, and I'm also
> suspicious about ChromeOS diverging from Chrome. in these areas. If it's
> possible to land something mostly useful without this change, that would be
nice
> so we can talk about the flag with some Linux build experts separately.

I've removed this. It doesn't need to be in here.

[Alexei Svitkine (slow), 2016-02-18 21:09:20]

I wonder if we really need the extra complexity from the BUILD files vs. just
having a header that does:

#if defined(OS_CHROMEOS)
#define ENABLE_LEAK_DETECTOR 1
#else
#define ENABLE_LEAK_DETECTOR 0
#endif

And just leave it at that? Then we don't need any generated files or build
changes and the above could be in the leak_detector.h header file at the top?

https://codereview.chromium.org/1681263003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chromeos_metrics_provider.h (right):

https://codereview.chromium.org/1681263003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chromeos_metrics_provider.h:89: // For runtime memory
leak detection
Nit: Full sentences.

[Simon Que, 2016-02-18 21:11:39]

On 2016/02/18 21:09:20, Alexei Svitkine wrote:
> I wonder if we really need the extra complexity from the BUILD files vs. just
> having a header that does:
> 
> #if defined(OS_CHROMEOS)
> #define ENABLE_LEAK_DETECTOR 1
> #else
> #define ENABLE_LEAK_DETECTOR 0
> #endif
> 
> And just leave it at that? Then we don't need any generated files or build
> changes and the above could be in the leak_detector.h header file at the top?

I'm not looking to unconditionally enable this on Chrome OS right now. Instead,
we would enable it thru the CrOS portage build system, by passing in the
enable_leak_detector flag.

[brettw, 2016-02-18 21:14:14]

On 2016/02/18 21:09:20, Alexei Svitkine wrote:
> I wonder if we really need the extra complexity from the BUILD files vs. just
> having a header that does:
> 
> #if defined(OS_CHROMEOS)
> #define ENABLE_LEAK_DETECTOR 1
> #else
> #define ENABLE_LEAK_DETECTOR 0
> #endif
> 
> And just leave it at that? Then we don't need any generated files or build
> changes and the above could be in the leak_detector.h header file at the top?

This is a reasonable question. It depends on what we expect the future will be
for this feature and how we expect it to be used.

[brettw, 2016-02-18 21:14:58]

It looks like you added the code to generate the header to the GN build but not
GYP. See build/buildflag_header.gypi

https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi#newc...
build/common.gypi:3089: 'defines': ['ENABLE_LEAK_DETECTOR=1'],
With the build flag system, this should not be necessary.

https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
File build/config/allocator.gni (right):

https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
build/config/allocator.gni:19: enable_leak_detector = false
This is still global in the sense that it's in the shared build directory. This
is used by different projects, including standalone V8. It doesn't apply to
non-Chrome projects.

Since this is a feature controlled by a component, the flag should also be in
that component. I'd add a new file
	components/metrics/leak_detector/leak_detector.gni and put this flag in a
declare_args block in there.

Can the documentation for this be more complete? This of this from the
perspective of somebody looking at this and wondering if they should turn it on.

[Simon Que, 2016-02-18 21:23:45]

On 2016/02/18 21:14:58, brettw wrote:
> It looks like you added the code to generate the header to the GN build but
not
> GYP. See build/buildflag_header.gypi

That was intentional. See my earlier comment about not being able to figure out
adding the flag to the GN build. I didn't want to do GYP until I had figured out
GN, in case there were similar issues in GYP.

> 
> https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi#newc...
> build/common.gypi:3089: 'defines': ['ENABLE_LEAK_DETECTOR=1'],
> With the build flag system, this should not be necessary.
> 
>
https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
> File build/config/allocator.gni (right):
> 
>
https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
> build/config/allocator.gni:19: enable_leak_detector = false
> This is still global in the sense that it's in the shared build directory.
This
> is used by different projects, including standalone V8. It doesn't apply to
> non-Chrome projects.
> 
> Since this is a feature controlled by a component, the flag should also be in
> that component. I'd add a new file
> 	components/metrics/leak_detector/leak_detector.gni and put this flag in a
> declare_args block in there.

OK

> 
> Can the documentation for this be more complete? This of this from the
> perspective of somebody looking at this and wondering if they should turn it
on.

OK

[brettw, 2016-02-18 21:30:48]

On 2016/02/18 21:23:45, Simon Que wrote:
> On 2016/02/18 21:14:58, brettw wrote:
> > It looks like you added the code to generate the header to the GN build but
> not
> > GYP. See build/buildflag_header.gypi
> 
> That was intentional. See my earlier comment about not being able to figure
out
> adding the flag to the GN build. I didn't want to do GYP until I had figured
out
> GN, in case there were similar issues in GYP.

The GN code doesn't look wrong at first glance. You'll have to be more specific
about the error you see.

[Simon Que, 2016-02-18 23:22:08]

On 2016/02/18 21:30:48, brettw wrote:
> On 2016/02/18 21:23:45, Simon Que wrote:
> > On 2016/02/18 21:14:58, brettw wrote:
> > > It looks like you added the code to generate the header to the GN build
but
> > not
> > > GYP. See build/buildflag_header.gypi
> > 
> > That was intentional. See my earlier comment about not being able to figure
> out
> > adding the flag to the GN build. I didn't want to do GYP until I had figured
> out
> > GN, in case there were similar issues in GYP.
> 
> The GN code doesn't look wrong at first glance. You'll have to be more
specific
> about the error you see.

There is no error message. The file metrics_flags.h was not generated, that's
all.

[Simon Que, 2016-02-19 00:02:09]

On 2016/02/18 23:22:08, Simon Que wrote:
> On 2016/02/18 21:30:48, brettw wrote:
> > On 2016/02/18 21:23:45, Simon Que wrote:
> > > On 2016/02/18 21:14:58, brettw wrote:
> > > > It looks like you added the code to generate the header to the GN build
> but
> > > not
> > > > GYP. See build/buildflag_header.gypi
> > > 
> > > That was intentional. See my earlier comment about not being able to
figure
> > out
> > > adding the flag to the GN build. I didn't want to do GYP until I had
figured
> > out
> > > GN, in case there were similar issues in GYP.
> > 
> > The GN code doesn't look wrong at first glance. You'll have to be more
> specific
> > about the error you see.
> 
> There is no error message. The file metrics_flags.h was not generated, that's
> all.

I figured it out... I had to export "metrics_flags" as a public_dep in
metrics/BUILD.gn AND add "metrics:metrics_flags" as a dependency of e.g.
unit_tests for it to be enabled.

[Simon Que, 2016-02-19 02:10:11]

Patchset #9 (id:160001) has been deleted

[Simon Que, 2016-02-19 02:13:02]

https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/1681263003/diff/140001/build/common.gypi#newc...
build/common.gypi:3089: 'defines': ['ENABLE_LEAK_DETECTOR=1'],
On 2016/02/18 21:14:57, brettw wrote:
> With the build flag system, this should not be necessary.

Done.

https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
File build/config/allocator.gni (right):

https://codereview.chromium.org/1681263003/diff/140001/build/config/allocator...
build/config/allocator.gni:19: enable_leak_detector = false
On 2016/02/18 21:14:58, brettw wrote:
> This is still global in the sense that it's in the shared build directory.
This
> is used by different projects, including standalone V8. It doesn't apply to
> non-Chrome projects.
> 
> Since this is a feature controlled by a component, the flag should also be in
> that component. I'd add a new file
> 	components/metrics/leak_detector/leak_detector.gni and put this flag in a
> declare_args block in there.
> 
> Can the documentation for this be more complete? This of this from the
> perspective of somebody looking at this and wondering if they should turn it
on.

Done.

https://codereview.chromium.org/1681263003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/chromeos_metrics_provider.h (right):

https://codereview.chromium.org/1681263003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/chromeos_metrics_provider.h:89: // For runtime memory
leak detection
On 2016/02/18 21:09:19, Alexei Svitkine wrote:
> Nit: Full sentences.

Done.

[Simon Que, 2016-02-19 02:33:55]

The GN build completes successfully, but the GYP build does not. There is a
metrics_flags_buildflag_header.rsp file generated by the GYP script, with the
correct contents ("ENABLE_LEAK_DETECTOR=1"). I've added a dependency on
metrics_flags in chrome_browser.gypi, but this doesn't seem to generate a
corresponding header file.

Also, should "metrics_flags.h" be named "metrics_features.h"? That seems to be
the common usage elsewhere but I don't see it listed as a requirement.

https://codereview.chromium.org/1681263003/diff/180001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1681263003/diff/180001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3616: ['chromeos==1 and enable_leak_detector==1', {
Still have to include the flag definition somewhere.

[Will Harris, 2016-02-19 02:49:58]

On 2016/02/18 21:11:39, Simon Que wrote:
> I'm not looking to unconditionally enable this on Chrome OS right now.
Instead,
> we would enable it thru the CrOS portage build system, by passing in the
> enable_leak_detector flag.

I'm not familiar with the CrOS portage but how do you plan to deploy this users?
Since this is a compile time flag any build with enable_leak_detector will
construct the leak detector object and have it enabled. Are you going to deploy
different builds to different users, or plan to deploy to an entire population?
Will you also have a runtime flag that you can finch on or off as well?

[Simon Que, 2016-02-19 03:05:23]

On 2016/02/19 02:49:58, Will Harris (ooo until 25 Feb) wrote:
> On 2016/02/18 21:11:39, Simon Que wrote:
> > I'm not looking to unconditionally enable this on Chrome OS right now.
> Instead,
> > we would enable it thru the CrOS portage build system, by passing in the
> > enable_leak_detector flag.
> 
> I'm not familiar with the CrOS portage but how do you plan to deploy this
users?
> Since this is a compile time flag any build with enable_leak_detector will
> construct the leak detector object and have it enabled. Are you going to
deploy
> different builds to different users, or plan to deploy to an entire
population?
> Will you also have a runtime flag that you can finch on or off as well?

It will be deployed to users who opt in to be send usage statistics. Not to all
users. However, that will come only after extensive performance testing to make
sure it does not introduce too much overhead.

In the future, I plan to use Finch to control different input parameters. So
it's possible that it could be turned on only for a subset of opted-in users.

[Will Harris, 2016-02-19 03:09:32]

On 2016/02/19 03:05:23, Simon Que wrote:
> On 2016/02/19 02:49:58, Will Harris (ooo until 25 Feb) wrote:
> > On 2016/02/18 21:11:39, Simon Que wrote:
> > > I'm not looking to unconditionally enable this on Chrome OS right now.
> > Instead,
> > > we would enable it thru the CrOS portage build system, by passing in the
> > > enable_leak_detector flag.
> > 
> > I'm not familiar with the CrOS portage but how do you plan to deploy this
> users?
> > Since this is a compile time flag any build with enable_leak_detector will
> > construct the leak detector object and have it enabled. Are you going to
> deploy
> > different builds to different users, or plan to deploy to an entire
> population?
> > Will you also have a runtime flag that you can finch on or off as well?
> 
> It will be deployed to users who opt in to be send usage statistics. Not to
all
> users. However, that will come only after extensive performance testing to
make
> sure it does not introduce too much overhead.
> 
> In the future, I plan to use Finch to control different input parameters. So
> it's possible that it could be turned on only for a subset of opted-in users.

I suppose what I'm asking is whether the code could be unconditionally compiled
on Cros but just enabled in runtime via a command line flag. This would avoid
having to use any build flags at all.

But I don't know if that fits your deployment scenario. I expect you will want
experiment groups to determine the real-world impact on performance, even while
you are testing that.

[Simon Que, 2016-02-19 03:29:16]

On 2016/02/19 03:09:32, Will Harris (ooo until 25 Feb) wrote:
> On 2016/02/19 03:05:23, Simon Que wrote:
> > On 2016/02/19 02:49:58, Will Harris (ooo until 25 Feb) wrote:
> > > On 2016/02/18 21:11:39, Simon Que wrote:
> > > > I'm not looking to unconditionally enable this on Chrome OS right now.
> > > Instead,
> > > > we would enable it thru the CrOS portage build system, by passing in the
> > > > enable_leak_detector flag.
> > > 
> > > I'm not familiar with the CrOS portage but how do you plan to deploy this
> > users?
> > > Since this is a compile time flag any build with enable_leak_detector will
> > > construct the leak detector object and have it enabled. Are you going to
> > deploy
> > > different builds to different users, or plan to deploy to an entire
> > population?
> > > Will you also have a runtime flag that you can finch on or off as well?
> > 
> > It will be deployed to users who opt in to be send usage statistics. Not to
> all
> > users. However, that will come only after extensive performance testing to
> make
> > sure it does not introduce too much overhead.
> > 
> > In the future, I plan to use Finch to control different input parameters. So
> > it's possible that it could be turned on only for a subset of opted-in
users.
>
> I suppose what I'm asking is whether the code could be unconditionally
compiled
> on Cros but just enabled in runtime via a command line flag. This would avoid
> having to use any build flags at all.

That's possible too. However, during some earlier discussions, people were
concerned about overhead (e.g. in the initial design doc)... so I thought it was
politically safer to go with enabling it in the CrOS build system.

[Will Harris, 2016-02-19 04:07:47]

Description was changed from

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=chromium:382705
TEST=build successfully, run unit_tests
==========

to

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests
==========

[Will Harris, 2016-02-19 04:12:14]

On 2016/02/19 03:29:16, Simon Que wrote:
> On 2016/02/19 03:09:32, Will Harris (ooo until 25 Feb) wrote:
> > On 2016/02/19 03:05:23, Simon Que wrote:
> > > On 2016/02/19 02:49:58, Will Harris (ooo until 25 Feb) wrote:
> > > > On 2016/02/18 21:11:39, Simon Que wrote:
> > > > > I'm not looking to unconditionally enable this on Chrome OS right now.
> > > > Instead,
> > > > > we would enable it thru the CrOS portage build system, by passing in
the
> > > > > enable_leak_detector flag.
> > > > 
> > > > I'm not familiar with the CrOS portage but how do you plan to deploy
this
> > > users?
> > > > Since this is a compile time flag any build with enable_leak_detector
will
> > > > construct the leak detector object and have it enabled. Are you going to
> > > deploy
> > > > different builds to different users, or plan to deploy to an entire
> > > population?
> > > > Will you also have a runtime flag that you can finch on or off as well?
> > > 
> > > It will be deployed to users who opt in to be send usage statistics. Not
to
> > all
> > > users. However, that will come only after extensive performance testing to
> > make
> > > sure it does not introduce too much overhead.
> > > 
> > > In the future, I plan to use Finch to control different input parameters.
So
> > > it's possible that it could be turned on only for a subset of opted-in
> users.
> >
> > I suppose what I'm asking is whether the code could be unconditionally
> compiled
> > on Cros but just enabled in runtime via a command line flag. This would
avoid
> > having to use any build flags at all.
> 
> That's possible too. However, during some earlier discussions, people were
> concerned about overhead (e.g. in the initial design doc)... so I thought it
was
> politically safer to go with enabling it in the CrOS build system.

but surely if the object is never constructed, then it never gets initialized,
the allocator hook never lands, and so there is no overhead? Are you talking
about binary size overhead?

[Simon Que, 2016-02-19 05:53:36]

Will and I talked over hangouts about this matter. I intend to have some sort of
runtime flag for the leak detector (e.g. for Finch) at some point anyway, so we
could just implement it now. Here's what we propose:

- The GN/GYP variables and compiler #define will be removed.
- Include the new modules in the Chrome build for CrOS only.
- Add a runtime flag "--enable-leak-detector", similar to e.g. "--no-sandbox".
- In ChromeOSMetricsProvider, the LeakDetectorController will be stored as a
scoped_ptr. The ChromeOSMetricsProvider constructor will check the runtime flag.
If it is set, then it will instantiate a LeakDetectorController in the
scoped_ptr.

This avoids the trouble of trying to tie it all together in parallel across GYP
and GN builds.

Alexei and Brett, what do you think?

[Simon Que, 2016-02-19 05:53:39]

Will and I talked over hangouts about this matter. I intend to have some sort of
runtime flag for the leak detector (e.g. for Finch) at some point anyway, so we
could just implement it now. Here's what we propose:

- The GN/GYP variables and compiler #define will be removed.
- Include the new modules in the Chrome build for CrOS only.
- Add a runtime flag "--enable-leak-detector", similar to e.g. "--no-sandbox".
- In ChromeOSMetricsProvider, the LeakDetectorController will be stored as a
scoped_ptr. The ChromeOSMetricsProvider constructor will check the runtime flag.
If it is set, then it will instantiate a LeakDetectorController in the
scoped_ptr.

This avoids the trouble of trying to tie it all together in parallel across GYP
and GN builds.

Alexei and Brett, what do you think?

[Alexei Svitkine (slow), 2016-02-19 15:46:20]

+1 for the runtime check. Instead of using a flag, use a base::Feature
which allows control via a field trial without any extra code in the client.
On 19 Feb 2016 12:53 a.m., <sque@chromium.org> wrote:

>
> Will and I talked over hangouts about this matter. I intend to have some sort
of
> runtime flag for the leak detector (e.g. for Finch) at some point anyway, so
we
> could just implement it now. Here's what we propose:
>
> - The GN/GYP variables and compiler #define will be removed.
> - Include the new modules in the Chrome build for CrOS only.
> - Add a runtime flag "--enable-leak-detector", similar to e.g.
"--no-sandbox".
> - In ChromeOSMetricsProvider, the LeakDetectorController will be stored as a
> scoped_ptr. The ChromeOSMetricsProvider constructor will check the runtime
flag.
> If it is set, then it will instantiate a LeakDetectorController in the
> scoped_ptr.
>
> This avoids the trouble of trying to tie it all together in parallel across
GYP
> and GN builds.
>
> Alexei and Brett, what do you think?
> https://codereview.chromium.org/1681263003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2016-02-19 17:40:58]

On 2016/02/18 23:22:08, Simon Que wrote:
> There is no error message. The file metrics_flags.h was not generated, that's
> all.

I'm glad you figured this out below. For future reference, what you wrote here
is still not anything I can help with. You have a rule that definitely does
generate this file. Something I maybe could have helped with is an error message
about a file not being found when compiling a specific source file.

[Simon Que, 2016-02-19 19:09:47]

On 2016/02/19 15:46:20, Alexei Svitkine wrote:
> +1 for the runtime check. Instead of using a flag, use a base::Feature
> which allows control via a field trial without any extra code in the client.

That means add it to here?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/abo...

[Alexei Svitkine (slow), 2016-02-19 19:21:27]

Adding it to about_flags.cc is optional and it's up to you if you want it
there.

My point was instead of a command-line flag, have be a base::Feature via
base/feature_list.h. See go/feature-api-announcement for details.

On Fri, Feb 19, 2016 at 2:09 PM, <sque@chromium.org> wrote:

> On 2016/02/19 15:46:20, Alexei Svitkine wrote:
> > +1 for the runtime check. Instead of using a flag, use a base::Feature
> > which allows control via a field trial without any extra code in the
client.
>
> That means add it to
here?https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/about_flags.cc&sq=package:chromium&type=cs&l=600&rcl=1455885257
> https://codereview.chromium.org/1681263003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2016-02-19 22:52:14]

Patchset #10 (id:200001) has been deleted

[Simon Que, 2016-02-19 23:05:08]

I've added the runtime flag.

I still need to resolve the thread safety issues. At first I thought to update
LeakDetector::NotifyObservers() in leak_detector.cc to use PostTask() on each
observer. But that doesn't take care of the thread safety of the observer list,
which could be modified by another thread.

Instead, I propose that the class LeakDetector should be made thread-safe:
1. Add a base::ThreadChecker and use it in {Add|Remove}Observer().
2. In NotifyObservers, post the task to the main thread if it is not currently
called from the main thread. That way the FOR_EACH_OBSERVER() loop stays intact.
The Closure that is passed to PostTask will have a std::vector<LeakReport>
attached to it -- is that okay for PostTask()?

[Alexei Svitkine (slow), 2016-02-20 00:13:00]

That SGTM
On 19 Feb 2016 6:05 p.m., <sque@chromium.org> wrote:

>
> I've added the runtime flag.
>
> I still need to resolve the thread safety issues. At first I thought to
update
> LeakDetector::NotifyObservers() in leak_detector.cc to use PostTask() on each
> observer. But that doesn't take care of the thread safety of the observer
list,
> which could be modified by another thread.
>
> Instead, I propose that the class LeakDetector should be made thread-safe:
> 1. Add a base::ThreadChecker and use it in {Add|Remove}Observer().
> 2. In NotifyObservers, post the task to the main thread if it is not
currently
> called from the main thread. That way the FOR_EACH_OBSERVER() loop stays
intact.
> The Closure that is passed to PostTask will have a std::vector<LeakReport>
> attached to it -- is that okay for PostTask()?
> https://codereview.chromium.org/1681263003/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Simon Que, 2016-02-20 02:18:29]

On 2016/02/20 00:13:00, Alexei Svitkine wrote:
> That SGTM
> On 19 Feb 2016 6:05 p.m., <mailto:sque@chromium.org> wrote:
> 
> >
> > I've added the runtime flag.
> >
> > I still need to resolve the thread safety issues. At first I thought to
> update
> > LeakDetector::NotifyObservers() in leak_detector.cc to use PostTask() on
each
> > observer. But that doesn't take care of the thread safety of the observer
> list,
> > which could be modified by another thread.
> >
> > Instead, I propose that the class LeakDetector should be made thread-safe:
> > 1. Add a base::ThreadChecker and use it in {Add|Remove}Observer().
> > 2. In NotifyObservers, post the task to the main thread if it is not
> currently
> > called from the main thread. That way the FOR_EACH_OBSERVER() loop stays
> intact.
> > The Closure that is passed to PostTask will have a std::vector<LeakReport>
> > attached to it -- is that okay for PostTask()?
> > https://codereview.chromium.org/1681263003/

I'm trying to implement it but it fails in LeakDetectorTest. I've added a
TestSimpleTaskRunner and ThreadTaskRunnerHandle to the unit test but they don't
seem to be sufficient. I think the whole system also requires the instantiation
of a BrowserThread object to run properly. How is this done in other unit tests
where the class being tested posts to a BrowserThread?

[Will Harris, 2016-02-20 02:29:25]

On 2016/02/20 02:18:29, Simon Que wrote:
> On 2016/02/20 00:13:00, Alexei Svitkine wrote:
> > That SGTM
> > On 19 Feb 2016 6:05 p.m., <mailto:sque@chromium.org> wrote:
> > 
> > >
> > > I've added the runtime flag.
> > >
> > > I still need to resolve the thread safety issues. At first I thought to
> > update
> > > LeakDetector::NotifyObservers() in leak_detector.cc to use PostTask() on
> each
> > > observer. But that doesn't take care of the thread safety of the observer
> > list,
> > > which could be modified by another thread.
> > >
> > > Instead, I propose that the class LeakDetector should be made thread-safe:
> > > 1. Add a base::ThreadChecker and use it in {Add|Remove}Observer().
> > > 2. In NotifyObservers, post the task to the main thread if it is not
> > currently
> > > called from the main thread. That way the FOR_EACH_OBSERVER() loop stays
> > intact.
> > > The Closure that is passed to PostTask will have a std::vector<LeakReport>
> > > attached to it -- is that okay for PostTask()?
> > > https://codereview.chromium.org/1681263003/
> 
> I'm trying to implement it but it fails in LeakDetectorTest. I've added a
> TestSimpleTaskRunner and ThreadTaskRunnerHandle to the unit test but they
don't
> seem to be sufficient. I think the whole system also requires the
instantiation
> of a BrowserThread object to run properly. How is this done in other unit
tests
> where the class being tested posts to a BrowserThread?

you likely need a TestBrowserThreadBundle instantiated in your test class.

[Simon Que, 2016-02-20 03:12:12]

On 2016/02/20 02:29:25, Will Harris (ooo until 25 Feb) wrote:
> On 2016/02/20 02:18:29, Simon Que wrote:
> > On 2016/02/20 00:13:00, Alexei Svitkine wrote:
> > > That SGTM
> > > On 19 Feb 2016 6:05 p.m., <mailto:sque@chromium.org> wrote:
> > > 
> > > >
> > > > I've added the runtime flag.
> > > >
> > > > I still need to resolve the thread safety issues. At first I thought to
> > > update
> > > > LeakDetector::NotifyObservers() in leak_detector.cc to use PostTask() on
> > each
> > > > observer. But that doesn't take care of the thread safety of the
observer
> > > list,
> > > > which could be modified by another thread.
> > > >
> > > > Instead, I propose that the class LeakDetector should be made
thread-safe:
> > > > 1. Add a base::ThreadChecker and use it in {Add|Remove}Observer().
> > > > 2. In NotifyObservers, post the task to the main thread if it is not
> > > currently
> > > > called from the main thread. That way the FOR_EACH_OBSERVER() loop stays
> > > intact.
> > > > The Closure that is passed to PostTask will have a
std::vector<LeakReport>
> > > > attached to it -- is that okay for PostTask()?
> > > > https://codereview.chromium.org/1681263003/
> > 
> > I'm trying to implement it but it fails in LeakDetectorTest. I've added a
> > TestSimpleTaskRunner and ThreadTaskRunnerHandle to the unit test but they
> don't
> > seem to be sufficient. I think the whole system also requires the
> instantiation
> > of a BrowserThread object to run properly. How is this done in other unit
> tests
> > where the class being tested posts to a BrowserThread?
> 
> you likely need a TestBrowserThreadBundle instantiated in your test class.

Thanks, that did it. PTAL

[Will Harris, 2016-02-20 08:54:05]

looking good

about to jump on plane, but few comments

Q. where will the logic be for actually deciding to generate a leak report - I
presume inside components/metrics, with chrome/browser/metrics just being a
consumer of these?

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.cc:39:
DCHECK(thread_checker_.CalledOnValidThread());
rest of these methods need thread_checker_ to be checked, also dtor.

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

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.h:30: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
why is this protected and not public, normally interface overrides are public.

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:35: // This class is
thread-safe.
This class is not thread-safe, and it should always be called on the same thread
that instantiated it.

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:96: void NotifyObservers(const
std::vector<LeakReport>& reports);
what calls (or will call) this method, other than test? if only test then it can
be private and friended just for the test. (FRIEND_TEST_ALL_PREFIXES)

[Will Harris, 2016-02-20 08:54:07]

looking good

about to jump on plane, but few comments

Q. where will the logic be for actually deciding to generate a leak report - I
presume inside components/metrics, with chrome/browser/metrics just being a
consumer of these?

[Simon Que, 2016-02-22 23:26:18]

On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> looking good
> 
> about to jump on plane, but few comments
> 
> Q. where will the logic be for actually deciding to generate a leak report - I
> presume inside components/metrics, with chrome/browser/metrics just being a
> consumer of these?

It's in here:
https://codereview.chromium.org/1665553002/

I'll rebase that CL on top of this one when this lands.

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
File chrome/browser/metrics/leak_detector_controller.cc (right):

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.cc:39:
DCHECK(thread_checker_.CalledOnValidThread());
On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> rest of these methods need thread_checker_ to be checked, also dtor.

Done. But not ctor?

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

https://codereview.chromium.org/1681263003/diff/240001/chrome/browser/metrics...
chrome/browser/metrics/leak_detector_controller.h:30: void OnLeakFound(const
LeakDetector::LeakReport& report) override;
On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> why is this protected and not public, normally interface overrides are public.

In an earlier comment, Alexei suggested it be made private, presumably because
it's not being called directly from anywhere except the unit test.

But I can make this public if you think this is the more proper way.

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:35: // This class is
thread-safe.
On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> This class is not thread-safe, and it should always be called on the same
thread
> that instantiated it. 

Done.

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:96: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> what calls (or will call) this method, other than test? if only test then it
can
> be private and friended just for the test. (FRIEND_TEST_ALL_PREFIXES)

It will be called from the hook function, so this might have to be made public
in the next CL. See discussion here:
https://codereview.chromium.org/1681263003/diff2/40001:60001/components/metri...

[Alexei Svitkine (slow), 2016-02-23 01:23:35]

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:11: using
content::BrowserThread;
Nit: Just use the content:: prefix in the code directly and remove this.

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:15: const base::Feature
kEnableRuntimeMemoryLeakDetector = {
Nit: No =

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:16:
"enable-runtime-memory-leak-detector", base::FEATURE_DISABLED_BY_DEFAULT};
Nit: "RuntimeMemoryLeakDetector"

Given you're not checking for this feature under components/, suggest putting it
in chrome/ - for example in chrome_features.cc.

[Simon Que, 2016-02-23 02:38:29]

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:11: using
content::BrowserThread;
On 2016/02/23 01:23:35, Alexei Svitkine (slow) wrote:
> Nit: Just use the content:: prefix in the code directly and remove this.

Done.

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:15: const base::Feature
kEnableRuntimeMemoryLeakDetector = {
On 2016/02/23 01:23:35, Alexei Svitkine (slow) wrote:
> Nit: No =

Done.

https://codereview.chromium.org/1681263003/diff/260001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:16:
"enable-runtime-memory-leak-detector", base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/02/23 01:23:35, Alexei Svitkine (slow) wrote:
> Nit: "RuntimeMemoryLeakDetector"
> 
> Given you're not checking for this feature under components/, suggest putting
it
> in chrome/ - for example in chrome_features.cc.

Done.

[Alexei Svitkine (slow), 2016-02-23 16:56:53]

lgtm

https://codereview.chromium.org/1681263003/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/1681263003/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/chromeos_metrics_provider.cc:149:
features::kEnableRuntimeMemoryLeakDetector)) {
Nit: No need for "Enable" in the feature variable name.

https://codereview.chromium.org/1681263003/diff/280001/chrome/common/chrome_f...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1681263003/diff/280001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:30: "EnableRuntimeMemoryLeakDetector",
base::FEATURE_DISABLED_BY_DEFAULT};
Nit: No "Enable" in the name or variable name, please.

[Simon Que, 2016-02-23 18:43:23]

https://codereview.chromium.org/1681263003/diff/280001/chrome/browser/metrics...
File chrome/browser/metrics/chromeos_metrics_provider.cc (right):

https://codereview.chromium.org/1681263003/diff/280001/chrome/browser/metrics...
chrome/browser/metrics/chromeos_metrics_provider.cc:149:
features::kEnableRuntimeMemoryLeakDetector)) {
On 2016/02/23 16:56:53, Alexei Svitkine (slow) wrote:
> Nit: No need for "Enable" in the feature variable name.

Done.

https://codereview.chromium.org/1681263003/diff/280001/chrome/common/chrome_f...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/1681263003/diff/280001/chrome/common/chrome_f...
chrome/common/chrome_features.cc:30: "EnableRuntimeMemoryLeakDetector",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/02/23 16:56:53, Alexei Svitkine (slow) wrote:
> Nit: No "Enable" in the name or variable name, please.

Done.

[Simon Que, 2016-02-23 19:07:27]

sque@chromium.org changed reviewers:
+ jochen@chromium.org
- brettw@chromium.org, thakis@chromium.org

[Simon Que, 2016-02-23 19:07:29]

+jochen for:

- components/components_tests.gyp
- chrome/*.gypi
- chrome/browser/BUILD.gn

[Will Harris, 2016-02-23 20:48:26]

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:96: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/22 23:26:18, Simon Que wrote:
> On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> > what calls (or will call) this method, other than test? if only test then it
> can
> > be private and friended just for the test. (FRIEND_TEST_ALL_PREFIXES)
> 
> It will be called from the hook function, so this might have to be made public
> in the next CL. See discussion here:
>
https://codereview.chromium.org/1681263003/diff2/40001:60001/components/metri...

I still don't get why this is public. The code being called will be inside this
class, so it can just call itself? The interface to the hook is a method which
can be passed to the allocator during init of the object, it can still all be
private since there will only be one copy instantiated...

[Simon Que, 2016-02-23 23:19:23]

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.h (right):

https://codereview.chromium.org/1681263003/diff/240001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.h:96: void NotifyObservers(const
std::vector<LeakReport>& reports);
On 2016/02/23 20:48:26, Will Harris (ooo until 25 Feb) wrote:
> On 2016/02/22 23:26:18, Simon Que wrote:
> > On 2016/02/20 08:54:05, Will Harris (ooo until 25 Feb) wrote:
> > > what calls (or will call) this method, other than test? if only test then
it
> > can
> > > be private and friended just for the test. (FRIEND_TEST_ALL_PREFIXES)
> > 
> > It will be called from the hook function, so this might have to be made
public
> > in the next CL. See discussion here:
> >
>
https://codereview.chromium.org/1681263003/diff2/40001:60001/components/metri...
> 
> I still don't get why this is public. The code being called will be inside
this
> class, so it can just call itself? The interface to the hook is a method which
> can be passed to the allocator during init of the object, it can still all be
> private since there will only be one copy instantiated...

Done.

[jochen (gone - plz use gerrit), 2016-02-24 08:05:22]

it's a bit unclear to me how a sampling profiler would detect leaks?

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:18: int max_stack_depth,
what does a negative value mean here?

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:21: int
call_stack_suspicion_threshold)
and for those two? what do those values mean anyways?

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:24:
LeakDetector::LeakDetector() : LeakDetector(1.0f, 4, 32 * 1024 * 1024, 4, 4) {}
can you please define constants for these values, so they have names?

[Simon Que, 2016-02-24 21:28:35]

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
File components/metrics/leak_detector/leak_detector.cc (right):

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:18: int max_stack_depth,
On 2016/02/24 08:05:22, jochen wrote:
> what does a negative value mean here?

Changed to size_t.

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:21: int
call_stack_suspicion_threshold)
On 2016/02/24 08:05:22, jochen wrote:
> and for those two? what do those values mean anyways?

See comment in header file for meaning.

Changed to uint32_t.

https://codereview.chromium.org/1681263003/diff/340001/components/metrics/lea...
components/metrics/leak_detector/leak_detector.cc:24:
LeakDetector::LeakDetector() : LeakDetector(1.0f, 4, 32 * 1024 * 1024, 4, 4) {}
On 2016/02/24 08:05:22, jochen wrote:
> can you please define constants for these values, so they have names?

Done.

[jochen (gone - plz use gerrit), 2016-02-25 11:53:18]

what about my high-level question? how does this system detect leaks?

[Simon Que, 2016-02-25 19:02:47]

On 2016/02/25 11:53:18, jochen wrote:
> what about my high-level question? how does this system detect leaks?

It will be connected to class LeakDetectorImpl and the allocator in a subsequent
CL:
https://codereview.chromium.org/1665553002/diff/60001/components/metrics/leak...

That CL originally contained the contents of this one. But I've decided to split
it up to make review easier.

[Simon Que, 2016-02-26 01:54:12]

Patchset #19 (id:400001) has been deleted

[jochen (gone - plz use gerrit), 2016-02-26 12:15:24]

lgtm

[Simon Que, 2016-02-26 17:09:51]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-02-26 17:09:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1681263003/#ps420001
(title: "Remove default constructor of LeakDetector, move to unit test")

[commit-bot: I haz the power, 2016-02-26 17:13:10]

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

[commit-bot: I haz the power, 2016-02-26 17:27:04]

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

[commit-bot: I haz the power, 2016-02-26 17:27:06]

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

[Simon Que, 2016-02-26 19:22:08]

Patchset #20 (id:440001) has been deleted

[Simon Que, 2016-02-26 20:14:37]

The CQ bit was checked by sque@chromium.org

[Simon Que, 2016-02-26 20:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1681263003/#ps460001
(title: "Add content/public/browser to GN deps")

[commit-bot: I haz the power, 2016-02-26 20:18:53]

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

[commit-bot: I haz the power, 2016-02-26 20:37:24]

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

[commit-bot: I haz the power, 2016-02-26 20:37:26]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Simon Que, 2016-02-26 21:15:01]

The CQ bit was checked by sque@chromium.org

[commit-bot: I haz the power, 2016-02-26 21:18:29]

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

[commit-bot: I haz the power, 2016-02-26 22:47:26]

Description was changed from

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests
==========

to

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests
==========

[commit-bot: I haz the power, 2016-02-26 22:47:28]

Committed patchset #20 (id:460001)

[commit-bot: I haz the power, 2016-02-26 22:49:47]

Description was changed from

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests
==========

to

==========
metrics: Add leak detector controller in Chrome OS metrics system

The new class, LeakDetectorController, provides an interface to the leak
detector. It initializes the leak detector and registers itself to
receive leak reports. It converts the leak reports into protobuf format,
and can later pass them to UMA.

BUG=382705
TEST=build successfully, run unit_tests

Committed: https://crrev.com/82e4d1559b83487b7d75527a8d74bdb41b5f5472
Cr-Commit-Position: refs/heads/master@{#378011}
==========

[commit-bot: I haz the power, 2016-02-26 22:49:49]

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