59817 - FTP leaks nsThread*

Status: VERIFIED FIXED

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

Description

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

DESCRIPTION:  Loading an FTP url (e.g., ftp://ftp.mozilla.org/ as on the bloat
tests) leaks an nsThread, nsThreadPool, nsThreadPoolRunnable, and 2
nsSupportsArray.  I've traced this problem to the nsThreadPool::Shutdown not
being called on the nsThreadPool that is the nsFtpProtocolHandler's mPool.

The following patch fixes the leak (I used it to test my theory).  I've yet to
try and figure out whether it is safe and whether it's the right thing to do.
Comments would be appreciated...

Index: nsFtpProtocolHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp,v
retrieving revision 1.52
diff -u -d -r1.52 nsFtpProtocolHandler.cpp
--- nsFtpProtocolHandler.cpp    2000/09/13 23:54:25     1.52
+++ nsFtpProtocolHandler.cpp    2000/11/11 04:19:10
@@ -74,6 +74,7 @@

 nsFtpProtocolHandler::~nsFtpProtocolHandler() {
     PR_LOG(gFTPLog, PR_LOG_ALWAYS, ("~nsFtpProtocolHandler() called"));
+    if (mPool) mPool->Shutdown();
     if (mLock) PR_DestroyLock(mLock);
 }

Comment 1

[Judson Valeski]

hmmm. nsIThreadPools are supposed to shutdown themselves when the last ref goes
away (http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsThread.cpp#364).
The protocol handler keeps a ref (mPool), as does each FTP channel (mPool
again). I suspect the channel is still outstanding when the handler tries to go
away, thus, the threadpool never shuts down. Are we seeing channels leaked
(before your diff)?

Comment 2

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

No.  However, nsThreadPool, nsThread, and nsThreadPoolRunnable maintain circular
references (I forget exactly how) that need to be broken by
nsThreadPool::Shutdown (unless you want to call that a bug in the thread code).
 I think other users call nsThreadPool::Shutdown:

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsFileTransportService.cpp#60
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsFileTransportService.cpp#152

I think this should be better documented...

Comment 3

[Judson Valeski]

grrr. we broke semantics, *again.* It's a toss up. if all outstanding refs to
that pool go away, it will shut itself down. If we don't want to track down the
ellusive refs, then yea, this patch will force the cleanup.

Comment 4

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

The leaks are as follows:

3 references to nsThread leaked:
 * 1 from |nsCOMPtr<nsIThread> currentThread| in nsThreadPoolRunnable::Run() (so
I guess the thread is still running)
 * 1 from nsThreadPool::mThreads
 * 1 from NS_ADDREF_THIS() in nsThread::Init (to be released in nsThread::Exit)

1 reference to nsThreadPool leaked:
 * from nsThreadPoolRunnable::mPool

1 reference to nsThreadPoolRunnable leaked:
 * from nsThread::mRunnable

Comment 5

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

If we can get nsThreadPoolRunnable::Run to exit, everything will work fine...

Comment 6

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

But it seems the (or one) way to do that is to call nsThreadPool::Shutdown. 
That's the way the only other user of nsIThreadPool (the file transport service)
does it.

Comment 7

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

Attachment: alternative fix: make nsThreadPoolRunnable::mPool a raw pointer

Comment 8

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

The above patch seems to work, but I have no idea if it's safe or not...

Comment 9

[Judson Valeski]

Don't make mPool a raw pointer, that's changing the underlying ownership model.

Comment 10

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

Do you have any suggestions other than making the underlying ownership model
non-circular or making an explicit call to break the circular ownership?  (I'm
really not a big fan of custom release methods that delete when they hit some
nonzero refcount -- they're hard to get right.)

Comment 11

[Judson Valeski]

I believe the root of this problem is that we're (last I checked) doing double
FTP requests. To see it, turn on FTP logging (export
NSPR_LOG_MODULES=nsFTPProtocol:5) and notice the same request generated twice.
The double request, I believe, is not intended and is causing refs to get
confused on the consumer side. Once that is cleaned up, I bet these leaks go away.

Comment 12

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

Why?  It seems the way thread pools work, they will leak unless they are shut
down or unless we change the ownership model of thread pools.

But anyway, since you claim the fix is more complicated, assigning to default
FTP owner.

Comment 13

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

This was fixed by dougt's ftp changes, since FTP no longer uses threadpools.

Comment 14

[Doug Turner (:dougt)]

that is correct.  Ftp still leaks, but does not leak nsThreads/.

Comment 15

[Tom Everingham]

verified