593528 - Audio write API leaks memory at about 500KB/s

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christopher Blizzard (:blizzard)]

I've been seeing memory leaks in my browser.  I listen to a lot of live html5 audio streams (ogg vorbis.)  Turns out I can watch memory be leaked as long as the streams are running.

Steps to reproduce:

1. Load html5radio.com
2. Click on Party 107
3. Watch memory get used

Comment 1

[Boris Zbarsky [:bzbarsky]]

How fast are we leaking?  Is it leaked even after you close the page?  Which OS is this?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

I don't see a leak using [Mozilla/5.0 (Windows NT 6.1; rv:2.0b4) Gecko/20100818 Firefox/4.0b4]. That's on Win7 32bit.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Hmm, but I saw ever increasing memory usage on [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6pre) Gecko/20100902 Firefox/4.0b6pre], that's trunk on Win7 64bit.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for catching this blizzard!

This regressed on trunk between [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre]  and [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100826 Minefield/4.0b5pre]. This means it's probably a regression from bug 490705, the audio write API, which landed at Wed Aug 25 09:10:00 2010 PDT.

This bug affects video as well as audio.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem is in nsBuiltinDecoder::AudioAvailable(). This gets passed in a float* of audio data to pass on to the listeners. AudioAvailable() bails out if the element doesn't have any listeners, and we don't free the audio data if we bail out.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1

* Store references to the framebuffer in nsAutoArrayPtrs so that if we fail and exit, or otherwise exit (say if we don't have listeners) from the functions handling the framebuffer, it will be released. This fixes the leaks for me.
* Add MOZ_COUNT_{C,D}TOR to the ctor and dtors of the objects used in the audio available functionality to ensure we're not leaking those objects. They're not leaking BTW. :)
* Make the dtors for the object used in the audio available functionality virtual, to ensure the right one gets called.

Comment 7

[cajbir (:cajbir)]

(In reply to comment #6)
> Created attachment 472307 [details] [diff] [review]
> * Make the dtors for the object used in the audio available functionality
> virtual, to ensure the right one gets called.

I think you'll find the base classes had virtual destructors so the correct destructors were being called.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

doublec is totally right, the extra virtuals are not needed. This patch removes the unnecessary virtuals. Switching review to kinetik, because it's Dave's day off, so we can land this quicker.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 472308 [details] [diff] [review]
Patch v2

I think you need to perform the same surgery on 
nsHTMLMediaElement::DispatchAudioAvailableEvent and nsHTMLMediaElement::NotifyAudioAvailable.

Other than that. can you please add a comment to each of the changed methods explaining the ownership of that data?

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/e0d98549c7ec