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.
Didn't darin fix this recently? Or did that not get checked in yet?
bug 191835 but there seems to be conflicting reports that more leaking than that leak is happening.
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...
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.)
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.
Created attachment 118830 [details] [diff] [review] 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 on attachment 118830 [details] [diff] [review] patch sr=darin
Attachment #118830 - Flags: superreview?(darin) → superreview+
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.