Closed
Bug 1304660
Opened 8 years ago
Closed 8 years ago
Intermittent LeakSanitizer | leak at NS_NewRunnableFunction, TelemetryHistogram::Accumulate
Categories
(Toolkit :: Telemetry, defect, P2)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | --- | fixed |
People
(Reporter: intermittent-bug-filer, Unassigned)
References
Details
(Keywords: intermittent-failure, Whiteboard: [measurement:client])
Attachments
(1 file)
Updated•8 years ago
|
Comment 1•8 years ago
|
||
> =================================================================
> ==2679==ERROR: LeakSanitizer: detected memory leaks
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0x4b247b in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
> #1 0x4e0bcd in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17
> #2 0x7fea6d507daf in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
> #3 0x7fea6d507daf in NS_NewRunnableFunction<(lambda at /home/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1324:52)> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:293
> #4 0x7fea6d507daf in (anonymous namespace)::internal_RemoteAccumulate(mozilla::Telemetry::ID, unsigned int) /home/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1324
> #5 0x7fea6d4ffd1a in (anonymous namespace)::internal_Accumulate(mozilla::Telemetry::ID, unsigned int) /home/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:1358:7
> #6 0x7fea6d4ffc29 in TelemetryHistogram::Accumulate(mozilla::Telemetry::ID, unsigned int) /home/worker/workspace/build/src/toolkit/components/telemetry/TelemetryHistogram.cpp:2124:3
Comment 2•8 years ago
|
||
The relevant code:
> NS_DispatchToMainThread(NS_NewRunnableFunction([]() -> void {
> StaticMutexAutoLock locker(gTelemetryHistogramMutex);
> internal_armIPCTimerMainThread();
> }));
This is the same sort of thing that ImageEncoder::EnsureThreadPool() does [1].
The detected leak... I'm not sure how to read it. Is it blaming the NS_NewRunnableFunction for being the leak or for holding it?
[1]: https://dxr.mozilla.org/mozilla-central/rev/560b2c805bf7bebeb3ceebc495a81b2aa4c0c755/dom/base/ImageEncoder.cpp#550
Comment 4•8 years ago
|
||
I'd be willing to bet that you're dispatching runnables to the main thread post-xpcom-shutdown, and those runnables leak since the main thread isn't dispatching events any more.
Flags: needinfo?(nfroyd)
Comment 5•8 years ago
|
||
So... I need to query if xpcom has shut down before dispatching the new runnable. Sounds like a threadrace waiting to happen. Do you happen to know of a common pattern I should follow for ensuring that I don't dispatch just at the wrong time?
Flags: needinfo?(nfroyd)
Comment 6•8 years ago
|
||
(In reply to Chris H-C :chutten from comment #5)
> So... I need to query if xpcom has shut down before dispatching the new
> runnable. Sounds like a threadrace waiting to happen. Do you happen to know
> of a common pattern I should follow for ensuring that I don't dispatch just
> at the wrong time?
The usual pattern is to observe xpcom-shutdown (or xpcom-shutdown-threads, which is slightly later) and set a flag. That pattern still carries the possibility of being racy, but we take steps to ensure that all runnables get drained appropriately:
// After firing xpcom-shutdown:
http://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#893
// Shutting down threads, called from the above code.
http://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThreadManager.cpp#115
Flags: needinfo?(nfroyd)
Comment hidden (mozreview-request) |
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8795836 [details]
bug 1304660 - Don't dispatch to main once xpcom starts shutting down.
https://reviewboard.mozilla.org/r/81754/#review80642
This seems OK, but I am confused about the warning, below. Did you leave something out of the patch, or do you need to reword the warning?
::: toolkit/components/telemetry/TelemetryHistogram.cpp:2052
(Diff revision 1)
> + NS_WARNING("Could not get observer service during Telemetry init. "
> + "Will not dispatch non-main-thread Telemetry from this process.");
Uh. What is actually enforcing the consequence described in this warning?
Attachment #8795836 -
Flags: review?(nfroyd)
Comment 9•8 years ago
|
||
Oh right. Uh... so I got that completely backwards. Should be "Will not _cease_ dispatching non-main-thread Telemetry from this process."
It's a little wordy. Maybe "May leak a dispatched runnable on xpcom shutdown" would be better. I'll edit.
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
mozreview-review |
Comment on attachment 8795836 [details]
bug 1304660 - Don't dispatch to main once xpcom starts shutting down.
https://reviewboard.mozilla.org/r/81754/#review80714
r=me with the change below.
::: toolkit/components/telemetry/TelemetryHistogram.cpp:2052
(Diff revision 2)
> + NS_WARNING("Could not get observer service during Telemetry init. "
> + "May leak a dispatched runnable after xpcom shutdown.");
Might as well just `MOZ_ASSERT(obs)` here, and dispense with the warning.
Attachment #8795836 -
Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51199a9bdf29
Don't dispatch to main once xpcom starts shutting down. r=froydnj
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
status-firefox50:
--- → unaffected
status-firefox51:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•