Closed Bug 593528 Opened 10 years ago Closed 10 years ago

Audio write API leaks memory at about 500KB/s

Categories

(Core :: Audio/Video, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: blizzard, Assigned: cpearce)

References

Details

Attachments

(1 file, 2 obsolete files)

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
How fast are we leaking?  Is it leaked even after you close the page?  Which OS is this?
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.
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.
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.
blocking2.0: --- → ?
Summary: listening to live html5 audio streams causes a few MB of memory to be leaked every minute → Audio write API leaks memory at about 500KB/s
Blocks: 490705
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.
Attached patch Patch 1 (obsolete) — Splinter Review
* 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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #472307 - Flags: review?(david.humphrey)
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #472307 - Attachment is obsolete: true
Attachment #472308 - Flags: review?(kinetik)
Attachment #472308 - Flags: feedback?(david.humphrey)
Attachment #472307 - Flags: review?(david.humphrey)
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?
Attached patch Patch v3Splinter Review
Attachment #472308 - Attachment is obsolete: true
Attachment #472318 - Flags: review?(kinetik)
Attachment #472318 - Flags: feedback?(david.humphrey)
Attachment #472308 - Flags: review?(kinetik)
Attachment #472308 - Flags: feedback?(david.humphrey)
Attachment #472318 - Flags: review?(kinetik) → review+
http://hg.mozilla.org/mozilla-central/rev/e0d98549c7ec
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #472318 - Flags: feedback?(david.humphrey)
You need to log in before you can comment on or make changes to this bug.