Bugzilla bug #17601: fixed memory leak and some other problems in our
authorwtc%netscape.com
Tue, 16 Nov 1999 23:44:41 +0000
changeset 938 783bc0f536d6af01f783e64bd439a8989d8c9a5b
parent 937 ef78e0727d4acf3939a21967f39d4f4b92198d7a
child 939 2ddcd7844a16fc75b8a3702ead63dcf3cc882729
push idunknown
push userunknown
push dateunknown
bugs17601
Bugzilla bug #17601: fixed memory leak and some other problems in our thread-private data code. Modified files: primpl.h, ptthread.c, prcthr.c, prtpd.c, pruthr.c.
pr/include/private/primpl.h
pr/src/pthreads/ptthread.c
pr/src/threads/combined/pruthr.c
pr/src/threads/prcthr.c
pr/src/threads/prtpd.c
--- a/pr/include/private/primpl.h
+++ b/pr/include/private/primpl.h
@@ -1379,23 +1379,16 @@ struct PRThreadStack {
         PRThread* thr;          /* back pointer to thread owning this stack */
 
 #if defined(_PR_PTHREADS)
 #else /* defined(_PR_PTHREADS) */
     _MDThreadStack md;
 #endif /* defined(_PR_PTHREADS) */
 };
 
-/*
- * Thread private data destructor array
- * There is a destructor (or NULL) associated with each key and
- *    applied to all threads known to the system.
- *  Storage allocated in prtpd.c.
- */
-extern PRThreadPrivateDTOR *_pr_tpd_destructors;
 extern void _PR_DestroyThreadPrivate(PRThread*);
 
 typedef void (PR_CALLBACK *_PRStartFn)(void *);
 
 struct PRThread {
     PRUint32 state;                 /* thread's creation state */
     PRThreadPriority priority;      /* apparent priority, loosly defined */
 
--- a/pr/src/pthreads/ptthread.c
+++ b/pr/src/pthreads/ptthread.c
@@ -44,17 +44,16 @@ extern PRLock *_pr_sleeplock;
 
 static struct _PT_Bookeeping
 {
     PRLock *ml;                 /* a lock to protect ourselves */
     PRCondVar *cv;              /* used to signal global things */
     PRInt32 system, user;       /* a count of the two different types */
     PRUintn this_many;          /* number of threads allowed for exit */
     pthread_key_t key;          /* private private data key */
-    pthread_key_t highwater;    /* ordinal value of next key to be allocated */
     PRThread *first, *last;     /* list of threads we know about */
 #if defined(_PR_DCETHREADS) || defined(_POSIX_THREAD_PRIORITY_SCHEDULING)
     PRInt32 minPrio, maxPrio;   /* range of scheduling priorities */
 #endif
 } pt_book = {0};
 
 static void _pt_thread_death(void *arg);
 static void init_pthread_gc_support(void);
@@ -625,63 +624,16 @@ PR_IMPLEMENT(void) PR_SetThreadPriority(
 		if (rv != 0)
 			rv = -1;
     }
 #endif
 
     thred->priority = newPri;
 }  /* PR_SetThreadPriority */
 
-#if 0
-PR_IMPLEMENT(PRStatus) PR_NewThreadPrivateIndex(
-    PRUintn *newIndex, PRThreadPrivateDTOR destructor)
-{
-    int rv;
-
-    if (!_pr_initialized) _PR_ImplicitInitialization();
-
-    rv = _PT_PTHREAD_KEY_CREATE((pthread_key_t*)newIndex, destructor);
-
-    if (0 == rv)
-    {
-        PR_Lock(pt_book.ml);
-        if (*newIndex >= pt_book.highwater)
-            pt_book.highwater = *newIndex + 1;
-        PR_Unlock(pt_book.ml);
-        return PR_SUCCESS;
-    }
-    PR_SetError(PR_UNKNOWN_ERROR, rv);
-    return PR_FAILURE;
-}  /* PR_NewThreadPrivateIndex */
-
-PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn index, void *priv)
-{
-    PRIntn rv;
-    if ((pthread_key_t)index >= pt_book.highwater)
-    {
-        PR_SetError(PR_TPD_RANGE_ERROR, 0);
-        return PR_FAILURE;
-    }
-    rv = pthread_setspecific((pthread_key_t)index, priv);
-    PR_ASSERT(0 == rv);
-    if (0 == rv) return PR_SUCCESS;
-
-    PR_SetError(PR_UNKNOWN_ERROR, rv);
-    return PR_FAILURE;
-}  /* PR_SetThreadPrivate */
-
-PR_IMPLEMENT(void*) PR_GetThreadPrivate(PRUintn index)
-{
-    void *result = NULL;
-    if ((pthread_key_t)index < pt_book.highwater)
-        _PT_PTHREAD_GETSPECIFIC((pthread_key_t)index, result);
-    return result;
-}  /* PR_GetThreadPrivate */
-#endif
-
 PR_IMPLEMENT(PRStatus) PR_Interrupt(PRThread *thred)
 {
     /*
     ** If the target thread indicates that it's waiting,
     ** find the condition and broadcast to it. Broadcast
     ** since we don't know which thread (if there are more
     ** than one). This sounds risky, but clients must
     ** test their invariants when resumed from a wait and
@@ -782,16 +734,17 @@ static void _pt_thread_death(void *arg)
         thred->prev->next = thred->next;
         if (NULL == thred->next)
             pt_book.last = thred->prev;
         else
             thred->next->prev = thred->prev;
         PR_Unlock(pt_book.ml);
     }
     _PR_DestroyThreadPrivate(thred);
+    PR_Free(thred->privateData);
     if (NULL != thred->errorString)
         PR_Free(thred->errorString);
     if (NULL != thred->io_cv)
         PR_DestroyCondVar(thred->io_cv);
     PR_Free(thred->stack);
 #if defined(DEBUG)
     memset(thred, 0xaf, sizeof(PRThread));
 #endif /* defined(DEBUG) */
--- a/pr/src/threads/combined/pruthr.c
+++ b/pr/src/threads/combined/pruthr.c
@@ -309,33 +309,35 @@ static void
 
 void
 _PR_NativeDestroyThread(PRThread *thread)
 {
     if(thread->term) {
         PR_DestroyCondVar(thread->term);
         thread->term = 0;
     }
-
+    if (NULL != thread->privateData) {
+        PR_ASSERT(0 != thread->tpdLength);
+        PR_DELETE(thread->privateData);
+        thread->tpdLength = 0;
+    }
     PR_DELETE(thread->stack);
     _PR_DestroyThread(thread);
 }
 
 void
 _PR_UserDestroyThread(PRThread *thread)
 {
     if(thread->term) {
         PR_DestroyCondVar(thread->term);
         thread->term = 0;
     }
-    if (NULL != thread->privateData)
-    {
+    if (NULL != thread->privateData) {
         PR_ASSERT(0 != thread->tpdLength);
         PR_DELETE(thread->privateData);
-        thread->privateData = NULL;
         thread->tpdLength = 0;
     }
     _MD_FREE_LOCK(&thread->threadLock);
     if (thread->threadAllocatedOnStack == 1) {
         _PR_MD_CLEAN_THREAD(thread);
         /*
           *  Because the no_sched field is set, this thread/stack will
           *  will not be re-used until the flag is cleared by the thread
--- a/pr/src/threads/prcthr.c
+++ b/pr/src/threads/prcthr.c
@@ -35,27 +35,19 @@ extern PRLock *_pr_sleeplock;  /* alloca
 **
 **
 ** Clean up a thread object, releasing all of the attached data. Do not
 ** free the object itself (it may not have been malloc'd)
 */
 void _PR_CleanupThread(PRThread *thread)
 {
     PRUintn i;
-    void **ptd;
-    PRThreadPrivateDTOR *destructor;
 
     /* Free up per-thread-data */
-    ptd = thread->privateData;
-    destructor = _pr_tpd_destructors;
-    for (i = 0; i < thread->tpdLength; i++, ptd++, destructor++)
-    {
-            if (*destructor && *ptd) (**destructor)(*ptd);
-            *ptd = NULL;
-    }
+    _PR_DestroyThreadPrivate(thread);
 
     /* Free any thread dump procs */
     if (thread->dumpArg) {
         PR_DELETE(thread->dumpArg);
     }
     thread->dump = 0;
 
     PR_DELETE(thread->errorString);
--- a/pr/src/threads/prtpd.c
+++ b/pr/src/threads/prtpd.c
@@ -56,27 +56,24 @@
 ** Some local variables report warnings on Win95 because the code paths 
 ** using them are conditioned on HAVE_CUSTOME_USER_THREADS.
 ** The pragma suppresses the warning.
 ** 
 */
 #pragma warning(disable : 4101)
 #endif
 
-#define _PR_TPD_MODULO 8                /* vectors are extended by this much */
 #define _PR_TPD_LIMIT 128               /* arbitary limit on the TPD slots */
 static PRInt32 _pr_tpd_length = 0;      /* current length of destructor vector */
 static PRInt32 _pr_tpd_highwater = 0;   /* next TPD key to be assigned */
-PRThreadPrivateDTOR *_pr_tpd_destructors = NULL;
+static PRThreadPrivateDTOR *_pr_tpd_destructors = NULL;
                                         /* the destructors are associated with
                                             the keys, therefore asserting that
                                             the TPD key depicts the data's 'type' */
 
-/* Lock protecting the index assignment of per-thread-private data table */
-
 /*
 ** Initialize the thread private data manipulation
 */
 void _PR_InitTPD()
 {
     _pr_tpd_destructors = (PRThreadPrivateDTOR*)
         PR_CALLOC(_PR_TPD_LIMIT * sizeof(PRThreadPrivateDTOR*));
     PR_ASSERT(NULL != _pr_tpd_destructors);
@@ -99,43 +96,43 @@ void _PR_CleanupTPD(void)
 ** The index independently maintains specific values for each binding thread. 
 ** A thread can only get access to its own thread-specific-data.
 **
 ** Upon a new index return the value associated with the index for all threads
 ** is NULL, and upon thread creation the value associated with all indices for 
 ** that thread is NULL. 
 **
 **     "dtor" is the destructor function to invoke when the private
-**       data is destroyed
+**       data is set or destroyed
 **
 ** Returns PR_FAILURE if the total number of indices will exceed the maximun 
 ** allowed.
 */
 
 PR_IMPLEMENT(PRStatus) PR_NewThreadPrivateIndex(
     PRUintn *newIndex, PRThreadPrivateDTOR dtor)
 {
     PRStatus rv;
     PRInt32 index;
 
     if (!_pr_initialized) _PR_ImplicitInitialization();
 
     PR_ASSERT(NULL != newIndex);
     PR_ASSERT(NULL != _pr_tpd_destructors);
 
-    index = PR_AtomicIncrement(&_pr_tpd_highwater);  /* allocate index */
-    if (_PR_TPD_LIMIT < index)
+    index = PR_AtomicIncrement(&_pr_tpd_highwater) - 1;  /* allocate index */
+    if (_PR_TPD_LIMIT <= index)
     {
         PR_SetError(PR_TPD_RANGE_ERROR, 0);
         rv = PR_FAILURE;  /* that's just wrong */
     }
     else
     {
         _pr_tpd_destructors[index] = dtor;  /* record destructor @index */
-        *newIndex = (PRIntn)index;  /* copy into client's location */
+        *newIndex = (PRUintn)index;  /* copy into client's location */
         rv = PR_SUCCESS;  /* that's okay */
     }
 
     return rv;
 }
 
 /*
 ** Define some per-thread-private data.
@@ -145,119 +142,118 @@ PR_IMPLEMENT(PRStatus) PR_NewThreadPriva
 ** If the per-thread private data table has a previously registered
 ** destructor function and a non-NULL per-thread-private data value,
 ** the destructor function is invoked.
 **
 ** This can return PR_FAILURE if index is invalid (ie., beyond the current
 ** high water mark) or memory is insufficient to allocate an exanded vector.
 */
 
-PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn his, void *priv)
+PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn index, void *priv)
 {
-    PRStatus rv = PR_SUCCESS;
-    PRInt32 index = (PRInt32)his;
     PRThread *self = PR_GetCurrentThread();
 
     /*
     ** The index being set might not have a sufficient vector in this
     ** thread. But if the index has been allocated, it's okay to go
     ** ahead and extend this one now.
     */
-    if (index > _pr_tpd_highwater)
+    if ((index >= _PR_TPD_LIMIT) || (index >= _pr_tpd_highwater))
     {
         PR_SetError(PR_TPD_RANGE_ERROR, 0);
-        rv = PR_FAILURE;
+        return PR_FAILURE;
     }
-    else
+
+    PR_ASSERT(((NULL == self->privateData) && (0 == self->tpdLength))
+        || ((NULL != self->privateData) && (0 != self->tpdLength)));
+
+    if ((NULL == self->privateData) || (self->tpdLength <= index))
     {
-        if ((NULL == self->privateData) || (self->tpdLength <= (PRUint32)index))
-        {
-            void *extension = PR_CALLOC(_pr_tpd_length * sizeof(void*));
-            PR_ASSERT(
-                ((NULL == self->privateData) && (0 == self->tpdLength))
-                || ((NULL != self->privateData) && (0 != self->tpdLength)));
-            if (NULL != extension)
-            {
-                (void)memcpy(
-                    extension, self->privateData,
-                    self->tpdLength * sizeof(void*));
-                self->tpdLength = _pr_tpd_length;
-                self->privateData = (void**)extension;
-            }
-        }
-        /*
-        ** There wasn't much chance of having to call the destructor
-        ** unless the slot already existed.
-        */
-        else if (self->privateData[index] && _pr_tpd_destructors[index])
-        {
-            void *data = self->privateData[index];
-            self->privateData[index] = NULL;
-            (*_pr_tpd_destructors[index])(data);
-        }
-
-        /*
-        ** If the thread's private data is still NULL, then we couldn't
-        ** fix the problem. We must be outa-memory (again).
-        */
-        if (NULL == self->privateData)
+        void *extension = PR_CALLOC(_pr_tpd_length * sizeof(void*));
+        if (NULL == extension)
         {
             PR_SetError(PR_OUT_OF_MEMORY_ERROR, 0);
-            rv = PR_FAILURE;
+            return PR_FAILURE;
         }
-        else self->privateData[index] = priv;
+        (void)memcpy(
+            extension, self->privateData,
+            self->tpdLength * sizeof(void*));
+        PR_DELETE(self->privateData);
+        self->tpdLength = _pr_tpd_length;
+        self->privateData = (void**)extension;
+    }
+    /*
+    ** There wasn't much chance of having to call the destructor
+    ** unless the slot already existed.
+    */
+    else if (self->privateData[index] && _pr_tpd_destructors[index])
+    {
+        void *data = self->privateData[index];
+        self->privateData[index] = NULL;
+        (*_pr_tpd_destructors[index])(data);
     }
 
-    return rv;
+    PR_ASSERT(index < self->tpdLength);
+    self->privateData[index] = priv;
+
+    return PR_SUCCESS;
 }
 
 /*
 ** Recover the per-thread-private data for the current thread. "index" is
 ** the index into the per-thread private data table. 
 **
 ** The returned value may be NULL which is indistinguishable from an error 
 ** condition.
 **
 */
 
-PR_IMPLEMENT(void*) PR_GetThreadPrivate(PRUintn his)
+PR_IMPLEMENT(void*) PR_GetThreadPrivate(PRUintn index)
 {
-    PRInt32 index = (PRInt32)his;
     PRThread *self = PR_GetCurrentThread();
-    void *tpd = ((NULL == self->privateData) || ((PRUint32)index >= self->tpdLength)) ?
+    void *tpd = ((NULL == self->privateData) || (index >= self->tpdLength)) ?
         NULL : self->privateData[index];
 
     return tpd;
 }
 
 /*
 ** Destroy the thread's private data, if any exists. This is called at
 ** thread termination time only. There should be no threading issues
 ** since this is being called by the thread itself.
 */
 void _PR_DestroyThreadPrivate(PRThread* self)
 {
+#define _PR_TPD_DESTRUCTOR_ITERATIONS 4
+
     if (NULL != self->privateData)  /* we have some */
     {
-        PRBool clean = PR_TRUE;
-        PRInt32 index, passes = 4;
+        PRBool clean;
+        PRUint32 index;
+        PRInt32 passes = _PR_TPD_DESTRUCTOR_ITERATIONS;
         PR_ASSERT(0 != self->tpdLength);
         do
         {
-            for (index = 0; (PRUint32)index < self->tpdLength; ++index)
+            clean = PR_TRUE;
+            for (index = 0; index < self->tpdLength; ++index)
             {
                 void *priv = self->privateData[index];  /* extract */
                 if (NULL != priv)  /* we have data at this index */
                 {
                     if (NULL != _pr_tpd_destructors[index])
                     {
                         self->privateData[index] = NULL;  /* precondition */
                         (*_pr_tpd_destructors[index])(priv);  /* destroy */
                         clean = PR_FALSE;  /* unknown side effects */
                     }
                 }
             }
-        } while ((passes-- > 0) && !clean);  /* limit # of passes */
-        PR_DELETE(self->privateData);  /* that's just this thread's vector */
+        } while ((--passes > 0) && !clean);  /* limit # of passes */
+        /*
+        ** We give up after a fixed number of passes. Any non-NULL
+        ** thread-private data value with a registered destructor
+        ** function is not destroyed.
+        */
+        memset(self->privateData, 0, self->tpdLength * sizeof(void*));
     }
 }  /* _PR_DestroyThreadPrivate */
 
 #endif /* !XP_BEOS */