Closed Bug 95829 Opened 19 years ago Closed 12 years ago

memory leak in prcmon.c

Categories

(NSPR :: NSPR, defect, P2, major)

4.1.2

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jeff, Assigned: wtc)

Details

(Keywords: memory-leak)

Attachments

(1 file, 5 obsolete files)

in nsprpub/pr/src/threads/prcmon.c, various static variables
are not destroyed, including _pr_mcacheLock, free_entries,
hash_buckets, and the various monitors created for each entry.

consider adding the following function to prcmon.c and calling
from PR_Cleanup():

void _PR_CleanupCMon(void)
{
	// TODO should create a _PR_DESTROY_LOCK_MCACHE() macro for this...
	if (_pr_mcacheLock)
	{
		PR_DestroyLock(_pr_mcacheLock);
		_pr_mcacheLock = NULL;
	}

	if (free_entries)
	{
	    MonitorCacheEntry * p;
	    int k;

	    for (k=0, p=free_entries; (k<num_free_entries); k++, p++)
		if (p->mon)
		    PR_DestroyMonitor(p->mon);

	    PR_DELETE(free_entries);
	    free_entries = NULL;
	    num_free_entries = 0;
	}

	if (hash_buckets)
	{
	    PR_DELETE(hash_buckets);
	    hash_buckets = NULL;
	    num_hash_buckets = 0;
	    num_hash_buckets_log2 = 0;
	}
}
Awesome again.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: mlk, patch, review
Blocks: 92580
I took a look at this leak.

It is not trivial to free the cached monitors.  The
code fragment that Jeff provided won't work:
1. 'free_entries' may be pointing into an allocated
   array of MonitorCacheEntry's because CreateMonitor()
   may move 'free_entries'.
       /* Make a new monitor */
       p = free_entries;
       free_entries = p->next;
       num_free_entries--;
   So we can't say
       PR_DELETE(free_entries);
2. 'free_entries' does not always point to an array of
   MonitorCacheEntry's.  In fact, it points to arrays
   of MonitorCacheEntry's concatenated together by
   ExpandMonitorCache():
       for (i = 0, p = new_entries; i < added - 1; i++, p++)
           p->next = p + 1;
       p->next = free_entries;
       free_entries = new_entries;
       num_free_entries += added;
   So this for loop in Jeff's patch won't work:
       for (k=0, p=free_entries; (k<num_free_entries); k++, p++)
           if (p->mon)
               PR_DestroyMonitor(p->mon);

With the current data structure definition, it is impossible
to free the cached monitors.  Without redesigning the data
structures, the best I can do is to destroy _pr_mcachLock
and hash_buckets.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Future
OK, I took a stab at this.  I added a new datatype
MonitorCacheEntryBlock, which is an array of
MonitorCacheEntry's plus a pointer to link the
MonitorCacheEntryBlock's together.  Then I added a
global variable mcache_blocks, which is a linked
list of MonitorCacheEntryBlock's.  mcache_blocks
allows me to destroy the cached monitors in
_PR_CleanupCMon().

I will attach a patch.
No longer blocks: 92580
OS: Windows 2000 → All
Priority: P3 → P2
Hardware: PC → All
QA Contact: wtchang → nspr
What ever happened with this patch / bug?
Comment on attachment 49053 [details] [diff] [review]
Proposed patch, based on the suggestion of jeff@NerdOne.com.

Maybe requesting review will resurrect this patch.
Attachment #49053 - Flags: review?(julien.pierre.boogz)
Target Milestone: Future → 4.7
bugzilla's patch diff tool barfs on the first patch when you ask 
it to display more lines of context.  
Maybe it will like this one better.
The patch diff tool didn't like the second patch any better.
Filed bug 396852 about bugzilla's patch diff tool barfing.
I brought the previous patch forward to the trunk, Fixed some 
warnings, and made one change proposed in bug 334285.  
Julien, please review.
Attachment #49053 - Attachment is obsolete: true
Attachment #281605 - Attachment is obsolete: true
Attachment #49053 - Flags: review?(julien.pierre.boogz)
Attachment #281618 - Flags: review?(julien.pierre.boogz)
Attachment #281618 - Flags: review?(rrelyea)
Comment on attachment 281618 [details] [diff] [review]
patch v2, brought forward to trunk, with fix from bug 334285

r+ = rrelyea
Attachment #281618 - Flags: review?(rrelyea) → review+
I just found another NSPR leak bug with an attached patch that seems to
duplicate some of the content of this patch.  See bug 367075. 
Now I'm not so certain that the reviewed patch for THIS bug should be
checked in, or not.  Wan-Teh, please advise.  
Comment on attachment 281618 [details] [diff] [review]
patch v2, brought forward to trunk, with fix from bug 334285

It would be nice to take the best ideas from both
patches.  The patches differ in how they keep track
of the blocks of cache entries -- I link them together,
whereas Brodie stores them in a fixed-size array --
and how the monitors are destroyed.  Brodie also resets
several global variables.

>Index: nsprpub/pr/src/misc/prinit.c
...
>         _PR_CleanupIO();
>+	_PR_CleanupCMon();

Move the line to the right by one space.
Attachment #281618 - Attachment is obsolete: true
Attachment #281618 - Flags: review?(julien.pierre.boogz)
Attachment #282861 - Flags: review?(rrelyea)
Attached patch patch v3.1 (obsolete) — Splinter Review
I took another change from Brodie's patch in bug 367075.
In the code used only for Windows in prinit.c, he noted that
_PR_CleanupCMon should be called after _PR_CleanupThreads is
called.  I don't know why, and the ordering isn't important
in the current code, but it seems prudent to use his ordering
just in case we add new code to _PR_CleanupThreads (which has
a TODO comment).
Attachment #282861 - Attachment is obsolete: true
Attachment #282861 - Flags: review?(rrelyea)
Attachment #282862 - Flags: review?(rrelyea)
The main difference between patches v2 and v3 is that in patch v3 any monitor that is not on the free list is leaked. This would, of course, be an application bug.

In v2 of the bug these monitors would get freed up, but an application that called PR_Shutdown and then tried to reference on of those monitors would likely crash.

If that was the intent I'll give it an r+
Yes, that was the intent.  The v3 patch also resets the 'expanding'
and 'OnMonitorRecycle' global variables.
Comment on attachment 282862 [details] [diff] [review]
patch v3.1

r+ rrelyea
Attachment #282862 - Flags: review?(rrelyea) → review+
is this ready for check in?
Comment on attachment 282862 [details] [diff] [review]
patch v3.1

Additional r=nelson.
This patch also fixes bug 334285.  I suggest that this patch be committed, 
rather than the patch attached to bug 334285.

Here are some changes I would suggest to this patch before it is 
committed.  In function ExpandMonitorCache this patch changes:

>-    for (i = 0, added = 0, p = new_entries; i < entries; i++, p++, added++) {
>+    for (i = 0, added = 0, p = new_block->entries; i < entries; 
>+         i++, p++, added++) {
>         p->mon = PR_NewMonitor();
>         if (!p->mon)
>             break;
>     }
>     if (added != entries) 
  ... <lines that do not use the variable i> ...
>-    for (i = 0, p = new_entries; i < added - 1; i++, p++)
>+    for (i = 0, p = new_block->entries; i < added - 1; i++, p++)
>         p->next = p + 1;

1. In the first for-loop above, the variable i is needlessly initialized to 
zero and counted up.  Then, before it is ever used, it is initialized to zero 
again in the second for loop.  So I would remove the initialization and 
increment of variable i from the first loop.  e.g.

>+    for (added = 0, p = new_block->entries; added < entries; 
>+         p++, added++) {

2. The second loop above will recompute the expression (added - 1) each 
time through the loop.  It would be slightly less expensive to rewrite that
second for loop as:
>+    for (i = added - 1, p = new_block->entries; i > 0 ; i--, p++)
Attachment #282862 - Flags: review+
I checked in this patch on the NSPR trunk for NSPR 4.7.

Checking in src/misc/prinit.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prinit.c,v  <--  prinit.c
new revision: 3.48; previous revision: 3.47
done
Checking in src/pthreads/ptthread.c;
/cvsroot/mozilla/nsprpub/pr/src/pthreads/ptthread.c,v  <--  ptthread.c
new revision: 3.77; previous revision: 3.76
done
Checking in include/private/primpl.h;
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v  <--  primpl.h
new revision: 3.88; previous revision: 3.87
done
Checking in src/threads/prcmon.c;
/cvsroot/mozilla/nsprpub/pr/src/threads/prcmon.c,v  <--  prcmon.c
new revision: 3.8; previous revision: 3.7
done
Attachment #282862 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(In reply to comment #19)

It helps me understand the code to have the two for loops
look similar, so I only removed the operations on 'added'
from the first for loop.  I hope compilers can move the
computation of 'added - 1' out of the second for loop.
You need to log in before you can comment on or make changes to this bug.