215163 - nsComposerCommandsUpdater leaks due to incorrect timer usage

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Due to the patch in bug 207469, nsComposerCommandsUpdater leaks.

Steps to reproduce:
 1. setenv XPCOM_MEM_LEAK_LOG leak.log
 2. Run ./mozilla -editor
 3. Type a few characters
 4. Close the window with the X in the corner.
 5. Click "Don't Save".

Actual results:
 nsComposerCommandsUpdater object leaked (and perhaps a bunch of others because
of it)

This is caused by the patch in bug 207469.  Each time InitWithCallback is
called, it |AddRefs| the timer callback implementor (the
nsComposerCommandsUpdater), but the callback is only released in the timer's
destructor.

(Maybe the timer API should be better?)

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix to xpcom/threads/nsTimerImpl.cpp

Fixed so you can re-Init* any whichway.  This should go into 1.5b.

/be

Comment 2

[Brendan Eich [:brendan]]

Taking, but Pav owes me review.

/be

Comment 3

[Kathleen :Brade]

I'm guessing this is leaking everywhere... :-)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 129241 [details] [diff] [review]
proposed fix to xpcom/threads/nsTimerImpl.cpp

Do you want to make the same optimization in the nsIObserver case as the
nsITimerCallback case?	Or perhaps in neither case?  (Or do you want to allow
re-initialization with the same callback if the timer is the one holding a
reference?)

Comment 5

[Doug Turner (:dougt)]

can't you do something simpiler?  This does addref when you don't have to, but
it is much easier to read.

NS_IMETHODIMP nsTimerImpl::InitWithCallback(nsITimerCallback *aCallback,
                                             PRUint32 aDelay,
                                             PRUint32 aType)
{
   if (!gThread)
     return NS_ERROR_FAILURE;
 
   if (mCallbackType == CALLBACK_TYPE_INTERFACE)
      NS_RELEASE(mCallback.i);
   else if (mCallbackType == CALLBACK_TYPE_OBSERVER)
      NS_RELEASE(mCallback.o);

   NS_ADDREF(mCallback.i = aCallback);
   mCallbackType = CALLBACK_TYPE_INTERFACE;

   return InitCommon(aType, aDelay);
 }

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix, take 2

Argh, dbaron's right -- for some reason I was concerned with avoiding release
before addref, but if the same interface or observer is being re-inited, the
refcnt will be 2 until the release, then 1, then 2 again (the caller must have
a strong ref to the parameter it passes in).

/be

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Now that you have the same code copied to 4 places, perhaps it would be better
off in an inline function?  |ReleaseCallback()|?

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix, take 3

Dbaron makes me work hard for the money.

/be

Comment 9

[Brendan Eich [:brendan]]

Fixed (I didn't wait for pavlov's r=).

/be

Comment 10

[Stuart Parmenter]

Comment on attachment 129248 [details] [diff] [review]
proposed fix, take 3

r=pavlov

Comment 11

[R.K.Aa.]

I now crash at http://www.bilnorge.no

Comment 12

[R.K.Aa.]

Attachment: backtrace, non-debug

Head of stack:
#0  0x409169fd in nsTimerImpl::InitWithCallback(nsITimerCallback*, unsigned,
unsigned) () from /mozilla/dist/bin/libxpcom.so
#1  0x40bc634d in nsSelectUpdateTimer::Start(nsIPresContext*) ()
   from /mozilla/dist/bin/components/libgklayout.so
#2  0x40bc552b in nsListControlFrame::StartUpdateTimer(nsIPresContext*) ()
   from /mozilla/dist/bin/components/libgklayout.so
#3  0x40bc2d84 in nsListControlFrame::RemoveOption(nsIPresContext*, int) ()
   from /mozilla/dist/bin/components/libgklayout.so
#4  0x40baaac4 in nsComboboxControlFrame::RemoveOption(nsIPresContext*, int) ()

   from /mozilla/dist/bin/components/libgklayout.so
#5  0x40d6ffef in nsHTMLSelectElement::RemoveOptionsFromList(nsIContent*, int,
int) () from /mozilla/dist/bin/components/libgklayout.so
#6  0x40d7052f in nsHTMLSelectElement::WillRemoveOptions(nsIContent*, int) ()
   from /mozilla/dist/bin/components/libgklayout.so
#7  0x40d6fd0e in nsHTMLSelectElement::RemoveChildAt(int, int) ()
   from /mozilla/dist/bin/components/libgklayout.so
#8  0x40ca80b2 in nsGenericElement::doRemoveChild(nsIDOMNode*, nsIDOMNode**) ()

   from /mozilla/dist/bin/components/libgklayout.so
#9  0x40d74c6d in nsHTMLSelectElement::RemoveChild(nsIDOMNode*, nsIDOMNode**)
    () from /mozilla/dist/bin/components/libgklayout.so
#10 0x40d70a1d in nsHTMLSelectElement::Remove(int) ()
   from /mozilla/dist/bin/components/libgklayout.so

Comment 13

[R.K.Aa.]

Filed bug 215250