Closed Bug 73308 Opened 24 years ago Closed 23 years ago

[MLK] Leaking nsCStringKey objects


(Core :: Networking, defect)

Not set





(Reporter: beard, Assigned: t_mutreja)




(Keywords: memory-leak, verifyme, Whiteboard: [patch] [needs sr=])


(1 file, 10 obsolete files)

Look at the blame URL above. On error return, you're leaking the |source| 
Keywords: mlk
This may be a dumb question, but I can't see how it *ever* is deleted?
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.
futuring unless someone can show me how to repro this.
Target Milestone: --- → Future
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.
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.
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.
-nsStreamConverterService::FindConverter(const char *aContractID, nsCStringArray 
**aEdgeList) {
+nsStreamConverterService::FindConverter(const char *aContractID, nsCStringArray 
     nsresult rv;
     if (!aEdgeList) return NS_ERROR_NULL_POINTER;
@@ -295,14 +281,19 @@
     NS_ASSERTION(lBFSTable.Count() == vertexCount, "strmconv BFS table init 
     // 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;
+    }
once 72722 goes in, I'll close this bug out. However, I would still like to know
under what conditions this was being seen.
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.
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?

once brendan's in, this goes away.
Depends on: 72722
See bug 75337 for another leak caused by early return from the same function
(with steps to reproduce that I just found).
mass move, v2.
qa to me.
QA Contact: tever → benc
Blocks: 92580
No longer blocks: 92580
Modified for following reasons:

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

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

increase the values after checking this condition.

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

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


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



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


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

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

class. This is required for removing and deleting the queue elements by

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.
I don't think there's any need for all the explicit |grayQ.Erase()| before
returning since nsDeque calls |Erase| from its destructor.
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
Attachment #62493 - Flags: review+
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.
Target Milestone: Future → mozilla0.9.8
Attached patch Updating the patch again. (obsolete) — Splinter Review
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.

Need a r/sr on this one after the latest set of changes. 
Comment on attachment 62702 [details] [diff] [review]
Updating the patch again.

Attachment #62702 - Flags: review+
Attachment #62320 - Attachment is obsolete: true
Attachment #62493 - Attachment is obsolete: true
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,

Included Brendan's suggestions from his patch for bug# 72722.
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

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

Attached patch included proper styles. (obsolete) — Splinter Review
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.
Attachment #66124 - Flags: needs-work+
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 

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 on attachment 66124 [details] [diff] [review]
included proper styles.

thanks for clearing those issues up.
Attachment #66124 - Flags: needs-work+ → review+
re-assigning. this looks ready to land once it has sr.
Assignee: valeski → tmutreja
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Keywords: patch
Whiteboard: [patch needs sr=]
Comment on attachment 66124 [details] [diff] [review]
included proper styles.

Sorry, I thought I sr'd this already.

Attachment #66124 - Flags: superreview+
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.

Fixed with checkin
cvs commit: Examining .
Checking in nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  <--  nsS
new revision: 1.53; previous revision: 1.52
Checking in nsStreamConverterService.h;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.h,v  <--  nsStr
new revision: 1.14; previous revision: 1.13
Closed: 23 years ago
Resolution: --- → FIXED
This checkin is causing crashes on file save (bug 126311)
Rolled back changes and reopening bug.
C:\mozilla\netwerk\streamconv\src>cvs commit nsStreamConverterService.h nsStream
Checking in nsStreamConverterService.h;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.h,v  <--  nsStr
new revision: 1.15; previous revision: 1.14
Checking in nsStreamConverterService.cpp;
/cvsroot/mozilla/netwerk/streamconv/src/nsStreamConverterService.cpp,v  <--  nsS
new revision: 1.54; previous revision: 1.53
Resolution: FIXED → ---
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?

I'm working on it to figure out the cause...
Attached patch Modified Patch. (obsolete) — Splinter Review
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.
Tested the latest patch(id=70728) on Linux. It works fine on Linux too.
Whiteboard: [patch needs sr=] → [patch] [needs r/sr=]
*** Bug 126311 has been marked as a duplicate of this bug. ***
Comment on attachment 70728 [details] [diff] [review]
Modified Patch.

>Index: nsStreamConverterService.cpp


>+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?
Attachment #70728 - Flags: needs-work+
Attached patch Including Judson's Comments. (obsolete) — Splinter Review
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.
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

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?

Attachment #70728 - Attachment is obsolete: true
Attachment #70728 - Flags: needs-work+
Comment on attachment 71058 [details] [diff] [review]
Including Judson's Comments.

looks good to me. r=valeski
Attachment #71058 - Flags: review+
Attached patch Including Brendan's suggestions. (obsolete) — Splinter Review
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?
Attachment #71058 - Attachment is obsolete: true
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).

Attached patch Including copy constructors. (obsolete) — Splinter Review
Included copy constructors for:
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.
Attachment #71486 - Flags: review+
Attachment #62702 - Attachment is obsolete: true
Attachment #65877 - Attachment is obsolete: true
Attachment #66124 - Attachment is obsolete: true
Attachment #71285 - Attachment is obsolete: true
Whiteboard: [patch] [needs r/sr=] → [patch] [needs sr=]
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;

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

    : mStr(aKey.mStr), mStrLen(aKey.mStrLen), mOwnership(aKey.mOwnership)
#ifdef DEBUG
    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:
on your mail: I'm swamped with review and approval mail, and that helps my
filters help me cope.)
Attachment #71486 - Flags: superreview+
Attachment #71486 - Attachment is obsolete: true
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


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

Fix that nit and for 0.9.9.

Attachment #72016 - Flags: superreview+
Attachment #72016 - Flags: review+
Approving for 0.9.9 (with the comment typo and copy/paste fix).

Keywords: mozilla0.9.9+
Attachment #72016 - Attachment is obsolete: true
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,

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
Checking in xpcom/ds/nsHashtable.h;
/cvsroot/mozilla/xpcom/ds/nsHashtable.h,v  <--  nsHashtable.h
new revision: 1.48; previous revision: 1.47
Checking in netwerk/streamconv/src/nsStreamConverterService.cpp;
<--  nsS
new revision: 1.55; previous revision: 1.54
Checking in netwerk/streamconv/src/nsStreamConverterService.h;
<--  nsStr
new revision: 1.16; previous revision: 1.15
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
This is a "code change" bug, so if Tanu can provide a bonzai query that shows
the check, then this can be marked VERIFIED.
Keywords: verifyme
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.
VERIFIED: per owner.
You need to log in before you can comment on or make changes to this bug.