[MLK] Massive memory leak of buffers in ns4xPluginStreamListener::OnDataAvailable()

VERIFIED FIXED in mozilla0.8.1

Status

()

VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: beard, Assigned: serhunt)

Tracking

({memory-leak})

Trunk
mozilla0.8.1
PowerPC
Mac System 9.x
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: have patch ready for 0.8.1)

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
Every buffer that gets allocated in ns4xPluginStreamListener::OnDataAvailable() 
leaks. When testing the cache with the Quicktime plugin on the Mac, this 
generated a HUGE memory leak as it displayed a Quicktime movie. I have a patch, 
and this should probably go in to Mozilla 0.8.1, IMHO.
(Reporter)

Comment 1

18 years ago
Created attachment 27735 [details] [diff] [review]
Fix for memory leak and some additional error handling.
(Reporter)

Updated

18 years ago
Keywords: mlk
Target Milestone: --- → mozilla0.8.1

Comment 2

18 years ago
Some comments on the patch:

* maybe you should switch from PR_Free() to nsCRT::free() and PR_Malloc to
nsCRT::malloc()?

* please avoid goto's if possible

otherwise the patch looks good to me.
(Reporter)

Comment 3

18 years ago
The goto is legitimate, to implement a common exit point for early returns. If we 
had a generic class for managing dynamically allocated buffers, this could be 
used and early returns wouldn't require a goto. I was interested in creating a 
minimal patch.

Comment 4

18 years ago
Personally, I'd use nsMemory instead of nsCRT. What it comes down to is we have 
too many ways to manipulate memory (IMO). And yes, breaks could be used instead 
of gotos, but the final code will be essentially the same.

The patch is solid and a distinct improvement over the rather severe leakage 
currently happening. r=bnesse.

Comment 5

18 years ago
lets do it as soon as leaf is ready for checkin's
Whiteboard: have patch ready for 0.8.1
(Assignee)

Comment 6

18 years ago
a=av

Comment 7

18 years ago
nice! sr=waterson

Comment 8

18 years ago
let's get this checked in  a=asa (on behalf of drivers)
(Reporter)

Comment 9

18 years ago
So, who's checking this in to the trunk? Am I?
(Assignee)

Comment 10

18 years ago
I've done this. Marking fixed.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 11

18 years ago
verif(rubberstamp)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.