200094 - FTP control connection-related leaks

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Brian Ryner (not reading)]

We're no longer leaking nsFtpControlConnections, but its socket transport and
some related objects are leaking.  The reason is that there's a circular
reference.  I'll try my best to illustrate:

    nsFtpControlConnection
             |
             | (mOutStream)
             V
      nsPipeOutputStream
             ^
             | (mSource)
             |
       nsStreamCopierIB
          ^          |
(mNotify) |          | (mSink)
          |          V
       nsSocketOutputStream

So, without any explicit cleanup, the SocketOutputStream (really
nsSocketTransport) and the StreamCopierIB leak each other, causing the
nsPipeOutputStream (really nsPipe) and the SocketTransportService to leak.

This might be a symptom of a more generic problem (shouldn't these objects be
able to clean up after themselves when you release all references to them?) 
I'll leave that question for Darin and Doug.  We can easily break the cycle in
this case though, by calling Close on mOutStream in
nsFtpControlConnection::Disconnect.

Comment 1

[Brian Ryner (not reading)]

Attachment: explicitly break the cycle

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 119053 [details] [diff] [review]
explicitly break the cycle

Feel free to smack this down if there's a way to fix this generically.

Comment 3

[Brian Ryner (not reading)]

The above diagram is somewhat ambiguous... it's the nsPipeInputStream, not the
nsPipeOutputStream, that's the nsStreamCopierIB's |mSource|.  But they're both
really just the pipe object.

Comment 4

[Darin Fisher]

bryner:

zero'ing out mOutStream should be equivalent to calling Close on the output
stream.  notice that the pipe's output and input streams each have separate
reference counts.  though there is an overall reference count for the entire
pipe object, there are separate read and write ref counts.  when those
individual ref counts go to zero, Close is called on the respective input or
output stream.  so, it sounds like there is an extra reference to the pipe's
output stream somewhere.

Comment 5

[Darin Fisher]

Attachment: alternate patch

this patch also breaks the cycle.  i'm still not entirely sure why the original
code doesn't work.  but, something about closing mOutStream first is critical.

Comment 6

[Brian Ryner (not reading)]

At Darin's request I opened bug 200219 about the more general problem.  I went
ahead and checked in this patch.