1304660 - Intermittent LeakSanitizer | leak at NS_NewRunnableFunction, TelemetryHistogram::Accumulate

Status: RESOLVED FIXED

Description

[Treeherder Bug Filer]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

> =================================================================
> ==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

[Chris H-C :chutten]

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 3

[Georg Fritzsche [:gfritzsche]]

Nathan, do you have an idea on whats leaking here?

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[Chris H-C :chutten]

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?

Comment 6

[Nathan Froyd [:froydnj]]

(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

Comment 7

[Chris H-C :chutten]

Attachment: bug 1304660 - Don't dispatch to main once xpcom starts shutting down.

If we manage to dispatch to main after xpcom shutdown, the main thread won't
care for our new runnable and leak it.

Review commit: https://reviewboard.mozilla.org/r/81754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81754/

Comment 8

[Nathan Froyd [:froydnj]]

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?

Comment 9

[Chris H-C :chutten]

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 10

[Chris H-C :chutten]

Comment on attachment 8795836 [details]
bug 1304660 - Don't dispatch to main once xpcom starts shutting down.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81754/diff/1-2/

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Chris H-C :chutten]

Comment on attachment 8795836 [details]
bug 1304660 - Don't dispatch to main once xpcom starts shutting down.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/81754/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51199a9bdf29
Don't dispatch to main once xpcom starts shutting down. r=froydnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51199a9bdf29