Bas Schouten (:bas.schouten) Assignee
	Description • 18 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: 

When two threads communicate through an asynchronous proxy call they push additional event queues on the thread, and use those. Once they are done with these they are popped but the actual nsEventQueue object is not deleted, the lock and pages belonging to the event therefore linger in memory. Furthermore event pages are not properly freed. In my case I'm seeing this leak cause memory usage increase by 5 MB/s, going up until the program crashes.

Reproducible: Always

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 18 years ago

Oh, good catch!

Deleting the page shouldn't really be needed because the caller should have gotten all the events from the queue first, which would delete the page, no?.  But it seems like the safe thing to do.  We should also perhaps consider releasing whatever events are left in the queue, if any.

	Bas Schouten (:bas.schouten) Assignee
	Comment 3 • 18 years ago

      if (mOffsetHead == EVENTS_PER_PAGE) {
        Page *dead = mHead;
        mHead = mHead->mNext;
        FreePage(dead);
        mOffsetHead = 0;
      }
Take a note of this particular part of code, see how the logic is flawed? The page only gets deleted if the pointer reaches the end of it... you could alter this by adding logic here like:
if (mOffsetHead == EVENTS_PER_PAGE || (mHead == mTail && mOffsetHead = mOffsetTail))

But this would mean you're deallocating every time the queue is processed and re-allocating every time it gets an event, it's far more efficient to delete the page upon destruction.. as far as I can see that is :)

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 18 years ago

Comment on attachment 256164 [details] [diff] [review]
Patch

Bas, you want to request module owner review on patches.  ;)

And good point on the page thing!

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 18 years ago

Comment on attachment 256164 [details] [diff] [review]
Patch

sr=bzbarsky

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 18 years ago

Checked in.  Bas, thank you very much for the patch!

	Bas Schouten (:bas.schouten) Assignee
	Comment 7 • 18 years ago

My pleasure, thanks for the quick reviews.

	Darin Fisher
	Comment 8 • 18 years ago

Is it really necessary to enter the monitor in the nsEventQueue's destructor?  That implies that more than one consumer might have a reference to the nsEventQueue while the nsEventQueue is being destroyed.  How can that be?  If that is possible, then it surely indicates a problem, no?

	Bas Schouten (:bas.schouten) Assignee
	Comment 9 • 18 years ago

I don't think it is necessary, no. But it seemed like the safest choice when I contributed the patch, considering the monitor is entered on PutEvent and GetEvent as well. I thought someone who knew the code better might be more able to make the 'right' decision on the usage of the monitor.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 18 years ago

Checked in that followup patch.