Closed
Bug 77672
Opened 23 years ago
Closed 23 years ago
nsLDAPConnection leaking threads
Categories
(Directory :: LDAP XPCOM SDK, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.2
People
(Reporter: leif, Assigned: leif)
References
Details
(Whiteboard: [PDT+] ready for checkin, critical for 0.9.2)
Attachments
(8 files)
14.63 KB,
patch
|
Details | Diff | Splinter Review | |
14.59 KB,
patch
|
Details | Diff | Splinter Review | |
17.29 KB,
patch
|
Details | Diff | Splinter Review | |
17.63 KB,
patch
|
Details | Diff | Splinter Review | |
18.27 KB,
patch
|
Details | Diff | Splinter Review | |
18.86 KB,
patch
|
Details | Diff | Splinter Review | |
19.15 KB,
patch
|
Details | Diff | Splinter Review | |
19.14 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•23 years ago
|
Severity: normal → major
Priority: -- → P2
Comment 1•23 years ago
|
||
Making this depend on the master autocompletion bug, since we'll need to fix this before we ship.
Depends on: 17880
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9.1
Comment 3•23 years ago
|
||
Setting target milestone to 0.9.2 (check it in anytime, even before, when the tree is open for). Per PDT triage.
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Assignee | ||
Comment 5•23 years ago
|
||
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
Whiteboard: [PDT+] → [PDT+] Should be able to check in on Wednesday
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 years ago
|
||
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
Assignee | ||
Comment 8•23 years ago
|
||
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
Assignee | ||
Comment 9•23 years ago
|
||
Comment 10•23 years ago
|
||
If you can add some comments about how and why you're using weak references to nsLDAPConnection::Init, that'd be good.
Comment 11•23 years ago
|
||
With those comments, as well as the renaming of mConnection to indicate it's |weak|ness, r=dmose.
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Comment 13•23 years ago
|
||
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
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
Comment 16•23 years ago
|
||
this 5th patch looks good to me.
Assignee | ||
Comment 17•23 years ago
|
||
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
Whiteboard: [PDT+] Should be able to check in on Wednesday → [PDT+] SR=requested from Darin, 6/20
Assignee | ||
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
sr=darin
Assignee | ||
Comment 20•23 years ago
|
||
Comment 21•23 years ago
|
||
Thanks for helping us sort this out, darin! r=dmose
Comment 22•23 years ago
|
||
a=chofmann
Assignee | ||
Comment 23•23 years ago
|
||
Updated•23 years ago
|
Whiteboard: [PDT+] SR=requested from Darin, 6/20 → [PDT+] ready for checkin
Assignee | ||
Comment 24•23 years ago
|
||
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Updated•23 years ago
|
Whiteboard: [PDT+] ready for checkin → [PDT+] ready for checkin, critical for 0.9.2
Comment 25•23 years ago
|
||
verified on WinNT and Linux with latest builds - no leaking threads (wasn't able to find a way to count threads on Mac)
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•