leaking SCTableData from nsStreamConverterService::FindConverter

VERIFIED FIXED in mozilla0.9

Status

()

Core
Networking
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: dbaron, Assigned: Judson Valeski)

Tracking

({memory-leak})

Trunk
mozilla0.9
x86
Linux
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: waiting for sr)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
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...
(Reporter)

Updated

17 years ago
Keywords: mlk
(Assignee)

Comment 1

17 years ago
ugh, early return implies broken logic in this code. how did you reproduce this 
leak? I purified all this code before checking it in.
(Reporter)

Comment 2

17 years ago
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.
(Assignee)

Updated

17 years ago
Target Milestone: --- → Future
(Reporter)

Comment 3

17 years ago
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?
(Reporter)

Comment 4

17 years ago
Created attachment 30889 [details] [diff] [review]
fix for leak
(Reporter)

Comment 5

17 years ago
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
...
(Assignee)

Comment 6

17 years ago
Created attachment 30902 [details] [diff] [review]
patch to use nsObjectHashtable
(Assignee)

Comment 7

17 years ago
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.
(Reporter)

Comment 8

17 years ago
r=dbaron
(Reporter)

Comment 9

17 years ago
BTW, should this be moved forward from Future (so it gets checked in)?
(Assignee)

Comment 10

17 years ago
moving to 0.9, all I need is an sr.
Keywords: patch
Whiteboard: waiting for sr
Target Milestone: Future → mozilla0.9

Comment 11

17 years ago
sr=vidur.
(Assignee)

Comment 12

17 years ago
fix is in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

17 years ago
QA Contact: tever → bbaetz
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.