Closed Bug 1304660 Opened 4 years ago Closed 4 years ago

Intermittent LeakSanitizer | leak at NS_NewRunnableFunction, TelemetryHistogram::Accumulate

Categories

(Toolkit :: Telemetry, defect, P2)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
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)

bug 1304660 - Don't dispatch to main once xpcom starts shutting down.
58 bytes, text/x-review-board-request
froydnj
: review+
Details
Blocks: 1218576
Priority: -- → P2
Whiteboard: [measurement:client]
> =================================================================
> ==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
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
Nathan, do you have an idea on whats leaking here?
Flags: needinfo?(nfroyd)
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)
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)
(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 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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/51199a9bdf29
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.