77672 - nsLDAPConnection leaking threads

Status: VERIFIED FIXED

(Directory Graveyard :: LDAP XPCOM SDK, defect, P1)

Description

[Leif Hedstrom]

After some digging around with refcnt tools, and a lot of help from #mozilla
contributors :), I've finally figured out why we are leaking threads in
nsLDAPConnection. There are actually two issues:

1. We never know when the thread can exit, so it's going to linger around
forever. This is easy to fix by adding an internal state variable, say
mShutdown, which we can set in the destructor method, and then check for it in
the while(1) "poll" loop in the Run method.

2. However, even doing #1 is not enough. The problem is that nsLDAPConnection
implement nsILDAPConnection *and* nsIRunnable. The thread started will have a
reference to it's nsIRunnable implementation, which happens to be the same
object as the nsLDAPConnection implementation. So, even when the last reference
to an nsLDAPConnection is released, there's still one left in the Thread object.
Not exactly a circular reference, but somewhat similar.


Solving #2 could be done by separating the implementation of nsIRunnable into a
separate object. And the implementation of nsLDAPConnection would have a
reference to this object. When the last reference to an nsLDAPConnection goes
out of scope, the destructor will get called (since refcnt is 0), and we can
properly exit the thread as in #1 above.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Making this depend on the master autocompletion bug, since we'll need to fix
this before we ship.

Comment 2

[yulian chang]

reassign to Olga as QA contact

Comment 3

[Marek Z. Jeziorek]

Setting target milestone to 0.9.2 (check it in anytime, even before, when the
tree is open for). Per PDT triage.

Comment 4

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 5

[Leif Hedstrom]

I finally have a solution to this problem, but I doubt I'll be able to get
reviwers and super reviewers approval for check in today. I'm pretty confident I
can get it checked in no later than Wednesday though.

-- Leif

Comment 6

[Leif Hedstrom]

Attachment: First patch, finally...

Comment 7

[Leif Hedstrom]

Attached is a first patch, it still needs a little more testing, particularly on
Mac and Windows. But, dmose, if you have some spare time to look at it, and give
feedback, we have better chance of getting this checked in sooner than later. :)

Thanks!

-- Leif

Comment 8

[Leif Hedstrom]

Oh I forgot, the patch looks a little messy, cause I moved one function within
the file (into a new objectclass). "diff" doesn't handle this very gracefully,
so the patch looks bigger than it actually is. For instance, I don't touch the
method |nsLDAPConnection::InvokeMessageCallback()| at all.

-- Leif

Comment 9

[Leif Hedstrom]

Attachment: Second patch, removed one obsolete line.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

If you can add some comments about how and why you're using weak references to
nsLDAPConnection::Init, that'd be good.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

With those comments, as well as the renaming of mConnection to indicate it's
|weak|ness, r=dmose.

Comment 12

[Leif Hedstrom]

Attachment: 3rd patch, after Dmose/Darin reviews

Comment 13

[Leif Hedstrom]

This third patch needs serious reviews and testing, I'm on a little thin ice
here... Darin pointed out that my 2nd patch had a race condition, basically
|do_QueryReferent()| is not MT safe, and could possible get trashed if the
|nsLDAPConnection| object is GC "mid air".

I've implemented my own |Release()| in |nsLDAPConnection|, and use a lock before
calling |do_QueryReferent()|, which is also locked in the |Release|. That way,
the |nsLDAPConnection| object can't get deleted while |do_QueryReferent()| runs.

Eeeek.

-- Leif

Comment 14

[Leif Hedstrom]

Attachment: 4th patch, more comments from Darin

Comment 15

[Leif Hedstrom]

Attachment: 5th patch, comments etc.

Comment 16

[Darin Fisher]

this 5th patch looks good to me.

Comment 17

[Leif Hedstrom]

Darin, can you please do a final SR= for this patch? And, thanks a *ton* for all
the help helping me understand how to do all this stuff. :)

Thanks,

-- leif

Comment 18

[Leif Hedstrom]

Attachment: One more patch (5.1), added a couple of more comments

Comment 19

[Darin Fisher]

sr=darin

Comment 20

[Leif Hedstrom]

Attachment: Last patch, changed a comment, and tweaked two timeout values

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Thanks for helping us sort this out, darin!

r=dmose

Comment 22

[chris hofmann]

a=chofmann

Comment 23

[Leif Hedstrom]

Attachment: Ack, this is last patch, left a bogus (unused) line by mistake

Comment 24

[Leif Hedstrom]

Checked in.

Comment 25

[olga charapaev]

verified on WinNT and Linux with latest builds - no leaking threads
(wasn't able to find a way to count threads on Mac)