nsLDAPConnection leaking threads

VERIFIED FIXED in mozilla0.9.2

Status

P1
major
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: leif, Assigned: leif)

Tracking

(Blocks: 1 bug)

other
mozilla0.9.2
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+] ready for checkin, critical for 0.9.2)

Attachments

(8 attachments)

(Assignee)

Description

18 years ago
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

18 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

18 years ago
Severity: normal → major
Priority: -- → P2
Making this depend on the master autocompletion bug, since we'll need to fix
this before we ship.
Depends on: 17880
No longer depends on: 17880
(Assignee)

Updated

18 years ago
Keywords: nsbeta1
Priority: P2 → P1

Updated

18 years ago
Target Milestone: --- → mozilla0.9.1

Comment 2

18 years ago
reassign to Olga as QA contact
QA Contact: yulian → olgac

Comment 3

18 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

Comment 4

18 years ago
PDT+ per 6/12 mtg.
Whiteboard: [PDT+]
(Assignee)

Comment 5

18 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

18 years ago
Created attachment 39252 [details] [diff] [review]
First patch, finally...
(Assignee)

Comment 7

18 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

18 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

18 years ago
Created attachment 39270 [details] [diff] [review]
Second patch, removed one obsolete line.
If you can add some comments about how and why you're using weak references to
nsLDAPConnection::Init, that'd be good.
With those comments, as well as the renaming of mConnection to indicate it's
|weak|ness, r=dmose.
(Assignee)

Comment 12

18 years ago
Created attachment 39364 [details] [diff] [review]
3rd patch, after Dmose/Darin reviews
(Assignee)

Comment 13

18 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

18 years ago
Created attachment 39411 [details] [diff] [review]
4th patch, more comments from Darin
(Assignee)

Comment 15

18 years ago
Created attachment 39421 [details] [diff] [review]
5th patch, comments etc.

Comment 16

18 years ago
this 5th patch looks good to me.
(Assignee)

Comment 17

18 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

18 years ago
Created attachment 39454 [details] [diff] [review]
One more patch (5.1), added a couple of more comments

Comment 19

18 years ago
sr=darin
(Assignee)

Comment 20

18 years ago
Created attachment 39552 [details] [diff] [review]
Last patch, changed a comment, and tweaked two timeout values
Thanks for helping us sort this out, darin!

r=dmose

Comment 22

18 years ago
a=chofmann
(Assignee)

Comment 23

18 years ago
Created attachment 39556 [details] [diff] [review]
Ack, this is last patch, left a bogus (unused) line by mistake

Updated

18 years ago
Whiteboard: [PDT+] SR=requested from Darin, 6/20 → [PDT+] ready for checkin
(Assignee)

Comment 24

18 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+] ready for checkin → [PDT+] ready for checkin, critical for 0.9.2

Comment 25

18 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.