nsBulletFrame::Paint() leaks an image...

VERIFIED FIXED

Status

()

Core
Layout
P1
critical
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: rpotts (gone), Assigned: rpotts (gone))

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: {rtm++] [have fix], URL)

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
Inside of nsBulletFrame::Paint(...) there is a code path which causes an 
nsIImage to leak.

To see the problem, load test1.html from the samples directory.  With bloat 
logging turned on, you will see an nsImageWin (on windows) being leaked at 
shutdown.  This is due to the image being loaded by the bullet frame...

This leak could result in serious resource leakage under Win95...

It appears that rev 1.35 tried to fix this problem, but it did not handle the 
common case :-(  The bug being refered to in that checkin was bug #19114...
(Assignee)

Comment 1

17 years ago
I'm attaching a patch which fixes the leak.  Basically the change is to use an 
nsCOMPtr to insure that the object is released...
Keywords: rtm
Priority: P3 → P1
Whiteboard: [have fix]
(Assignee)

Comment 2

17 years ago
Created attachment 16460 [details] [diff] [review]
proposed fix
r=jst
(Assignee)

Comment 4

17 years ago
I've checked the patch into the trunk. (sr=buster)

I'll also check it into the embedding branch shortly...

I'm waiting for approval to check it into the Seamonkey branch...
Whiteboard: [have fix] → {rtm+] [have fix]

Comment 5

17 years ago
marking rtm++
Whiteboard: {rtm+] [have fix] → {rtm++] [have fix]

Comment 6

17 years ago
This patch is good, however, prefer direct-initialization over
copy-initialization.  That is, use

  nsCOMPtr<nsIImage> image( dont_AddRef(mImageLoader.GetImage()) );

instead of the |=| form.  A good compiler will optimize the |=| form into the
right thing, but a _bad_ compiler may create a temporary and use the
copy-constructor.  Therefore, always prefer the direct-initialization form for
all user-defined types.  Fix that and it's sr=scc.
(Assignee)

Comment 7

17 years ago
hey buster,

Can you check this one into the N6 branch for me?  I don't have one of those 
trees :-(  I think that the bug is all ++'ed and ready to go :-)

I've already checked the patch into the trunk and the embedding branch.
Assignee: rpotts → buster

Comment 8

17 years ago
with pleasure...
Status: NEW → ASSIGNED

Comment 9

17 years ago
checked in on branch.  I think that means this is fixed (checked in 
everywhere), but not sure so assigning back to rpotts.
Assignee: buster → rpotts
Status: ASSIGNED → NEW

Comment 10

17 years ago
Marking fixed per above comments.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
Marking verified in the Oct 16th build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.