Under the boehm GC I just saw a leak of SCTableData allocated on line 255 of nsStreamConverterService.cpp, in InitBFSTable, which is called from nsStreamConverterService::FindConverter. I suspect the problem is an early return (like in bug 73308, which describes a different leak in the same function). An easy solution would be to convert lBFSTable to an nsObjectHashtable. I think all you would need to do is pass (nsnull, nsnull, DeleteBFSEntry, nsnull) to the constructor and then change the |lBFSTable.Reset(DeleteBFSEntry, nsnull)| calls to |lBFSTable.Reset()|. Or something like that...
ugh, early return implies broken logic in this code. how did you reproduce this leak? I purified all this code before checking it in.
I think I saw it running precheckin tests, although I may have done some other stuff in that browser session too -- I'm not sure.
Steps to reproduce: * start mozilla * go to Edit | Preferences * open the Navigator twisty * click on the History subitem * click on the Navigator item again (it was where you started) * cancel out of prefs * quit mozilla I have a fix for this leak that I will attach. Should there be a separate bug for the fact that there's an unexpected early return from this function, or should either this bug or bug 73308 serve that purpose?
Debugging a little bit more, I see that the early return is happening every time I load a given pref panel for the second time, and it's happening at line 305, here: nsCStringKey *source = new nsCStringKey(fromC.get()); SCTableData *data = (SCTableData*)lBFSTable.Get(source); if (!data) return NS_ERROR_FAILURE; /* <=== we return here */ The contractID given to the FindConverter is @mozilla.org/streamconv;1?from=mozilla.application/cached-xul&to=*/* and FindConverter only seems to be called the second or later times that the given panel is loaded. The stack is: nsStreamConverterService::FindConverter nsStreamConverterService::AsyncConvertData nsDocumentOpenInfo::RetargetOutput nsDocumentOpenInfo::DispatchContent nsDocumentOpenInfo::OnStartRequest nsCachedChromeChannel::HandleStartLoadEvent PL_HandleEvent ...
Thanks for digging David. nsObjectHashtable didn't exist when this code was written, and doing the Reset() in the destructor (as nsObjectHashtable does) will indeed solve this. I've added another patch to take it one step further and use the object hash table in mAdjacencyList too. Can you r= this? No need for another bug, we're supposed to gracefully (not leak) fail in this case. The reason purify wasn't picking this up was because I wasn't testing w/ NS_XPCOMSHUTDOWN() being called which was causing the app to terminate in a funky way (hiding these leaks). I'm a bit suprised we call this code for prefs pane manipulation, but it's conceivable.
BTW, should this be moved forward from Future (so it gets checked in)?
moving to 0.9, all I need is an sr.
Whiteboard: waiting for sr
Target Milestone: Future → mozilla0.9
fix is in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
According to my source tree, we're now using nsobjecthashtable. VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.