Last Comment Bug 857653 - Audio channel agents leak the world
: Audio channel agents leak the world
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 826410 859244
Blocks: 844323
  Show dependency treegraph
 
Reported: 2013-04-03 10:29 PDT by Justin Lebar (not reading bugmail)
Modified: 2013-04-18 21:32 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
wontfix
fixed
fixed
wontfix
fixed


Attachments
Patch, v1 (11.01 KB, patch)
2013-04-03 12:06 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review+
Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2013-04-03 10:29:00 PDT
In bug 844323 I'm writing unit tests which exercise the process priority manager.

As part of this effort, I modified the audio channel agent code in nsHTMLMediaElement to be controlled by a pref, instead of #ifdef B2G.  I then flip this pref in my test.

I discovered that flipping this pref and playing an <audio> is enough to leak a page until shutdown.

Looking at the audio code, an AudioChannelAgent holds a strong reference to its <audio> element.  So this means that the window which contains the <audio> can be GC'ed only once the AudioChannelAgent is destroyed.  Leak the AudioChannelAgent and you leak its window.

It's not clear to me exactly how we're leaking the AudioChannelAgent, but it's happening somehow.

I have the testcase, so I'll write a fix.
Comment 1 Justin Lebar (not reading bugmail) 2013-04-03 12:06:20 PDT
Created attachment 732965 [details] [diff] [review]
Patch, v1
Comment 2 Justin Lebar (not reading bugmail) 2013-04-03 12:07:23 PDT
This fixes it for me.  There must have been a cycle somewhere.

Even if there isn't a cycle, holding a weak ref seems sane here; we don't want the AudioChannelAgent to keep the node alive, I think.
Comment 3 Justin Lebar (not reading bugmail) 2013-04-03 12:08:29 PDT
Blocks a blocker.  This is also a pretty bad leak.
Comment 4 Justin Lebar (not reading bugmail) 2013-04-03 12:09:32 PDT
> This fixes it for me.

To be clear, we no longer leak the window or the AudioChannelAgent with this patch.
Comment 5 Boris Zbarsky [:bz] 2013-04-03 12:09:47 PDT
Comment on attachment 732965 [details] [diff] [review]
Patch, v1

The IDL should document that the passed-in object needs to implement nsISupportsWeakReference to actually work.

r=me with that
Comment 6 Justin Lebar (not reading bugmail) 2013-04-03 12:14:24 PDT
And how about we make nsHTMLMediaElement implement nsISupportsWeakReference while we're at it?  Apparently this is not tested so well...
Comment 7 Boris Zbarsky [:bz] 2013-04-03 12:16:43 PDT
> And how about we make nsHTMLMediaElement implement nsISupportsWeakReference

All elements implement nsISupportsWeakReference, via a tearoff.  See http://hg.mozilla.org/mozilla-central/file/f20b0ce9e528/content/base/src/FragmentOrElement.cpp#l1698
Comment 8 Justin Lebar (not reading bugmail) 2013-04-03 12:18:04 PDT
Ah, perfect.  Thanks.
Comment 9 Daniel Coloma:dcoloma 2013-04-03 12:18:28 PDT
blocks a blocker
Comment 10 Justin Lebar (not reading bugmail) 2013-04-03 13:35:47 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3ce751dd8f
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-04-03 19:21:38 PDT
https://hg.mozilla.org/mozilla-central/rev/0a3ce751dd8f
Comment 13 Marco Chen [:mchen] 2013-04-08 00:46:23 PDT
Hi Justin,

I found that the code as below will return null for callback pointer. Could you help to take a look? Thanks. And this will cause media element malfunctional on AudioChannelService.

http://mxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelAgent.cpp#148
Comment 14 Marco Chen [:mchen] 2013-04-08 01:57:42 PDT
Hi Justing,

This is my fault on Bug 859244 - nsIAudioChannelAgentCallback didn't be added into nsHTMLMediaElement::QueryInterface().
Comment 15 Justin Lebar (not reading bugmail) 2013-04-08 06:30:04 PDT
Ah, I have unit tests for this, but I haven't run them on the b2g branch.  Thanks for catching this!

Note You need to log in before you can comment on or make changes to this bug.