51937 - FTP is leaking

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P3)

Description

[Judson Valeski]

1. start up the browser w/ about:blank
2. goto ftp.mozilla.org
3. exit

results: two ftp channels, two ftp connection threads, two FTP dir listing
converters, and the ftp protocol handler are leaked.

I tested this on linux using XPCOM_MEM_LEAK_LOG (results attatched).

Comment 1

[Judson Valeski]

Attachment: leak log

Comment 2

[Robert John Churchill]

Jud, what's the root of the leak?  I'd guess that documents are leaking.

Comment 3

[Judson Valeski]

good question and good guess. I'm not sure the relationship between documents
and FTP/file (directory viewer consumers) was ever really thought out.

Comment 4

[Robert John Churchill]

I'm sure that's true.  <grin>  Anyway, I've seen some other email/bugs lately
(cc'ing dougt) that bookmarks are leaking as well... all this stuff really
smells like documents are leaking.

Who wants to try and find the root leaks?  I nominate dougt as he's already been
looking at leaks for a bit.

Comment 5

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

Also see bug 43567.

This leak is now 2 nsPipe, and 1 nsSocketTransport per site (plus the
nsSocketTransportService).  (More leaks if an FTP directory load is cancelled
partway through, but that's another issue.  And there's also occasionally an
nsConnectionCacheObj leaked on tinderbox.)

Comment 6

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

It seems to me that the root of the leak is a cycle.
nsSocketTransport::Open(Input|Output)Stream or nsSocketTransport::AsyncRead
creates nsPipe objects that it owns, m(Read|Write)Pipe(In|Out).  It then call
pipe->SetObserver(this), which makes the pipe's mObserver be an owning reference
to the nsSocketTransport.

This cycle is broken in nsSocketTransport::Process, where the nsSocketTransport
releases the pipes in certain cases, or in certain places in the nsPipe
implementation.  But neither is happening here.

There's also seems to be one leaked reference from the nsSocketTransportService
to the nsSocketTransport, in mActiveTransportList, but I'm not sure about this...

Comment 7

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

Another interesting point:  ftp://ftp.mozilla.ogr/ (nonexistent) leaks an
nsConnectionCacheObj and an additional 2 nsStr that are not leaked for
ftp://ftp.mozilla.org/

Comment 8

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

What I described in my previous comment was a separate bug, now described by bug
58948.

Comment 9

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

The patch below fixes the leak described here using an approach similar to the
one waterson used in bug 43567:

Index: nsFtpProtocolHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp,v
retrieving revision 1.52
diff -u -d -r1.52 nsFtpProtocolHandler.cpp
--- nsFtpProtocolHandler.cpp	2000/09/13 23:54:25	1.52
+++ nsFtpProtocolHandler.cpp	2000/11/11 05:03:20
@@ -262,7 +263,11 @@
 
 // cleans up a connection list entry
 static PRBool PR_CALLBACK CleanupConnEntry(nsHashKey *aKey, void *aData, void
*closure) {
-    delete (nsConnectionCacheObj*)aData;
+    nsConnectionCacheObj *entry = NS_STATIC_CAST(nsConnectionCacheObj*,
aData);+    if (entry && entry->mSocketTransport) {
+      entry->mSocketTransport->Cancel(NS_BINDING_ABORTED);
+    }
+    delete entry;
     return PR_TRUE;
 }

Comment 10

[Judson Valeski]

I wonder if including these cancels solves this problem too???

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1856

Comment 11

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

Hmmm.  I don't think there should be an error state here, but I could check...

Comment 12

[Doug Turner (:dougt)]

ftp bug.  no time to play.  assigning to gagan.

Comment 13

[Gagan]

ftp bugs to component:ftp

Comment 14

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

valeski:  Including those cancels (in nsFTPConnectionThread.cpp) does not fix
the leak.

Comment 15

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

Attachment: proposed patch

Comment 16

[Judson Valeski]

hmm, the only difference between calling cancel and not calling cancel (ie.
letting the socket transport destructor "clean up" the transport) is that the
destructor basically just calls CloseConnection(), while Cancel() will cause
listener OnStart|Stop sequences to be fired if they haven't already. So by
forcing the Cancel() your telling listenrs to release their ref to the socket
transport. I don't like that we're Cancel()'ing the transport under normal
operation conditions. I believe one of the listener's/consumers is causing this
cycle and that this patch is masking the underlying problem. Proof of this is in
testing FTP through TestProtocols (it doesn't leak there).

Comment 17

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

Attachment: leaks loading ftp://ftp.mozilla.org/ in TestProtocols

Comment 18

[Judson Valeski]

grr. I didn't used to leak there under testprotocols. sounds like the thread
pool impl might be hosed.

Comment 19

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

These leaks seem to have been fixed by dougt's changes for bug 63565.  Or, at
the least, it's all being freed on shutdown.

Comment 20

[Doug Turner (:dougt)]

it should also fix leaks from IMAP caused by a similar condition.  Marking as 
fixed.

Comment 21

[Doug Turner (:dougt)]

we are leaking itermittenly still after my ftp landing. damn it!  


--NEW-LEAKS-----------------------------------leaks------leaks%-----------------
------
nsHashKey                                         4          -
nsEventQueueImpl                                 32          -
nsAsyncStreamListener                            20          -
nsFtpControlConnection                          276          -
nsEventQueueServiceImpl                          56          -
nsAsyncStreamObserver                            32          -
nsMemoryImpl                                     64          -
nsPipe                                          184          -
nsSocketTransport                               360          -
nsSocketTransportService                         64          -
nsHashtable                                      44          -
nsStr                                           480       9.09%
TOTAL                                          1616


I bet the nsAsyncStreamListener is holding on to one of the transports and that 
transport is holding on to most everything else. 

Comment 22

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

The leaked reference to the nsAsyncStreamObserver (that wasn't also an
nsAsyncStreamListener) was created here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsSocketTransport.cpp&rev=1.174&mark=2003#1993
and from a leaked nsSocketTransport.

The leaked reference to the nsAsyncStreamListener was created here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsSocketTransport.cpp&rev=1.174&mark=1927#1917
and from a leaked nsSocketTransport.

The 4 leaked references to the 1 leaked nsSocketTransport were created at the
following locations:

1)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/io/nsPipe2.cpp&rev=1.31&mark=82#72
called from
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsSocketTransport.cpp&rev=1.174&mark=1917#1907
i.e., from the socket transport's own mReadPipeIn.mObserver

2) ditto, except mReadPipeOut

3) from
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.cpp&rev=1.2&mark=66#56
i.e., the mCPipe of a leaked nsFtpControlConnection

4) One more addref in nsSocketTransportService::AddToSelectList than
release in nsSocketTransportService::RemoveFromSelectList

The leaked reference to the nsFtpControlConnection was created at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsAsyncStreamListener.cpp&rev=1.36&mark=159#149
and was thus held by the mReceiver of the leaked nsAsyncStreamListener

Comment 23

[Doug Turner (:dougt)]

these leaks should be different now because of my necko landing.

Comment 24

[Doug Turner (:dougt)]

marking fixed.  if there are new leak, please file new bugs.

Comment 25

[John Unruh]

After a year and a half, verified per dougt's comment. If there are new leaks,
please file new bugs.