75337 - leaking SCTableData from nsStreamConverterService::FindConverter

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

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

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...

Comment 1

[Judson Valeski]

ugh, early return implies broken logic in this code. how did you reproduce this 
leak? I purified all this code before checking it in.

Comment 2

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

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.

Comment 3

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

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?

Comment 4

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

Attachment: fix for leak

Comment 5

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

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
...

Comment 6

[Judson Valeski]

Attachment: patch to use nsObjectHashtable

Comment 7

[Judson Valeski]

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.

Comment 8

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

r=dbaron

Comment 9

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

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

Comment 10

[Judson Valeski]

moving to 0.9, all I need is an sr.

Comment 11

[vidur (gone)]

sr=vidur.

Comment 12

[Judson Valeski]

fix is in.

Comment 13

[Bradley Baetz (:bbaetz)]

According to my source tree, we're now using nsobjecthashtable. VERIFIED.