203596 - Memory leak of 52 bytes from 1 block allocated in FrameManager::CantRenderReplacedElement

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[[e-mail] (gone - use [e-mail] instead)]

Build: Latest trunk pull on Windows 2000 under Purify.

Steps to Reproduce:

1.  mozilla.exe -addressbook
2.  Click 'New Card'
3.  Type "S" into any field.
4.  Hit Save.

    [W] MLK: Memory leak of 52 bytes from 1 block allocated in
FrameManager::CantRenderReplacedElement(nsIPresContext *,nsIFrame *)
        Distribution of leaked blocks
            52 bytes from 1 block of 52 bytes (0x0ace5ff0) 
        Allocation location
        new(UINT)      [new.cpp:23]
        FrameManager::CantRenderReplacedElement(nsIPresContext *,nsIFrame *)
[nsFrameManager.cpp:1289]
              CantRenderReplacedElementEvent* ev;
        
              // Create a new event
     =>       ev = new CantRenderReplacedElementEvent(this, aFrame, mPresShell);
        
              // Add the event to our linked list of posted events
              ev->mNext = mPostedEvents;
        PresShell::CantRenderReplacedElement(nsIPresContext *,nsIFrame *)
[nsPresShell.cpp:3985]
                                             nsIFrame*       aFrame)
        {
          if (mFrameManager) {
     =>     return mFrameManager->CantRenderReplacedElement(aPresContext, aFrame);
          }
        
          return NS_OK;
        nsImageFrame::HandleLoadError(UINT,nsIPresShell *) [nsImageFrame.cpp:497]
            nsIFrame* primaryFrame = nsnull;
            aPresShell->GetPrimaryFrameFor(mContent, &primaryFrame);
            aPresShell->CantRenderReplacedElement(mPresContext,
     =>                                           primaryFrame ? primaryFrame :
this);
            return NS_ERROR_FRAME_REPLACED;
          }
        
        nsImageFrame::Init(nsIPresContext *,nsIContent *,nsIFrame
*,nsStyleContext *,nsIFrame *) [nsImageFrame.cpp:330]
            PRBool loadBlocked = PR_FALSE;
            imageLoader->GetImageBlocked(&loadBlocked);
            rv = HandleLoadError(loadBlocked ? NS_ERROR_IMAGE_BLOCKED :
NS_ERROR_FAILURE,
     =>                          presShell);
          }
        
          return rv;
        nsCSSFrameConstructor::InitAndRestoreFrame(nsIPresContext
*,nsFrameConstructorState&,nsIContent *,nsIFrame *,nsStyleContext *,nsIFrame
*,nsIFrame *) [nsCSSFrameConstructor.cpp:6832]
        
          // Initialize the frame
          rv = aNewFrame->Init(aPresContext, aContent, aParentFrame,
     =>                        aStyleContext, aPrevInFlow);
        
          if (aState.mFrameState && aState.mFrameManager) {
            // Restore frame state for just the newly created frame.
        nsCSSFrameConstructor::ConstructHTMLFrame(nsIPresShell *,nsIPresContext
*,nsFrameConstructorState&,nsIContent *,nsIFrame *,nsIAtom *,int,nsStyleContext
*,nsFrameItems&) [nsCSSFrameConstructor.cpp:4994]
            }
        
            rv = InitAndRestoreFrame(aPresContext, aState, aContent,
     =>                              geometricParent, aStyleContext, nsnull,
newFrame);
            if (rv == NS_ERROR_FRAME_REPLACED) {
              // The frame called CantRenderReplacedElement from inside Init().
 That
              // failed to do anything useful, since the frame was not in the frame
    nsCSSFrameConstructor::ConstructFrameInternal(nsIPresShell *,nsIPresContext
*,nsFrameConstructorState&,nsIContent *,nsIFrame *,nsIAtom *,int,nsStyleContext
*,nsFrameItems&,int) [nsCSSFrameConstructor.cpp:7426]
          // Handle specific frame types
          nsresult rv = ConstructHTMLFrame(aPresShell, aPresContext, aState,
                                           aContent, aParentFrame, aTag,
     =>                                    aNameSpaceID, styleContext, aFrameItems);
        
          // Failing to find a matching HTML frame, try creating a specialized
          // XUL frame. This is temporary, pending planned factoring of this
    nsCSSFrameConstructor::ConstructFrame(nsIPresShell *,nsIPresContext
*,nsFrameConstructorState&,nsIContent *,nsIFrame *,nsFrameItems&)
[nsCSSFrameConstructor.cpp:7323]
          }
          // construct the frame
          rv = ConstructFrameInternal(aPresShell, aPresContext, aState,
aContent, aParentFrame,
     =>                               tag, nameSpaceID, styleContext,
aFrameItems, PR_FALSE);
          if (NS_SUCCEEDED(rv) && pageBreakAfter) {
            // Construct the page break after
            ConstructPageBreakFrame(aPresShell, aPresContext, aState, aContent,
    nsCSSFrameConstructor::ProcessChildren(nsIPresShell *,nsIPresContext
*,nsFrameConstructorState&,nsIContent *,nsIFrame
*,int,nsFrameItems&,int,nsTableCreator *) [nsCSSFrameConstructor.cpp:12245]
                 iter != last;
                 ++iter) {
              rv = ConstructFrame(aPresShell, aPresContext, aState,
nsCOMPtr<nsIContent>(*iter),
     =>                           aFrame, aFrameItems);
        
              if (NS_FAILED(rv))
                return rv;

Comment 1

[Doug Turner (:dougt)]

found the problem

Comment 2

[Doug Turner (:dougt)]

Attachment: properly destroys plevent

Comment 3

[Darin Fisher]

Comment on attachment 123522 [details] [diff] [review]
properly destroys plevent

>+        delete ev;

shouldn't this be PL_DestroyEvent also?  otherwise the lock and cvar
will be leaked.  of course, if that other patch were in the tree...

Comment 4

[Doug Turner (:dougt)]

yup.  it should be (204963, right?)

Comment 5

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

Comment on attachment 123522 [details] [diff] [review]
properly destroys plevent

>       // Post the event
>-      eventQueue->PostEvent(ev);
>+      if (eventQueue->PostEvent(ev) == PR_FAILURE) {
>+        NS_WARNING("Could not post CantRenderReplacedElementEvent");
>+        mPostedEvents = ev->mNext;
>+        delete ev;
>+      }
>     }
>   }

Looking at PL_PostEvent, it looks like PR_FAILURE doesn't mean that it wasn't
stored in the queue -- in fact, it looks like it will always be stored in the
queue.	So perhaps you should skip this half and just do the other half of the
patch?

Comment 6

[Doug Turner (:dougt)]

Agreed.  Only the first part of the diff should be checked in.

Comment 7

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

Comment on attachment 123522 [details] [diff] [review]
properly destroys plevent

OK, then r+sr=dbaron on this part of the patch:

>Index: html/base/src/nsFrameManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/html/base/src/nsFrameManager.cpp,v
>retrieving revision 1.147
>diff -u -1 -0 -r1.147 nsFrameManager.cpp
>--- html/base/src/nsFrameManager.cpp	23 Apr 2003 00:43:03 -0000	1.147
>+++ html/base/src/nsFrameManager.cpp	16 May 2003 18:24:52 -0000
>@@ -1130,22 +1130,23 @@
> 
>       NS_ASSERTION(NS_SUCCEEDED(rv) && eventQueue,
>             "will crash soon due to event holding dangling pointer to frame");
>       if (NS_SUCCEEDED(rv) && eventQueue) {
>         PLEventQueue* plqueue;
> 
>         eventQueue->GetPLEventQueue(&plqueue);
>         NS_ASSERTION(plqueue,
>             "will crash soon due to event holding dangling pointer to frame");
>         if (plqueue) {
>-          // Removes the event and destroys it
>+          // Remove the event and then destroy it
>           PL_DequeueEvent(tmp, plqueue);
>+          PL_DestroyEvent(tmp);
>         }
>       }
>     }
>   }
> }
> 
> void
> FrameManager::HandlePLEvent(CantRenderReplacedElementEvent* aEvent)
> {
> #ifdef NOISY_EVENTS

Comment 8

[Darin Fisher]

uhm... i don't get it.  nsEventQueue::PostEvent can return a failure code
without queuing up the event.  PL_PostEvent may not even get called.  i think we
may seriously have a problem in a lot of code if we ever get to the point where
PL_PostEvent returns a failure code because most everywhere else we either call
PL_DestroyEvent when nsIEventQueue::PostEvent fails or we call operator delete
(which is definitely wrong).  so, we have major bugs here...

Comment 9

[Darin Fisher]

incidentally it appears that PL_PostEvent will never fail on WIN32, but on other
platforms it might.  the fact that it leaves the event in the event queue means
that we would crash if this ever happened.  (not in the case of the frame
manager code, but certainly in the case of a lot of other PostEvent call sites.)

Comment 10

[Doug Turner (:dougt)]

darin, i think the problem that you mention is a problem throughout the mozilla
code.  The first part of this patch addresses the the fundemental problem in
which we *never* delete these events.  I would like to try to get this part into
the tree for 1.4 and wait to address the 204963 post 1.4.  

Comment 11

[Darin Fisher]

yeah, absolutely.  that does make the most sense ;-)

Comment 12

[Asa Dotzler [:asa]]

Comment on attachment 123522 [details] [diff] [review]
properly destroys plevent

a=asa (on behalf of drivers) for checkin to 1.4

Comment 13

[Doug Turner (:dougt)]

Checking in html/base/src/nsFrameManager.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrameManager.cpp,v  <--  nsFrameManager.cpp
new revision: 1.149; previous revision: 1.148
done

Comment 14

[[e-mail] (gone - use [e-mail] instead)]

This is fixed.  

I verified it under a clean tree (debug) Win2000/Purify.

Thanks!