1193922 - Intermittent web audio leaks caused by RunInStableState changes

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Andrew McCreight [:mccr8]]

Kyle's event loop changes revealed an intermittent web audio leak of about 12kb. I bumped the leak threshold for all e10s content processes in bug 1193917, but the underlying bug should be fixed, and the leak threshold reduced, so additional leaks don't creep in. This is happening in e10s Mochitest-2.

Here's what is leaking:
TEST-INFO | leakcheck | tab process: leaked 1 AsyncLatencyLogger (72 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AsyncTransactionTrackersHolder (72 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioChannelService (88 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioContext (256 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioDestinationNode (248 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioNode (160 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioNodeEngine (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 AudioNodeStream (464 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 CipherSuiteChangeObserver (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 CompositorChild (840 bytes)
TEST-INFO | leakcheck | tab process: leaked 4 CondVar (160 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 DOMEventTargetHelper (160 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 DataChannelShutdown (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 GraphDriver (80 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 MediaSegment (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 MediaStream (688 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 MediaStreamGraph (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 MediaStreamGraphImpl (416 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 MessagePump (16 bytes)
TEST-INFO | leakcheck | tab process: leaked 7 Mutex (224 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 PCompositorChild (736 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 PImageBridgeChild (760 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 ReentrantMonitor (80 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 RefCountedMonitor (160 bytes)
TEST-INFO | leakcheck | tab process: leaked 4 RefCountedTask (64 bytes)
TEST-INFO | leakcheck | tab process: leaked 141 StoreRef (2256 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 StreamBuffer (40 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 TimeVaryingBase (2 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 Track (24 bytes)
TEST-INFO | leakcheck | tab process: leaked 1 WaitableEventKernel (72 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 WeakReference<MessageListener> (32 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 ipc::MessageChannel (1024 bytes)
TEST-INFO | leakcheck | tab process: leaked 60 nsTArray_base (480 bytes)
TEST-INFO | leakcheck | tab process: leaked 2 nsThread (496 bytes) 

Note that some of these things leak in every run (like PImageBridgeChild), so you are mostly interested in the web audio-ish stuff here.

Comment 1

[Andrew McCreight [:mccr8]]

Here's a link to some logs:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=497b3313e006&filter-searchStr=Linux%20x64%20debug%20Mochitest%20e10s%20Mochitest%20e10s%20M-e10s%282%29&tochange=65723a508074

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is very similar to what came up in bug 1190019 comment 13.  This is likely the media stream graph/web audio failing to shutdown until xpcom-shutdown.

Comment 3

[Nicholas Nethercote [inactive]]

jya, is this something that you could look at (based on comment 2)?

Comment 4

[Jean-Yves Avenard [:jya]]

PAssing to Karl..

Comment 5

[Karl Tomlinson (:karlt)]

Reproduced plenty of AudioChannelService leaks but no AudioContext/MediaStreamGraph leaks:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eec997e8ef98

Perhaps bug 1196632 may be helpful in debugging if this reproduces.

I don't have time to look into AudioChannelService lifetimes, sorry.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

If it was exposed by my patch it's likely that the leaked objects are being rooted by an xpcom-shutdown observer and only being released once the observer service is shut down.  But by then it's too late to run the events that media code dispatches before the final cycle collection.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> If it was exposed by my patch it's likely that the leaked objects are being
> rooted by an xpcom-shutdown observer and only being released once the
> observer service is shut down.  But by then it's too late to run the events
> that media code dispatches before the final cycle collection.

FWIW, MediaStreamGraph observes "xpcom-shutdown" but unregisters when
notified, which is before the observer service is shut down.

I guess it might be conceivable though that something is somehow triggering a registration after "xpcom-shutdown".

Comment 8

[Karl Tomlinson (:karlt)]

I can reproduce something similar with

./mach mochitest --keep-open=false dom/media/webaudio/test/test_audioContextSuspendResumeClose.html

and all subtests disabled except for testMultiContextOutput.

During FreeSnowWhite(true) from nsCycleCollector::Shutdown() AudioNodes get
destroyed and AudioDestinationNode::SetIsOnlyNodeForContext() calls
nsContentUtils::RunInStableState() with a runnable holding a reference to the
AudioDestinationNode.

CycleCollectedJSRuntime holds onto the runnable until CycleCollectedJSRuntime is destroyed.  CycleCollectedJSRuntime is owned by XPCJSRuntime by nsXPConnect released during nsComponentManagerImpl::Shutdown().

References get released at that stage but cycle collection doesn't happen
again and so the objects in a cycle (AudioDestinationNode and AudioContext are
not released).

I don't know how this worked before 
https://hg.mozilla.org/mozilla-central/rev/a13c1f26e351
but there's not much point running SetIsOnlyNodeForContext() after
AudioContext shutdown anyway because all streams are suspended at that point, so we could avoid the issue by skipping the call.

Comment 9

[Karl Tomlinson (:karlt)]

This didn't leak before
https://hg.mozilla.org/mozilla-central/rev/a13c1f26e351#l1.16
because nsContentUtils::RunInStableState() failed and released the runnable when the service manager had been shut down, which happens before the last cycle collection.

I filed bug 1200514 on solving the core problem and I'll attach a patch to work around the problem here.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: skip SetIsOnlyNodeForContext after shutdown

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/854649a96d72
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa66f3e9300c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/854649a96d72