63549 - Need API to tell nsIImage to release bits obtained by GetBits()

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Mike Pinkerton (not reading bugmail)]

nsIImage::GetBits() may cause memory to be allocated to convert from a
device-dependent bitmap to a device-independent bitmap. Clients need a way to
tell the image that these bits are no longer needed and can be disposed of.

From kmcclusk in bugscape bug 3322:

We also need to file a separate bug that we need a way to release the DIB memory
on WIN32 when we are through with it. We could have a generic ReleaseBits which
releases the mImageBits when the following conditions are true:

if (mIsOptimized == PR_TRUE){
if (mHBitmap != nsnull){
if (mImageBits != nsnull) {
delete [] mImageBits;
mImageBits = nsnull;
}
}
}

Comment 1

[dcone (gone)]

We have a LockImagePixels and an UnLockImagePixels mechanism.  I think that if 
someone wants the bits safely.. they can lock this object.. and when they are 
finished with the bits.. unlock should be called.  When the nsImage is 
unlocked.. it is free to compress the image or get rid of unessisery memory.  
Any objections to this method.  

Comment 2

[Mike Pinkerton (not reading bugmail)]

sounds good to me, just make sure it's well commented in the docs that this
buffer can go away at any time unless the image pixels are locked.

Comment 3

[dcone (gone)]

Attachment: diff file for the fix.

Comment 4

[Kevin McCluskey (gone)]

Your patch is missing the diff for nsImageWin.h. It does not compile after
applying the patch. It seems odd that LockImagePixels sets mIsLocked to
PR_FALSE. Shouldn't it set it to PR_TRUE? It seems like it would be more natural
for the Creation of the DIB to happen in LockImagePixels instead of GetBits.
Would it be possible to move the Creation of the DIB to LockImagePixels and
throw an ASSERTION if GetBits is called and mLocked is set to PR_FALSE? This
would force the caller to do a LockImagePixels before calling GetBits.

Do really need to keep a separate mDIBTemp flag. Couldn't we always get rid of
the DIB if there is a DDB for the image. I'm assuming that we never need both a
DIB and DDB for the image unless it was created temporarly for printing or
because someone temporary needs to get at the bits to copy them to the clipboard. 

Comment 5

[buster]

sr=buster.  I don't fully understand what it's doing, but the mechanism looks
safe.  I don't see any place where you can get yourself into cycles, or leak,
etc.  I'll leave it to you to prove to your reviewer that it's correct for what
you're trying to accomplish.  If you do the review in SD, I'm happy to sit in on
it as well.

One style note:  I dislike removing lines via comment, rather than just removing
them from the file.  The commented lines rarely have any benefit, and they just
make the code harder to read, give false positives on file searches, etc.  If
you're keeping around a line of code for historical purposes, that's why we have
a source code archive.  But that's all just my opinion.

Comment 6

[Kevin McCluskey (gone)]

r=[e-mail].

Comment 7

[dcone (gone)]

Checked in fixes

Comment 8

[Chris Petersen]

Marking verified per last comments.