Introduce the ANNOTATE_LEAKING_OBJECT_PTR annotation that can be used to mark

Introduce the ANNOTATE_LEAKING_OBJECT_PTR annotation that can be used to mark
heap allocated objects as intentionally leaked ones.
Annotate the histograms produced by {Histogram,BooleanHistogram,LinearHistogram,CustomHistogram}::FactoryGet(), as leaked.

Rename StatsHistogram::StatsHistogramFactoryGet to StatsHistogram::FactoryGet, annotate the result as leaky, update the suppressions.

BUG=79322
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82460

[Alexander Potapenko, 2011-04-15 13:01:53]

Please review

[jar (doing other things), 2011-04-15 15:53:37]

There is also one other caller that uses this FactoryGet pattern in
stats_histogram.cc. It would be nice to similarly update that fragment to keep
them all in sync. The function is currently called
StatsHistogram::StatsHistogramFactoryGet.  If you are feeling energetic, I'd
suggest its name also be shortened analogously to StatsHistogram::FactoryGet()
as you add your element to the internal pattern.

http://codereview.chromium.org/6869009/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/6869009/diff/1/base/metrics/histogram.cc#newco...
base/metrics/histogram.cc:105: ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see
crbug.com/79322
There are actually 3 distinct FactoryGet routines in this file (each handling a
slightly different set of argument lists).  Each has this same pattern, and each
should probably be annotated this way.  

I tried to find a way to factor out the common code patterns (I hate
replication), but did not see a clean and readable way... but if you see a
way... great.

[Alexander Potapenko, 2011-04-18 09:45:13]

Annotated the rest of histogram.cc, changed stats_histogram.{cc.h}, fixed the
suppressions. Please take another look.

http://codereview.chromium.org/6869009/diff/1/base/metrics/histogram.cc
File base/metrics/histogram.cc (right):

http://codereview.chromium.org/6869009/diff/1/base/metrics/histogram.cc#newco...
base/metrics/histogram.cc:105: ANNOTATE_LEAKING_OBJECT_PTR(histogram);  // see
crbug.com/79322
There are actually four of them, as I can see:

Histogram::FactoryGet
BooleanHistogram::FactoryGet
LinearHistogram::FactoryGet
CustomHistogram::FactoryGet

I've annotated all of them -- is that ok?

[jar (doing other things), 2011-04-18 18:29:54]

LGTM

There is perchance one nit you could handle below.

http://codereview.chromium.org/6869009/diff/4001/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/6869009/diff/4001/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:341:
'-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv',
I don't grok this line... but I'll assume it is reasonable.

http://codereview.chromium.org/6869009/diff/4001/tools/valgrind/memcheck/supp...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/6869009/diff/4001/tools/valgrind/memcheck/supp...
tools/valgrind/memcheck/suppressions.txt:893:
fun:_ZN10disk_cache14StatsHistogram10FactoryGet*
nit: You can probably now combine lines 886 and 893 into:
fun:_ZN*Histogram10FactoryGet*

and only have one suppression.

[Alexander Potapenko, 2011-04-19 10:05:10]

http://codereview.chromium.org/6869009/diff/4001/base/allocator/allocator.gyp
File base/allocator/allocator.gyp (right):

http://codereview.chromium.org/6869009/diff/4001/base/allocator/allocator.gyp...
base/allocator/allocator.gyp:341:
'-Wl,-u_ZN15HeapLeakChecker12IgnoreObjectEPKv,-u_ZN15HeapLeakChecker14UnIgnoreObjectEPKv',
On 2011/04/18 18:29:54, jar wrote:
> I don't grok this line... but I'll assume it is reasonable.

HeapLeakChecker::[Un]IgnoreObject is deleted from liballocator.a by the linker,
because it isn't used anywhere. We need to explicitly tell the linker not to do
so.

http://codereview.chromium.org/6869009/diff/4001/tools/valgrind/memcheck/supp...
File tools/valgrind/memcheck/suppressions.txt (right):

http://codereview.chromium.org/6869009/diff/4001/tools/valgrind/memcheck/supp...
tools/valgrind/memcheck/suppressions.txt:893:
fun:_ZN10disk_cache14StatsHistogram10FactoryGet*
On 2011/04/18 18:29:54, jar wrote:
> nit: You can probably now combine lines 886 and 893 into:
> fun:_ZN*Histogram10FactoryGet*
> 
> and only have one suppression.

In fact these suppressions should be removed at all once we make sure that the
annotations work, so it's pretty ok to join them. But I always try to keep in
mind Dan Kegel's precept about not abusing wildcards in Memcheck suppressions ;)

[Alexander Potapenko, 2011-04-19 10:08:30]

Oh, in fact PRESUBMIT disliked the merged suppressions, so I've separated them
back.

[Alexander Potapenko, 2011-04-19 10:18:04]

On 2011/04/19 10:08:30, Alexander Potapenko wrote:
> Oh, in fact PRESUBMIT disliked the merged suppressions, so I've separated them
> back.

+willchan as the base/ owner.
Will, could you please take a look?

[willchan no longer on Chromium, 2011-04-20 15:00:24]

LGTM rubberstamp (trusting jar's review here).