58948 - leak nsConnectionCacheObj when loading nonexistent FTP site

Status: VERIFIED INVALID

(Core Graveyard :: Networking: FTP, defect, P3)

Description

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

If you start "./mozilla ftp://ftp.mozilla.ogr/" (note misspelling), it leaks an
nsConnectionCacheObj and the things it owns.  This is a different leak from the
one described in bug 51937, where the things that it would own are leaked for
other reasons.

The following patch fixes the leak:

Index: nsFtpConnectionThread.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v
retrieving revision 1.138
diff -u -d -r1.138 nsFtpConnectionThread.cpp
--- nsFtpConnectionThread.cpp   2000/10/04 22:58:54     1.138
+++ nsFtpConnectionThread.cpp   2000/11/03 00:02:49
@@ -315,6 +315,9 @@
                     mConn->mCwd = mCwd;
                     rv = mConnCache->InsertConn(mCacheKey.GetBuffer(), mConn);
                     if (NS_FAILED(rv)) return rv;
+                } else {
+                    delete mConn;
+                    mConn = 0;
                 }

                 rv = StopProcessing();

Does this seem like the right thing to do?  If it is, please assign the bug to
me and let me know what needs to be tested.

Comment 1

[Gagan]

seems ok to me, but valeski/rjc should review this. 

Comment 2

[Judson Valeski]

this is caused by http://bugzilla.mozilla.org/show_bug.cgi?id=24435 . Because
FTP doesn't know when it has a bad host, we wind up failing the first time we
try to read from the stream we setup based on the host. That failure winds up
sending us to the FTP_COMPLETE: state when we should have gone to the FTP_ERROR
state (which cleans up the connection just fine).

The proposed fix will indeed fix the leak in this case. However it disrupts the
assumptions the state machine makes. You *should* (w/ the fix to the above but,
I'd argue you can't ever get into the this leak state) never hit this leak (so
says the state machine logic). Rather than taking this fix and doing the
extransous (albeit cheap) if clause, I'm inclined to wait for 24435 to get
fixed. Maybe I'm being pedantic here. If you want to run w/ this go ahead,
r=valeski... I've said my piece :-)

Comment 3

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

Since bug 24435 doesn't look like it's going to be fixed anytime soon, and
fixing leaks makes it a lot easier to fix other leaks, so I'm inclined to go
ahead with this patch.  Isn't the presence of |if
(NS_SUCCEEDED(mInternalError))| contrary to the state machine logic?
Considering that it's there, why not fix the leak with an |else| clause too?  Or
is there something else that's bad?

Comment 4

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

In particular, one reason I want to check this in is to see it will fix the
intermittent nsConnectionCacheObj leaks on tinderbox, or if they're caused by
something else.  (Occasionally the numbers on harpoon jump up 300 bytes.)

Comment 5

[Judson Valeski]

roger that, run w/ the checkin.

the ftp state logic breakdown I was talking about occurs in the first rev of the
running loop. internal error and mState are independent.

Comment 6

[Brendan Eich [:brendan]]

http://lxr.mozilla.org/mozilla/source/netwerk/protocol/ftp/src/nsFtpConnectionTh
read.cpp#620

at least sets mNextState to FTP_COMPLETE after possibly setting mInternalError. 
Jud, why is mInternalError independent from the FTP_ERROR state? I'm curious and 
need educating.

dbaron: sr=[e-mail] on the patch.

/be

Comment 7

[Judson Valeski]

mInternalError captures internal error state which goes beyond what the states
capture. There's a list of ftp errors in ftpCore.h which provide more info (say
for status observers).

Comment 8

[Chris Waterson]

Arriving late to the party. My only comment is that maybe you should add an
assertion before deleting the object to enforce jud's point that "we shouldn't
be getting here."

Comment 9

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

Fix for leak checked in 2000-11-07 19:08 PDT.

Regarding waterson's comment -- we shouldn't be getting there in theory, but we
do every time there's an incorrect FTP url entered, so we probably don't want an
assertion there now.

Comment 10

[Judson Valeski]

if the assertion were hit enough, it would garner support for fixing the actual
bug (24435).

Comment 11

[Peter "jag" Annema]

So add an assertion with a reference to the bug :-)

Comment 12

[Gagan]

ftp bugs to component:ftp

Comment 13

[Doug Turner (:dougt)]

nsConnectionCacheObj objects are history.  THis bug is now invalid.

Comment 14

[benc]

VERIFIED: 
A quick look in LXR could no find this.
(someone tell me if this is the wrong way of verifying...)