824647 - Valgrind on tbpl detects leak - at least 368 bytes are definitely lost (direct) with (anonymous namespace)::HistogramGet or (anonymous namespace)::TelemetryImpl::HistogramFrom on the stack

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Valgrind stack

Valgrind detects a leak of at least 368 bytes (direct) with (anonymous namespace)::HistogramGet or (anonymous namespace)::TelemetryImpl::HistogramFrom on the stack, see attached snippet which comes from:

https://tbpl.mozilla.org/php/getParsedLog.php?id=18238235&tree=Firefox&full=1

Guessing Toolkit: Telemetry, please change component if necessary.

Regression window:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bf26f61a0748&tochange=84320dffec6e

I'm guessing bug 819732 may be related, from the regression window.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Suppressed in https://hg.mozilla.org/mozilla-central/rev/5a1f68dbd885

Comment 2

[Théo Chevalier]

Not sure to understand what you are talking about :)
Regarding your first link, there is a problem in automation.py.in, correct?

The only change I made in this file is:
+#expand user_pref("toolkit.telemetry.prompted", __MOZ_TELEMETRY_DISPLAY_REV__);
+#expand user_pref("toolkit.telemetry.notifiedOptOut", __MOZ_TELEMETRY_DISPLAY_REV__);

IIRC, I verified that __MOZ_TELEMETRY_DISPLAY_REV__ was correctly replaced.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I thought that seemed to be the only telemetry-related changeset in that regression window :-/ Please feel free to change it to another changeset in that regression window if necessary.

Comment 4

[Josh Matthews [:jdm]]

Bug 819732 is related only in that it enabled a non-default feature. The actual leak almost certainly comes from the allocation in http://hg.mozilla.org/mozilla-central/annotate/5a1f68dbd885/ipc/chromium/src/base/histogram.cc#l101, which is attached to a JS object's private slot in http://hg.mozilla.org/mozilla-central/annotate/5a1f68dbd885/toolkit/components/telemetry/Telemetry.cpp#l691 but does not appear to ever be deleted. The JSClass may require a finalizer that does so.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: delete histograms when StatisticsRecorder is deleted

I think this solves the problem; we were deleting the map connecting names
to histograms, but we were never deleting the contents of the map.  Love C++.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: delete histograms when StatisticsRecorder is deleted

New patch, without a bunch of unrelated cruft.

Comment 7

[(dormant account)]

Comment on attachment 722271 [details] [diff] [review]
delete histograms when StatisticsRecorder is deleted

I'm ok with general idea. Will let someone else review chrome chromec++

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 722271 [details] [diff] [review]
delete histograms when StatisticsRecorder is deleted

Not really sure who "owns" ipc nowadays since Chris left.  Justin, do you feel like a plausible candidate?

The destructor for StatisticsHistogram currently gets run as a static destructor, fwiw.

Comment 9

[Justin Lebar (not reading bugmail)]

> Justin, do you feel like a plausible candidate?

Yeah, I think it's up to me and bent.  I'd guess I know this code better than him, which is to say, I've opened this file once or twice before.  :)

Aside from the fact that you're mixing spaces and tabs and this file does not appear to do so predominantly, this patch looks fine, as far as I can tell.  But do you think it's worthwhile to add dangerous code run at shutdown just to appease valgrind?  There seems to be relatively high risk and relatively little reward to this patch, as compared to an addition to a memcheck ignores file.

What do you think?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Yeah, I think it's up to me and bent.  I'd guess I know this code better
> than him, which is to say, I've opened this file once or twice before.  :)

You're hired!

> Aside from the fact that you're mixing spaces and tabs and this file does
> not appear to do so predominantly, this patch looks fine, as far as I can
> tell.  But do you think it's worthwhile to add dangerous code run at
> shutdown just to appease valgrind?  There seems to be relatively high risk
> and relatively little reward to this patch, as compared to an addition to a
> memcheck ignores file.

It's probably not worth fixing this if this bug was the end of the story.  But fixing this leak would be a prerequisite for fixing bug 849272 (free histogram bits at xpcom-shutdown) and bug 806514 (use nsTArray in histogram for more well-defined malloc statistics).  The latter requires things to be properly freed so debug builds don't leak nsTArrays.

Comment 11

[Justin Lebar (not reading bugmail)]

Ah, if this lets you fix bug 806514, I am totally game.  I just hope we don't break anything here...

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 722271 [details] [diff] [review]
delete histograms when StatisticsRecorder is deleted

r=me with a friendly reminder to fix the whitespace.

Comment 13

[Nathan Froyd [:froydnj]]

Try run (mochitest, xpcshell) looked pretty green, so I don't think we're breaking things:

https://tbpl.mozilla.org/?tree=Try&rev=f7bcdcefeea9

https://hg.mozilla.org/integration/mozilla-inbound/rev/a365ad559c45

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a365ad559c45

Comment 15

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Suppression removed in:

https://hg.mozilla.org/mozilla-central/rev/e562fcadfcf3