Closed
Bug 54264
Opened 24 years ago
Closed 24 years ago
XPConnect need to clean up it's TLS before it is unloaded from memory
Categories
(Core :: XPConnect, defect, P3)
Core
XPConnect
Tracking
()
VERIFIED
FIXED
People
(Reporter: jonsmirl, Assigned: jband_mozilla)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
XPConnect need to clean up it's TLS before it is unloaded from memory. This
only applies to primordial thread. A simple fix is attached.
In the current code if DLL unloading is enabled XPconnect can be unloaded while
NSPR still has a destructor callback. Clearing TLS will prevent the destructor
callback from being called.
Assignee | ||
Comment 2•24 years ago
|
||
I'm not byuibg this. Your patch has problems.
- If called from CleanupAllThreads it will repeatedly set the TLS for the
current thread.
- It has no check for gTLSIndex != BAD_TLS_INDEX. Luckily nspr will catch that.
- mMost importantly, it will cause a double delete on the object in question
when Cleanup is called from ~xpcPerThreadData. See:
http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/threads/prtpd.c#150
xpc_ThreadDataDtorCB is necessary for cases where lots of threads are created
and destroyed. Otherwise we leak this per-thread data.
I'm amazed that you are trying to support dll unloading. We have no code that is
prepared for that. xpconnect was never intended to support being unloaded.
If this were to be supported why would it only apply to the primordial thread?
Because that is the only place you see it crash in your application? This
problem could just as easily happen if anpother thread were to finish after
unloading xpconnect. No?
How can you possibly safely unload xpconnect? Are you also killing off all of
xpcom and everything associated with it? But leaving nspr running in the
process?
At the point where I call XPCOM shutdown my app has ensured all of the other
thread have exited. When the other threads exit NSPR automatically catches
their exit and calls their TLS destructors. Only the main thread has a
shutdown problem. Isn't it an error to be calling XPCOMShutdown if other
threads are active?
I'm only enabling DLL unloading as part of the shutdown process. A bunch of
leaks are associated with the DLL wrappers.
When I exit my app in a simple test case I have over 500 leaks from XPCOM. It
is very time consuming teying to sort out which ones I'm causing vs ones built
into XPCOM. The simplest solution is just to fix them all.
Check out bug 53933 for the changes to XPCOM shutdown
The patch adds PR_SetThreadPrivate(gTLSIndex, nsnull) to the end of
xpcPerThreadData::CleanupAllThreads() not Cleanup(). CleanupAllThreads is only
called from ~nsXPConnect which is only destroyed during XPCOM shutdown on the
primary thread. This patch has no effect on anything but the primary thread.
Without this patch the xpcPerThreadData for the primary thread is one of the
leaks I'm trying to clean up. xpc_ThreadDataDtorCB is not called for the
primary thread (it is called for all other threads) because XPCOM shutdown
didn't call PR_Cleanup.
When I added PR_Cleanup to XPCOM shutdown and enabled DLL unloading, I got a
GPF on xpc_ThreadDataDtorCB. The easiest solution to this was to clear the TLS
from CleanupAllThreads thus preventing xpc_ThreadDataDtorCB from being called
form PR_Cleanup.
Without this patch PR_Cleanup needs to be broken into a two phase process see
bug 54189 I didn't think it was likely that NSPR would get changed so I fixed
XPConnect instead.
Assignee | ||
Comment 5•24 years ago
|
||
Duh, Sorry I misread the patch and jumped to conclusions. I agree with your
reasoning.
I'm glad to see you did this outside the locked block.
One little change... I'll do the gTLSIndex != BAD_TLS_INDEX check first to
maintian local consistency.
Thanks for setting me straight. I'll attach my slightly different patch and get
this checked into the trunk soonish.
Assignee | ||
Comment 6•24 years ago
|
||
Assignee | ||
Comment 8•24 years ago
|
||
Fix checked in to trunk. Thanks!
Status: UNCONFIRMED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Attachment #15566 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #15580 -
Attachment description: slightly different fix → slightly different fix
[Checkin: Comment 8]
You need to log in
before you can comment on or make changes to this bug.
Description
•