71005 - Memory leak in implementation of nsJARURI

Status: RESOLVED FIXED

(Core :: Networking, defect, P4)

Description

[Marcus Webster]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; 0.8) Gecko/20010215
BuildID:    2001021508

Look in function nsJARURI::Equals

the URI passed to the argument is QueryInterface'd to a nsIJARURI and assigned
to a plain old nsIJARURI pointer, otherJar - not an nsCOMPtr. The reference is
then never released. 

Reproducible: Didn't try
Steps to Reproduce:
I have read the code - I have not tried to measure how much memory this code is
leaking.

The questionable code is at...
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/jar/src/nsJARURI.cpp#255

Comment 1

[Keyser Sose]

Marking NEW.

Comment 2

[benc]

mass move, v2.
qa to me.

Comment 3

[Vinay Badami]

Attachment: proposed fix

Assign the results of the Queryinterface to a nsCOMPtr<nsIJARURI>.

Comment 4

[neeti]

to darin for review of patch

Comment 5

[Darin Fisher]

Comment on attachment 60313 [details] [diff] [review]
proposed fix

>Index: src/nsJARURI.cpp

>-    nsJARURI* otherJAR;
>-    rv = other->QueryInterface(NS_GET_IID(nsIJARURI), (void**)&otherJAR);
>+    nsCOMPtr<nsIJARURI> otherJAR = nsnull;
>+    nsJARURI* tempPtr = nsnull;
>+    rv = other->QueryInterface(NS_GET_IID(nsIJARURI), (void**)&tempPtr);
>     if (NS_FAILED(rv))
>         return NS_OK;   // not equal
>+    else 
>+        otherJAR = tempPtr;

actually, this doesn't fix the leak.  tempPtr still holds a reference, and
assignment into a COMPtr adds one more reference.

the correct solution would be something like this:

nsCOMPtr<nsIJARURI> otherJAR( do_QueryInterface(other, &rv) );
if (NS_FAILED(rv))
    return rv;

Comment 6

[Darin Fisher]

vinay: your patch needs work

Comment 7

[Vinay Badami]

Attachment: use nsCOMPtr<nsIJARURI> otherJAR(do_QueryInterface(other, &rv));

Comment 8

[Darin Fisher]

vinay: wrong patch :P

Comment 9

[Vinay Badami]

Attachment: correct patch

Opps - attaching the new patch file.
Sorry for the confusion.

Comment 10

[Darin Fisher]

Comment on attachment 60469 [details] [diff] [review]
correct patch

sr=darin

Comment 11

[Darin Fisher]

Comment on attachment 60469 [details] [diff] [review]
correct patch

sr=darin

Comment 12

[gordon]

Comment on attachment 60469 [details] [diff] [review]
correct patch

Looks good.  r=gordon.

Comment 13

[Darin Fisher]

fix checked in.