Closed Bug 824647 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Telemetry, defect, major)

All
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: gkw, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, valgrind)

Attachments

(2 files, 1 obsolete file)

Attached file 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.
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.
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.
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.
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++.
Attachment #721817 - Flags: review?(taras.mozilla)
New patch, without a bunch of unrelated cruft.
Attachment #721817 - Attachment is obsolete: true
Attachment #721817 - Flags: review?(taras.mozilla)
Attachment #722271 - Flags: review?(taras.mozilla)
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++
Attachment #722271 - Flags: review?(taras.mozilla) → feedback+
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.
Attachment #722271 - Flags: review?(justin.lebar+bug)
> 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?
(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.
Ah, if this lets you fix bug 806514, I am totally game.  I just hope we don't break anything here...
Comment on attachment 722271 [details] [diff] [review]
delete histograms when StatisticsRecorder is deleted

r=me with a friendly reminder to fix the whitespace.
Attachment #722271 - Flags: review?(justin.lebar+bug) → review+
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
https://hg.mozilla.org/mozilla-central/rev/a365ad559c45
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.