199425 - FTP causing lots of leaks during bloat test

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Brian Ryner (not reading)]

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

[Brian Ryner (not reading)]

Attachment: diff of leak logs

Comment 2

[Brian Ryner (not reading)]

Attachment: nspr log of nsFTPProtocol and nsSocketTransport

Comment 3

[Bradley Baetz (:bbaetz)]

Didn't darin fix this recently? Or did that not get checked in yet?

Comment 4

[Nicholas Allen]

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

Comment 5

[Brian Ryner (not reading)]

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?

Comment 6

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

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

[Darin Fisher]

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

[Brian Ryner (not reading)]

dbaron: how would that cause nsFTPState to call StopProcessing and release its
channel reference?

Comment 9

[Bradley Baetz (:bbaetz)]

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

[Brian Ryner (not reading)]

Attachment: patch

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.

Comment 11

[Darin Fisher]

Comment on attachment 118830 [details] [diff] [review]
patch

sr=darin

Comment 12

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

So what I was talking about in comment 6 was really about the leak in bug
200094, I guess.

Comment 13

[Brian Ryner (not reading)]

checked in.