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.

5 files changed, 60 insertions, 124 deletions

diff --git a/pr/include/private/primpl.h b/pr/include/private/primpl.h
index 50b70dbf..20b61ca5 100644
--- a/pr/include/private/primpl.h
+++ b/pr/include/private/primpl.h
@@ -1384,13 +1384,6 @@ struct PRThreadStack {
 #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 *);
diff --git a/pr/src/pthreads/ptthread.c b/pr/src/pthreads/ptthread.c
index 2df58a06..d2760f82 100644
--- a/pr/src/pthreads/ptthread.c
+++ b/pr/src/pthreads/ptthread.c
@@ -49,7 +49,6 @@ static struct _PT_Bookeeping
     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 */
@@ -630,53 +629,6 @@ PR_IMPLEMENT(void) PR_SetThreadPriority(PRThread *thred, PRThreadPriority newPri
     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)
 {
     /*
@@ -787,6 +739,7 @@ static void _pt_thread_death(void *arg)
         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)
diff --git a/pr/src/threads/combined/pruthr.c b/pr/src/threads/combined/pruthr.c
index f9aba6eb..a55fc39b 100644
--- a/pr/src/threads/combined/pruthr.c
+++ b/pr/src/threads/combined/pruthr.c
@@ -314,7 +314,11 @@ _PR_NativeDestroyThread(PRThread *thread)
         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);
 }
@@ -326,11 +330,9 @@ _PR_UserDestroyThread(PRThread *thread)
         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);
diff --git a/pr/src/threads/prcthr.c b/pr/src/threads/prcthr.c
index 5f9af066..91788453 100644
--- a/pr/src/threads/prcthr.c
+++ b/pr/src/threads/prcthr.c
@@ -40,17 +40,9 @@ extern PRLock *_pr_sleeplock;  /* allocated and initialized in prinit */
 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) {
diff --git a/pr/src/threads/prtpd.c b/pr/src/threads/prtpd.c
index ddbbf2a7..f12f8243 100644
--- a/pr/src/threads/prtpd.c
+++ b/pr/src/threads/prtpd.c
@@ -61,17 +61,14 @@
 #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
 */
@@ -104,7 +101,7 @@ void _PR_CleanupTPD(void)
 ** 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.
@@ -121,8 +118,8 @@ PR_IMPLEMENT(PRStatus) PR_NewThreadPrivateIndex(
     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 */
@@ -130,7 +127,7 @@ PR_IMPLEMENT(PRStatus) PR_NewThreadPrivateIndex(
     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 */
     }
 
@@ -150,10 +147,8 @@ PR_IMPLEMENT(PRStatus) PR_NewThreadPrivateIndex(
 ** 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();
 
     /*
@@ -161,52 +156,45 @@ PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn his, void *priv)
     ** 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
-    {
-        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)
+    PR_ASSERT(((NULL == self->privateData) && (0 == self->tpdLength))
+        || ((NULL != self->privateData) && (0 != self->tpdLength)));
+
+    if ((NULL == self->privateData) || (self->tpdLength <= index))
+    {
+        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;
 }
 
 /*
@@ -218,11 +206,10 @@ PR_IMPLEMENT(PRStatus) PR_SetThreadPrivate(PRUintn his, void *priv)
 **
 */
 
-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;
@@ -235,14 +222,18 @@ PR_IMPLEMENT(void*) PR_GetThreadPrivate(PRUintn his)
 */
 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 */
@@ -255,8 +246,13 @@ void _PR_DestroyThreadPrivate(PRThread* self)
                     }
                 }
             }
-        } 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 */