Audio channel agents leak the world

RESOLVED FIXED in Firefox 23

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla23
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

4 years ago
Created attachment 732965 [details] [diff] [review]
Patch, v1
Attachment #732965 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Blocks a blocker.  This is also a pretty bad leak.
blocking-b2g: --- → tef?
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 4

4 years ago
> This fixes it for me.

To be clear, we no longer leak the window or the AudioChannelAgent with this patch.
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
Attachment #732965 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

4 years ago
And how about we make nsHTMLMediaElement implement nsISupportsWeakReference while we're at it?  Apparently this is not tested so well...
> 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
(Assignee)

Comment 8

4 years ago
Ah, perfect.  Thanks.
blocks a blocker
blocking-b2g: tef? → tef+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a3ce751dd8f
https://hg.mozilla.org/mozilla-central/rev/0a3ce751dd8f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
https://hg.mozilla.org/releases/mozilla-b2g18/rev/da523063aa7b
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e43e65ec89d2
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → fixed
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed

Comment 13

4 years ago
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

4 years ago
Hi Justing,

This is my fault on Bug 859244 - nsIAudioChannelAgentCallback didn't be added into nsHTMLMediaElement::QueryInterface().
Depends on: 859244
(Assignee)

Comment 15

4 years ago
Ah, I have unit tests for this, but I haven't run them on the b2g branch.  Thanks for catching this!
(Assignee)

Updated

4 years ago
Depends on: 826410
You need to log in before you can comment on or make changes to this bug.