FTP causing lots of leaks during bloat test




16 years ago
16 years ago


(Reporter: bryner, Assigned: darin.moz)



Firefox Tracking Flags

(Not tracked)



(3 attachments)



16 years ago
So, we're leaking a whole bunch of objects during the bloat test (mozilla -f
bloaturls.txt) that we don't leak if you remove the 1 FTP site from the list.  I
think something in the ftp code is leaking and bringing a lot of other objects
along with it.  I'll attach the parts of the refcount leak log that can be
attributed to having the FTP URL in the test.  I'm not sure what the root of the
leaks is yet, but I'm guessing it's one of the necko objects.

Comment 1

16 years ago
Created attachment 118628 [details]
diff of leak logs

Comment 2

16 years ago
Created attachment 118629 [details]
nspr log of nsFTPProtocol and nsSocketTransport
Didn't darin fix this recently? Or did that not get checked in yet?

Comment 4

16 years ago
bug 191835 but there seems to be conflicting reports that more leaking than that
leak is happening.

Comment 5

16 years ago
So, I think the problem here is that we're never entering the FTP_COMPLETE state
in the nsFtpConnectionThread, so StopProcessing() is never called and the
ownership cycle between the nsFtpChannel and nsFtpState is never broken.  I
guess this is because the directory listing converter handles all of the
OnDataAvailable notifications.  What's an appropriate way to get StopProcessing
to be called when the listing is complete?
Hmm, my latest theory was that nsSocketTransport::OnMsgInputClosed and
nsSocketTransport::OnMsgOutputClosed can't both call
mOutput.OnSocketReady(reason) and mInput.OnSocketReady(reason) (due to
mInputClosed / mOutputClosed), which means that one of them won't release their
|mNotify|.  (I also don't see how the delaying of setting mCondition =
NS_BASE_STREAM_CLOSED until both are closed can work, since
nsSocket[Input|Output]Stream::OnSocketReady sets mCondition.)

But I'm not seeing the leak right now, so I can't test this theory.  It also
doesn't explain why it wouldn't show up all the time...

Comment 7

16 years ago
dbaron: see nsSocketTransport::OnSocketDetached.

the undocumented :-( design here is that the socket transport should not be
shutdown until both the input and output streams have been closed.  the
OnSocketReady notification is just a means of triggering the input or output
stream to do work (given that some event has occured... poll returning, socket
error, etc.)

Comment 8

16 years ago
dbaron: how would that cause nsFTPState to call StopProcessing and release its
channel reference?
Yeah, this is all a pain because of the requirements for sometimes keeping PASV
connections open past the command. Note that despite the filename,
ftpconnectionthread.cpp does not run on a separate thread.

Comment 10

16 years ago
Created attachment 118830 [details] [diff] [review]

The ftp channel was causing leaks in the case where the directory listing comes
from the cache.  This patch makes sure that we don't keep a reference to the
FTPState after OnStopRequest is called.


16 years ago
Attachment #118830 - Flags: superreview?(darin)
Attachment #118830 - Flags: review?(dougt)

Comment 11

16 years ago
Comment on attachment 118830 [details] [diff] [review]

Attachment #118830 - Flags: superreview?(darin) → superreview+
Attachment #118830 - Flags: review?(dougt) → review+

Comment 13

16 years ago
checked in.
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.