Closed
Bug 199425
Opened 21 years ago
Closed 21 years ago
FTP causing lots of leaks during bloat test
Categories
(Core Graveyard :: Networking: FTP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bryner, Assigned: darin.moz)
Details
Attachments
(3 files)
11.91 KB,
text/plain
|
Details | |
1.60 KB,
text/plain
|
Details | |
1000 bytes,
patch
|
bbaetz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
Reporter | ||
Comment 2•21 years ago
|
||
Comment 3•21 years ago
|
||
Didn't darin fix this recently? Or did that not get checked in yet?
Comment 4•21 years ago
|
||
bug 191835 but there seems to be conflicting reports that more leaking than that leak is happening.
Reporter | ||
Comment 5•21 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...
Assignee | ||
Comment 7•21 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.)
Reporter | ||
Comment 8•21 years ago
|
||
dbaron: how would that cause nsFTPState to call StopProcessing and release its channel reference?
Comment 9•21 years ago
|
||
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.
Reporter | ||
Comment 10•21 years ago
|
||
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.
Reporter | ||
Updated•21 years ago
|
Attachment #118830 -
Flags: superreview?(darin)
Attachment #118830 -
Flags: review?(dougt)
Assignee | ||
Comment 11•21 years ago
|
||
Comment on attachment 118830 [details] [diff] [review] patch sr=darin
Attachment #118830 -
Flags: superreview?(darin) → superreview+
Updated•21 years ago
|
Attachment #118830 -
Flags: review?(dougt) → review+
So what I was talking about in comment 6 was really about the leak in bug 200094, I guess.
Reporter | ||
Comment 13•21 years ago
|
||
checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•2 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•