Memory leak of thread private data in classic NSPR

VERIFIED FIXED

Status

NSPR
NSPR
P3
normal
VERIFIED FIXED
19 years ago
19 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

19 years ago
This bug is reported by Chris Houck.

In pruthr.c, _PR_NativeDestroyThread(),
the thread private data is not disposed
of and hence leaked.  This memory leak
is caught by Purify.

Going through the file, I spotted other
places where thread private data is not
disposed of.  We need to do a careful
code review.

The pthreads version does not seem to have
this problem.
(Assignee)

Comment 1

19 years ago
Created attachment 2497 [details] [diff] [review]
The patch that Chris Houck submitted.  Note that this is not the complete fix.
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

19 years ago
I thoroughly reviewed our TPD code and fixed
other problems that I noticed.  I'm going to
post a patch with comments.
(Assignee)

Comment 3

19 years ago
Created attachment 2899 [details] [diff] [review]
A patch for the memory leak and other problems in our TPD code.
(Assignee)

Comment 4

19 years ago
Comments on the patch (attachment id 2899):
1. primpl.h: the global variable _pr_tpd_destructors does
   not need to be exported from prtpd.c.
2. ptthread.c: removed unused field 'highwater' from pt_book.
   Removed commented out code.  _pt_thread_death now needs to
   free thred->privateData because _PR_DestroyThreadPrivate no
   longer does that.
3. prcthr.c: _PR_CleanupThread should call _PR_DestroyThreadPrivate
   to free the thread-private data because that function takes care
   of all the necessary iterations (4) of invoking the destructors.
4. prtpd.c: Removed unused macro _PR_TPD_MODULO and made _pr_tpd_destructors
   static.  PR_NewThreadPrivateIndex should return index starting from
   0 as opposed to 1.  Fixed several problems in PR_SetThreadPrivate and
   _PR_DestroyThreadPrivate.  Also, _PR_DestroyThreadPrivate now doesn't
   free self->privateData, so that it can be called by _PR_CleanupThread
   to prepare a dead thread for recycling.
5. pruthr.c: _PR_NativeDestroyThread should free thread->privateData
   to plug the memory leak that chouck reported.
(Assignee)

Updated

19 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 19 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

19 years ago
I checked in the fix.
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h, revision 3.27
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c, revision 3.28
/cvsroot/mozilla/nsprpub/pr/src/threads/prcthr.c, revision 3.10
/cvsroot/mozilla/nsprpub/pr/src/threads/prtpd.c, revision 3.7
/cvsroot/mozilla/nsprpub/pr/src/threads/combined/pruthr.c, revision 3.16

The fix is also checked into the internal repository.
/m/src/ns/nspr20/pr/include/private/primpl.h, revision 2.68
/m/src/ns/nspr20/pr/src/pthreads/ptthread.c, revision 2.56
/m/src/ns/nspr20/pr/src/threads/prcthr.c, revision 2.15
/m/src/ns/nspr20/pr/src/threads/prtpd.c, revision 2.10
/m/src/ns/nspr20/pr/src/threads/combined/pruthr.c, revision 2.33

Chouck, can you verify that the memory leak is gone?  Thanks.

Comment 6

19 years ago
After applying your patch Purify is no longer reporting leaked thread
private data.

Thanks,
-Chris
(Assignee)

Updated

19 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 7

19 years ago
Thanks, Chris.  Marked the bug verified.

Comment 8

19 years ago
The patch looks good.

In _pt_thread_death, shouldn't thred->errorString, thred->io_cv and
thred->privateData be set to NULL?
(Assignee)

Comment 9

19 years ago
Setting those fields of 'thred' to NULL is not
necessary because 'thred' will be freed.

Comment 10

19 years ago
This isn't a bug, I was suggesting it more as a safety feature, if the threads
are recycled in the feature. When resources are freed conditionally, it is
better to reset the condition after freeing the resources.
You need to log in before you can comment on or make changes to this bug.