Closed
Bug 1304660
Opened 4 years ago
Closed 4 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)
Filed by: tomcat [at] mozilla.com https://treeherder.mozilla.org/logviewer.html#?job_id=3859041&repo=autoland https://queue.taskcluster.net/v1/task/PQpaJfdrSwmTn-qd-4kc9A/runs/0/artifacts/public%2Flogs%2Flive_backing.log
Updated•4 years ago
|
Comment 1•4 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•4 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•4 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•4 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•4 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•4 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•4 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•4 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•4 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•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51199a9bdf29
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•4 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
•