73308 - [MLK] Leaking nsCStringKey objects

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Patrick C. Beard]

Look at the blame URL above. On error return, you're leaking the |source| 
variable.

Comment 1

[Håkan Waara]

This may be a dumb question, but I can't see how it *ever* is deleted?

Comment 2

[Judson Valeski]

how was this leak detected? If we're erroring out in here, there's a bigger
problem than the leak. The mem gets cleaned up in the "delete cur;" stmt at the
bottom of the while loop. I ran this code through purify before checking it in,
so either there's some error case I'm not aware of, or this was detected
visually by looking at the code.

Comment 3

[Judson Valeski]

futuring unless someone can show me how to repro this.

Comment 4

[Patrick C. Beard]

The leak was detected with the Boehm GC leak detector™. On line 305, if the call 
to lBFSTable.Get(source) returns nsnull, then you clearly leak source. In 
addition, when you call grayQ.Push(source), does grayQ own it? If not, then you 
leak regardless.

Comment 5

[Judson Valeski]

I've acknowledged that the leak *can* happen (and yes the push moves ownership
to the deque), I'm wondering under what conditions cause it to leak. I've
purified this code and couldn't get it to leak. if it's leaking, we have a
bigger problem on our hands (broken stream converter graph construction), and
that's what I'm trying to get to.

Comment 6

[rbs]

Adding this excerpt for the record - coincidentally from brendan's patch on 
bug 72722
@@ -279,7 +264,8 @@
 // CONTRACTID should be made prior to calling this method in an attempt to find 
a direct
 // converter rather than walking the graph.
 nsresult
-nsStreamConverterService::FindConverter(const char *aContractID, nsCStringArray 
**aEdgeList) {
+nsStreamConverterService::FindConverter(const char *aContractID, nsCStringArray 
**aEdgeList)
+{
     nsresult rv;
     if (!aEdgeList) return NS_ERROR_NULL_POINTER;
 
@@ -295,14 +281,19 @@
     NS_ASSERTION(lBFSTable.Count() == vertexCount, "strmconv BFS table init 
problem");
 
     // This is our source vertex; our starting point.
-    nsCString fromC, toC;
+    nsCAutoString fromC, toC;
     rv = ParseFromTo(aContractID, fromC, toC);
     if (NS_FAILED(rv)) return rv;
 
     nsCStringKey *source = new nsCStringKey(fromC.get());
+    if (!source)
+        return NS_ERROR_OUT_OF_MEMORY;
 
     SCTableData *data = (SCTableData*)lBFSTable.Get(source);
-    if (!data) return NS_ERROR_FAILURE;
+    if (!data) {
+        delete source;
+        return NS_ERROR_FAILURE;
+    }
 

Comment 7

[rbs]

There is a continuation:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=29115 

Comment 8

[Judson Valeski]

once 72722 goes in, I'll close this bug out. However, I would still like to know
under what conditions this was being seen.

Comment 9

[Patrick C. Beard]

No telling what conditions. But leaks by inspection don't come often, take 
advantage when they do! I did observe it, but I didn't note what conditions 
caused it.

Comment 10

[Brendan Eich [:brendan]]

Jud, can you give the patch in 72722 that applies to the streamconv files an r=?
I eliminated other leaks and gratuitous allocations.  I hope I didn't bust anything?

/be

Comment 11

[Judson Valeski]

once brendan's in, this goes away.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See bug 75337 for another leak caused by early return from the same function
(with steps to reproduce that I just found).

Comment 13

[benc]

mass move, v2.
qa to me.

Comment 14

[Tanu Mutreja]

Attachment: Modified nsStreamConverterService.cpp for avoiding memory leaks.

Modified for following reasons:

1. At line# 224, condition 'if (-1 == fromLoc || -1 == toLoc ) return 
NS_ERROR_FAILURE;' 

never seems to be true if any or both substrings ('from=' / 'to=') are not
found as the 

values of fromLoc and toLoc are already increased before the check. So modified
it 
to 

increase the values after checking this condition.

2. At line 256, while checking the failure condition for 'data', we need to
delete 
'state'.

3. At line 275, added a new class 'CStreamConvDeallocator' inheriting 
'nsDequeFunctor'. This 

is required for using 'Erase()' function of 'nsDeque' class. 

4. Added '*aEdgeList = nsnull;' at line 285 for self deletion of aEdgeList in
failure 

situations.

5. At line 303, added the check for 'out of memory' condition while creation of

'source' 

ptr.

6. At line 305, if 'data' is null, 'source' should be deleted before returning
from the 

function.

7. At line 311, created 'grayQ', an object of nsDeque, with passing a not null
value 
of 

'dtorFunc'. 'dtorFunc' is pointing to an object of newly created 
'CStreamConvDeallocator' 

class. This is required for removing and deleting the queue elements by
'Erase()' 
function 

of nsDeque.

8. At line 318, if 'data' is null, we need to delete all the elements of
'grayQ', if there 

is any. Same is true for error check at line #330.

9. After creating a pointer 'curVertex' to an 'nsCStringKey' object, we need to

check this for 'OUT OF MEMORY' condition and erase 'grayQ' in that condition.

10. At line 359, finally assigning 'source' to be null.

11. At line 370, we need to check for 'out of memory' condition.

12. At line 377, we need to delete 'shortestPath'. Same is true for line# 316.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think there's any need for all the explicit |grayQ.Erase()| before
returning since nsDeque calls |Erase| from its destructor.

Comment 16

[Tanu Mutreja]

Attachment: Updating previous patch for incorporating David Baron's comments.

Comment 17

[Gagan]

Comment on attachment 62493 [details] [diff] [review]
Updating previous patch for incorporating David Baron's comments.

I had the same suggestions as dbaron's. good job! r=gagan

Comment 18

[Gagan]

jud: I am bringing this in for 0.9.8. and if you can't get it in let me
know/give it to me-- I'll land this.

Comment 19

[Tanu Mutreja]

Attachment: Updating the patch again.

I think I missed out few points in the last patch. Updating it for deleting
|data| at line#167, line#194 and |source| at line#333(if the instantiation of
'dtorFunc' fails due to 'out of memory' condition).

Please review this consolidated patch.

Thanks!

Comment 20

[Vinay Badami]

Need a r/sr on this one after the latest set of changes. 

Comment 21

[Gagan]

Comment on attachment 62702 [details] [diff] [review]
Updating the patch again.

r=gagan

Comment 22

[Brendan Eich [:brendan]]

Vinay, please see my rusting patch in bug 72722 -- look for the diffs for
nsStreamConverterService.cpp.  I'm sorry I didn't land just the leak fixes and
useless construction avoidance changes I made there, back when I was working on
that bug.  There is no need to new nsCStringKey in code that uses nsHashtable --
you declare a local (storage class auto) nsCStringKey to Get, and only if you
end up calling Put will it be Cloned.

There are other fixes in the nsStreamConverterService.cpp patch in bug 72722,
and I'm hopeful that the file has not changed so much that you can't quickly
take what is good from there and put it into a revised patch for this bug.  Thanks,

/be

Comment 23

[Tanu Mutreja]

Attachment: Including Brendan's modifications...

Included Brendan's suggestions from his patch for bug# 72722.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 65877 [details] [diff] [review]
Including Brendan's modifications...

Style nits only:

>+        nsCStringKey *key=(nsCStringKey*)anObject;

Spaces around = seem like prevailing style in this file, please uphold it.

>+    SCTableData(nsCStringKey& aKey):key(aKey){
>+        data.state = nsnull;
>+    }
>+};
> #endif // __nsstreamconverterservice__h___

Likewise, spaces around : and before { in the constructor head above would be
better.

Is this jud's code originally?	He may want to review.

/be

Comment 25

[Tanu Mutreja]

Attachment: included proper styles.

Comment 26

[Judson Valeski]

Comment on attachment 66124 [details] [diff] [review]
included proper styles.

>@@ -246,15 +233,17 @@
>     if (!BFSTable) return PR_FALSE;
> 
>     BFSState *state = new BFSState;
>-    if (!state) return NS_ERROR_OUT_OF_MEMORY;
>+    if (!state) return PR_FALSE;
> 
>     state->color = white;
>     state->distance = -1;
>     state->predecessor = nsnull;
> 
>-    SCTableData *data = new SCTableData;
>-    if (!data) return NS_ERROR_OUT_OF_MEMORY;
>-    data->key = (nsCStringKey*)aKey->Clone();
>+    SCTableData *data = new SCTableData(*NS_STATIC_CAST(nsCStringKey*, aKey));
>+    if (!data) {
>+        delete state;
>+        return PR_FALSE;
>+    }

This function returns a "nsresult". Why are you returning PR_FALSE? I think you
should continue to return NS_ERROR_OUT_OF_MEMORY.

> // walks the graph using a breadth-first-search algorithm which generates a discovered
> // verticies tree. This tree is then walked up (from destination ertex, to origin vertex)
> // and each link in the chain is added to an nsStringArray. A direct lookup for the given
>@@ -282,6 +277,7 @@
> nsStreamConverterService::FindConverter(const char *aContractID, nsCStringArray **aEdgeList) {
>     nsresult rv;
>     if (!aEdgeList) return NS_ERROR_NULL_POINTER;
>+    *aEdgeList = nsnull;

I'm concerned that you're null'ing the out-param here. These kinds of semantic
changes can have ugly ripple effects later on. Is there a need for this change?
If so, please add a comment to the code indicating that the out param get's
modified no matter what.

Comment 27

[Tanu Mutreja]

In the latest code that method is returning PRBool. It's declared as
"static PRBool PR_CALLBACK InitBFSTable(nsHashKey *aKey, void *aData, void* 
closure)". So returning PR_FALSE instead of NS_ERROR_OUT_OF_MEMORY should be 
fine.

And for the second suggestion, I understand your concern but in this particular 
case, addition of  ">+    *aEdgeList = nsnull;" does not seem to create any 
ripple effect.

Comment 28

[Judson Valeski]

Comment on attachment 66124 [details] [diff] [review]
included proper styles.

thanks for clearing those issues up.

Comment 29

[Judson Valeski]

re-assigning. this looks ready to land once it has sr.

Comment 30

[Brendan Eich [:brendan]]

Comment on attachment 66124 [details] [diff] [review]
included proper styles.

Sorry, I thought I sr'd this already.  sr=[e-mail].

/be

Comment 31

[Brendan Eich [:brendan]]

The patch has r= and sr=, can we get it in for 0.9.9?  I'm worrying someone lost
track and will forget.  I'm assuming people read their bugmail.

/be

Comment 32

[Vinay Badami]

Fixed with checkin
cvs commit: Examining .
Checking in nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  <--  nsS
treamConverterService.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in nsStreamConverterService.h;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.h,v  <--  nsStr
eamConverterService.h
new revision: 1.14; previous revision: 1.13
done

Comment 33

[Boris Zbarsky [:bzbarsky]]

This checkin is causing crashes on file save (bug 126311)

Comment 34

[Vinay Badami]

Rolled back changes and reopening bug.
C:\mozilla\netwerk\streamconv\src>cvs commit nsStreamConverterService.h nsStream
ConverterService.cpp
Checking in nsStreamConverterService.h;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.h,v  <--  nsStr
eamConverterService.h
new revision: 1.15; previous revision: 1.14
done
Checking in nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  <--  nsS
treamConverterService.cpp
new revision: 1.54; previous revision: 1.53
done

Comment 35

[Brendan Eich [:brendan]]

Code review without enough testing coverage isn't going to find all problems. 
Still, with the test results now in hand, can we have a fixed fix for 0.9.9?

/be

Comment 36

[Tanu Mutreja]

I'm working on it to figure out the cause...

Comment 37

[Tanu Mutreja]

Attachment: Modified Patch.

The problem seems to be caused as a result of having a nsCStringKey in
SCTableData and there being no copy constructor defined for this class. Hence
the default bitwise copy ctor was getting applied. The end result was that the
nsCStringKey objects in mAdjacency list were pointing to invalid addreses. 
Changing back to using nsCStringKey* in SCTableData solves the problem.

This patch was working fine on WinNT for the whole day. My Linux build is still
going on. 
Tested the new patch with test cases attached to the Mozilla Tree + all the
url's mentioned in this bug and the ones marked as duplicate to this bug.

Comment 38

[Tanu Mutreja]

Tested the latest patch(id=70728) on Linux. It works fine on Linux too.

Comment 39

[Vinay Badami]

*** Bug 126311 has been marked as a duplicate of this bug. ***

Comment 40

[Judson Valeski]

Comment on attachment 70728 [details] [diff] [review]
Modified Patch.

>Index: nsStreamConverterService.cpp
>===================================================================

<snip>

>+class CStreamConvDeallocator:public nsDequeFunctor {
please add a space before and after the colon for consistencies sake. from
..."r:p"... to ..."r : p"...

>+
>+    source = nsnull;
>+

I believe the null'ing of source is unneccessary; right?

>         // If this vertex isn't in the BFSTable, then no-one has registered for it,
>         // therefore we can't do the conversion.
>-        *aEdgeList = nsnull;
>+        delete shortestPath;
>         return NS_ERROR_FAILURE;
>     }
> 

>+    delete shortestPath;
>     *aEdgeList = nsnull;
>     return NS_ERROR_FAILURE; // couldn't find a stream converter or chain.

Here' you leave the nulling of *aEdgeList, yet, above you remove it. please
pick one or the other and be consistent.

>Index: nsHashtable.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.h,v
>retrieving revision 1.47
>diff -u -r1.47 nsHashtable.h
>--- nsHashtable.h       2001/12/02 23:17:11     1.47
>+++ nsHashtable.h       2002/02/21 14:43:32
>@@ -359,6 +359,9 @@
>     char*       mStr;
>     PRUint32    mStrLen;
>     Ownership   mOwnership;
>+
>+  private:
>+    nsCStringKey(nsCStringKey& a);
> };

because you've reverted to nsCStringKey*, I don't think the copy constructor is
needed here, correct, or did I miss something?

Comment 41

[Tanu Mutreja]

Attachment: Including Judson's Comments.

Changes:
1. Added space before and after |:| in
>+class CStreamConvDeallocator:public nsDequeFunctor {
2. Removed 
>+
>+    source = nsnull;
>+
3. As we are already nulling *aEdgeList in the beginning of the
FindConverter(), and *aEdgeList is not getting modified anywhere else in the
error path, removed *aEdgeList = nsnull; from the end.
4. Still keeping the copy constructor as this will help preventing anyone else
running into the same bug that we did.

Comment 42

[Brendan Eich [:brendan]]

Sorry I didn't spot the problem -- my patch in bug 72722, where I got rid of the
new nsCStringKey constructions before nsHashtable.Get calls in this stream-conv
code, did indeed add copy constructors for nsCStringKey and other nsHashKey
subclasses.

Now that you've added those, why not avoid the new nsCStringKey before Get perf
and (minor) footprint hit?  Only if Get misses will you need to copy-construct
an nsCStringKey in the state record, right?

/be

Comment 43

[Judson Valeski]

Comment on attachment 71058 [details] [diff] [review]
Including Judson's Comments.

looks good to me. r=valeski

Comment 44

[Tanu Mutreja]

Attachment: Including Brendan's suggestions.

Avoiding new nsCStringKey() before mAdjacencyList->Get(). Only when it fails in
the sense that fromEdges is null and we need to do mAdjacencyList->Put(), we do
a new.

Brendan, we have just declared the nsCStringKey copy constructor as private.
Should we define it the way you did in your patch?

Comment 45

[Mitchell Stoltz (not reading bugmail)]

nsbeta1+

Comment 46

[Brendan Eich [:brendan]]

I think it would be best to revive the nsHashtable.h/cpp copy constructors from
bug 72722's patch, before they're lost from memory.  Not just for nsCStringKey,
either -- what's good for it is good for other nsHashKey subclasses that cannot
use the default (memcpy) copy-constructor.  I'm turning over sr= duties to
shaver though (busy this week).

/be

Comment 47

[Tanu Mutreja]

Attachment: Including copy constructors.

Included copy constructors for:
nsCStringKey
nsStringKey
nsOpaqueKey
nsISupportsKey
nsVoidKey
nsIDKey

Comment 48

[Judson Valeski]

Comment on attachment 71486 [details] [diff] [review]
Including copy constructors.

looks good to me. FYI. when one patch superceeds another, please mark the
previous patch as obsolete.

Comment 49

[Mike Shaver (:shaver emeritus)]

Comment on attachment 71486 [details] [diff] [review]
Including copy constructors.

>+        if(predecessor)
>+            delete predecessor;

There's no need to check nullness here: deleting 0 is guaranteed to be safe,
specifically to avoid the need for such checks.

>+#ifdef DEBUG
>+    mKeyType = CStringKey;
>+#endif

Unconditional initialization wants to be in an initializer list, I think:

    : mStr(aKey.mStr), mStrLen(aKey.mStrLen), mOwnership(aKey.mOwnership)
#ifdef DEBUG
  mKeyType(CStringKey)
#endif
{
    char *str = ...;

(Ditto for the other key types.)

Looks good other than that, though a comment describing how the combination
of ownership settings and string duplication in the copy constructors can't
result in us leaking the initial mStr would be a nice addition;  I had to
stare at the code for quite a while to assure myself that it was the case.

sr=shaver, please make the above changes before checking in.  (If you want
me to sr the resulting patch, please remember to cc: [e-mail]
on your mail: I'm swamped with review and approval mail, and that helps my
filters help me cope.)

Comment 50

[Tanu Mutreja]

Attachment: Including Shaver's comments during sr.

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 72016 [details] [diff] [review]
Including Shaver's comments during sr.

Carrying r=/sr= forward.

Noticed this typo triplicated in nsHashtable.cpp:

+// destuctor here is freeing mStr is that case, mStr is NOT getting leaked
here.

s/destuctor/destructor/

Also the third copy of this comment talks about nsOpaqueKey, which has an mBuf,
not an mStr.

Fix that nit and a=[e-mail] for 0.9.9.

/be

Comment 52

[Brendan Eich [:brendan]]

Approving for 0.9.9 (with the comment typo and copy/paste fix).

/be

Comment 53

[Tanu Mutreja]

Attachment: Corrected the typo's..

Comment 54

[Brendan Eich [:brendan]]

Looks great -- no need for another patch in the bug, but if you like, fix the
old misspelling of vertices ("verticies") in unaltered lines visible in the diff
-u, and for extra points, line up the member names in

+struct BFSState {
     BFScolors   color;
     PRInt32     distance;
-    nsHashKey  *predecessor;
-} BFSState;
+    nsCStringKey  *predecessor;

so each starts in the same column.  Thanks,

/be

Comment 55

[Tanu Mutreja]

Fixed with following check-in details:
C:\mozilla>cvs commit xpcom/ds netwerk/streamconv
cvs commit: Examining xpcom/ds
cvs commit: Examining netwerk/streamconv
cvs commit: Examining netwerk/streamconv/converters
cvs commit: Examining netwerk/streamconv/public
cvs commit: Examining netwerk/streamconv/src
cvs commit: Examining netwerk/streamconv/test
Checking in xpcom/ds/nsHashtable.cpp;
/cvsroot/mozilla/xpcom/ds/nsHashtable.cpp,v  <--  nsHashtable.cpp
new revision: 3.55; previous revision: 3.54
done
Checking in xpcom/ds/nsHashtable.h;
/cvsroot/mozilla/xpcom/ds/nsHashtable.h,v  <--  nsHashtable.h
new revision: 1.48; previous revision: 1.47
done
Checking in netwerk/streamconv/src/nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  
<--  nsS
treamConverterService.cpp
new revision: 1.55; previous revision: 1.54
done
Checking in netwerk/streamconv/src/nsStreamConverterService.h;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.h,v  
<--  nsStr
eamConverterService.h
new revision: 1.16; previous revision: 1.15
done

Comment 56

[benc]

This is a "code change" bug, so if Tanu can provide a bonzai query that shows
the check, then this can be marked VERIFIED.

Comment 57

[Tanu Mutreja]

Benjamin, Comment#55 above mentions the complete revision history for this 
check-in. Also r=, sr=, a= and patch= are kept as check-in comments. 
Please let me know if something else is required here otherwise if it is 
sufficient, please verify this.

Comment 58

[benc]

VERIFIED: per owner.