1024226 - moz_gdk_pixbuf_to_channel leaks |buf| if |do_CreateInstance("@mozilla.org/io/string-input-stream;1", &rv);| fails

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Andrew McCreight [:mccr8]]

http://mxr.mozilla.org/mozilla-central/source/image/decoders/icon/gtk/nsIconChannel.cpp#125

Comment 2

[Walter Litwinczyk [:walter]]

Attachment: skia_bug_1024226

Hi Andrew,

Would you mind seeing if this patch fixes the leak?

Comment 3

[Andrew McCreight [:mccr8]]

As I said in the other bug, I don't have a test case or anything, but it looks like it would fix the leak to me.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8453204 [details] [diff] [review]
skia_bug_1024226

Looks good to me.  Didn't check - NS_Free nulls out buf?  I know we return right away, but I don't like non-null pointers to freed memory :)

Comment 5

[Walter Litwinczyk [:walter]]

NS_Free appears to forward to regular free, so no zeroing of buf.

I was going to make the change, but it's declared as a read-only variable:

http://dxr.mozilla.org/mozilla-central/source/image/decoders/icon/gtk/nsIconChannel.cpp#86

in order to verify that the copying below worked correctly.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8453204 [details] [diff] [review]
skia_bug_1024226

Review of attachment 8453204 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/icon/gtk/nsIconChannel.cpp
@@ +132,1 @@
>    NS_ENSURE_SUCCESS(rv, rv);

You might as well move this return below the NS_Free(buf) instead of having us check rv again.

@@ +132,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    rv = stream->AdoptData((char*)buf, buf_size);
>    NS_ENSURE_SUCCESS(rv, rv);

This NS_ENSURE_SUCCESS should also be:

if (NS_FAILED(rv)) {
    NS_Free(buf);
    return;
}

to avoid leaking here.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8453204 [details] [diff] [review]
> skia_bug_1024226
> 
> Review of attachment 8453204 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/decoders/icon/gtk/nsIconChannel.cpp
> @@ +132,1 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> You might as well move this return below the NS_Free(buf) instead of having
> us check rv again.
> 
> @@ +132,4 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    rv = stream->AdoptData((char*)buf, buf_size);
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> This NS_ENSURE_SUCCESS should also be:
> 
> if (NS_FAILED(rv)) {
>     NS_Free(buf);
>     return;
> }
This takes care of my comment as well.

Comment 8

[Walter Litwinczyk [:walter]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 8453204 [details] [diff] [review]
> skia_bug_1024226
> 
> Review of attachment 8453204 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +132,4 @@
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    rv = stream->AdoptData((char*)buf, buf_size);
> >    NS_ENSURE_SUCCESS(rv, rv);
> 
> This NS_ENSURE_SUCCESS should also be:
> 
> if (NS_FAILED(rv)) {
>     NS_Free(buf);
>     return;
> }
> 
> to avoid leaking here.

I think it shouldn't be freed because AdoptData will transfer ownership to the stream:

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsIStringStream.idl#39

Comment 9

[Walter Litwinczyk [:walter]]

Attachment: leak_bug_1024226

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Walter Litwinczyk [:walter] from comment #8)
> 
> I think it shouldn't be freed because AdoptData will transfer ownership to
> the stream:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsIStringStream.idl#39

Does the ownership still transfer if AdoptData fails?

Comment 11

[Walter Litwinczyk [:walter]]

It doesn't appear that AdoptData can fail:

http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#156
http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.cpp#447

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(In reply to Walter Litwinczyk [:walter] from comment #11)
> It doesn't appear that AdoptData can fail:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsStringStream.cpp#156
> http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.
> cpp#447

Maybe add a comment about that where we check for failure then

Comment 13

[Walter Litwinczyk [:walter]]

Attachment: leak_bug_1024226

I left the NS_ENSURE_SUCCESS() so it would print something in non-debug builds.

Comment 14

[Walter Litwinczyk [:walter]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=410e4a82baa3

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d8154a37ebb

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d8154a37ebb