208704 - Memory leaks in DOM Inspector's PNG Encoder

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Glenn Randers-Pehrson]

I think there's a memory leak in the DOM Inspector.  In inPNGEncoder.cpp, there
is no png_destroy_write_struct() to match the png_create_write_struct(). 
Immediately after the png_write_end() near line 101, the following line should
be added:

   png_destroy_write_struct(&pngStruct, &infoStruct);

See example.c that comes with the libpng distribution.

Glenn

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

It looks like we also leak here:

 80   FILE *file = fopen(ToNewCString(str), "wb");

Comment 2

[[e-mail]]

there's another unrelated string leak at
http://lxr.mozilla.org/seamonkey/source/extensions/inspector/base/src/inPNGEncoder.cpp#78:

 78   nsAutoString str;
 79   str.Assign(aURL);
 80   FILE *file = fopen(ToNewCString(str), "wb");

perhaps this should just be

      FILE *file = fopen(NS_LossyConvertUCS2toASCII(aURL).get(), "wb");

(presuming it really does want an ASCII string and not UTF8). caillon, wanna fix
that while you're in there?

Comment 3

[[e-mail]]

ah caillon beat me to it ;)

Comment 4

[Glenn Randers-Pehrson]

There are some opportunities to improve the encoding speed of imPNGEncoder and
reduce its resource consumption.  Do you want those suggestions here or in a
separate bug?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

dwitte: it seems doubtful to me that it really wants to open an url

that should really use OpenANSIFileDesc from nsILocalFile, to work with
non-ascii paths.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Glenn, please use a new bug for that.  Do you want to produce a patch for the leaks?

Comment 7

[Glenn Randers-Pehrson]

Attachment: Patch for memory leak described in comment #1

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Please patch both leaks.  and use diff -u

Comment 9

[Glenn Randers-Pehrson]

Sorry, the *file leak is a bit out of my territory.  I'd probably botch the fix.
Whoever does fix it can combine my one-liner with their fix.

Comment 10

[[e-mail]]

Attachment: fix for both leaks

switches over to nsILocalFile per biesi's request. includes the png writer leak
fix.

Comment 11

[[e-mail]]

Attachment: diff -w of same

Comment 12

[[e-mail]]

Comment on attachment 125276 [details] [diff] [review]
diff -w of same

is the localfile fu okay?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 125276 [details] [diff] [review]
diff -w of same

o) please rename aURL to aPath, in the .idl and the .cpp, because it is no URL

o) please declare |rv| on the line where it is used

o) maybe consider changing aURL to be an |AString|

o) Is there a special reason you don't want to follow symlinks? (PR_FALSE for
second argument to NewLocalFile)

r=biesi on the patch with the above changes (except possibly point 3)

However, please get caillon to review this too, as he is the module owner.

Comment 14

[[e-mail]]

> Is there a special reason you don't want to follow symlinks? (PR_FALSE for
> second argument to NewLocalFile)

thanks for catching that... I saw it was unused in the unix localfile impl, so I
didn't think it mattered - but the other impls do use it ;)

> However, please get caillon to review this too, as he is the module owner.

yeah, I know caillon's the mo, I just wanted you to look at the patch first
since you're the one who wanted it. fair's fair. ;)

thx for the review!

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 125275 [details] [diff] [review]
fix for both leaks

>+  nsresult rv;
>+  nsCOMPtr<nsILocalFile> localFile;
>+  FILE *file;
>+  rv = NS_NewLocalFile(nsDependentString(aURL), PR_FALSE, getter_AddRefs(localFile));

You know the drill.  Declare rv while assigning instead of way up above.

>+  if (NS_SUCCEEDED(rv))
>+    rv = localFile->OpenANSIFileDesc("wb", &file);

Please fix this to use braces (opening brace on the same line as the if), even
for one-liners.  Ad infinitum.


>+
>+  ReverseRGB(width, height, bits);
>+  PRUint8* rowPtr = bits;

Earlier, you use |Type *ident| and here it is |Type* ident|.  Consistency would
totally rock here.  I prefer putting the * on the identifier to avoid things
such as someone expecting |Type* foo, bar|; to be two pointers.

>+  for (PRUint32 row = 0; row < height; ++row) {
>+    png_write_row(pngStruct, rowPtr);
>+    rowPtr += width*3;

Nit: space out the multiplication's operands?


Biesi said r=biesi which is fine, go ahead and have at that, with the
additional nits I pointed out fixed.

Comment 16

[[e-mail]]

Attachment: fix v2

ugh, so I looked at the js that calls this method, and it has an nsILocalFile
all along. so I changed the interface to take a localfile directly... does that
sound ok?

fixed nits in the rest of the file and cleaned up some #includes as well.

Comment 17

[[e-mail]]

Comment on attachment 125427 [details] [diff] [review]
fix v2

caillon said r=biesi is fine.

(this is untested, btw... you're on windows right? any chance you could have a
quick poke?)

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 125427 [details] [diff] [review]
fix v2

ah, this sounds like an even better solution

I'm usually on linux... while I could test on windows, I don't currently have
an up-to-date and working build there, and have no time to make one currently,
so I'm sorry but I can't test the patch on windows

Comment 19

[[e-mail]]

Comment on attachment 125427 [details] [diff] [review]
fix v2

thanks biesi (np about the testing, i'll figure something out)

jst - any chance you can do the honors here? fixes a couple of leaks and cleans
up some stuff.

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 125427 [details] [diff] [review]
fix v2

sr=jst

Comment 22

[[e-mail]]

checked in.