33175 - MLK: nsBorderEdge out of nsBorderEdges out of nsTableCellFrame::InitCellFrame()

Status: VERIFIED WORKSFORME

(Core :: Layout: Tables, defect, P3)

Description

[Bruce Mitchener]

This leak is present when loading sample #4 in viewer.  To fix it, nsBorderEdges
(as defined at the URL above), needs to have a destructor to release the
nsBorderEdge stuff stuffed into the mBorderEdges->mEdges nsVoidArray.

         MLK: 288 bytes leaked in 12 blocks
         This memory was allocated from:
               malloc         [rtlib.o]
               __bUiLtIn_nEw  [libxpcom.so]
               __builtin_new  [rtlib.o]
               nsTableCellFrame::InitCellFrame(int) [nsTableCellFrame.cpp:176]
                        PRInt32 rowspan =
tableFrame->GetEffectiveRowSpan(*this);
                        PRInt32 i;
                        for (i=0; i<rowspan; i++) {
               =>         nsBorderEdge *borderToAdd = new nsBorderEdge();
                         
mBorderEdges->mEdges[NS_SIDE_LEFT].AppendElement(borderToAdd);
                          borderToAdd = new nsBorderEdge();
                         
mBorderEdges->mEdges[NS_SIDE_RIGHT].AppendElement(borderToAdd);
               nsCellMap::AppendCell(nsTableCellMap&,nsTableCellFrame&,int,int)
[nsCellMap.cpp:716]
              
nsCellMap::ExpandWithRows(nsIPresContext*,nsTableCellMap&,nsVoidArray&,int)
[nsCellMap.cpp:928]
              
nsCellMap::InsertRows(nsIPresContext*,nsTableCellMap&,nsVoidArray&,int,int)
[nsCellMap.cpp:629]
              
nsTableCellMap::InsertRows(nsIPresContext*,nsTableRowGroupFrame&,nsVoidArray&,int,int)
[nsCellMap.cpp:277]
              
nsTableFrame::InsertRows(nsIPresContext&,nsTableRowGroupFrame&,nsVoidArray&,int,int)
[nsTableFrame.cpp:992]
               nsTableFrame::InsertRowGroups(nsIPresContext&,nsIFrame*,int)
[nsTableFrame.cpp:1134]
               nsTableFrame::AppendRowGroups(nsIPresContext&,nsIFrame*)
[nsTableFrame.cpp:1050]
              
nsTableFrame::SetInitialChildList(nsIPresContext*,nsIAtom*,nsIFrame*)
[nsTableFrame.cpp:343]
              
nsCSSFrameConstructor::ConstructTableFrame(nsIPresShell*,nsIPresContext*,nsFrameConstructorState&,nsIContent*,nsIFrame*,nsIStyleContext*,nsIFrame*&,nsTableCreator&)
[nsCSSFrameConstructor.cpp:1506]
              
nsCSSFrameConstructor::ConstructFrameByDisplayType(nsIPresShell*,nsIPresContext*,nsFrameConstructorState&,const
nsStyleDisplay*,nsIContent*,nsIFrame*,nsIStyleContext*,nsFrameItems&)
[nsCSSFrameConstructor.cpp:5621]
              
nsCSSFrameConstructor::ConstructFrame(nsIPresShell*,nsIPresContext*,nsFrameConstructorState&,nsIContent*,nsIFrame*,nsFrameItems&)
[nsCSSFrameConstructor.cpp:6139]
              
nsCSSFrameConstructor::ContentAppended(nsIPresContext*,nsIContent*,int)
[nsCSSFrameConstructor.cpp:6745]
               StyleSetImpl::ContentAppended(nsIPresContext*,nsIContent*,int)
[nsStyleSet.cpp:956]
               PresShell::ContentAppended(nsIDocument*,nsIContent*,int)
[nsPresShell.cpp:2745]
               nsDocument::ContentAppended(nsIContent*,int)
[nsDocument.cpp:1654]
               nsHTMLDocument::ContentAppended(nsIContent*,int)
[nsHTMLDocument.cpp:1204]
               HTMLContentSink::NotifyAppend(nsIContent*,int)
[nsHTMLContentSink.cpp:3980]
               SinkContext::FlushTags(int) [nsHTMLContentSink.cpp:1919]
               SinkContext::DidAddContent(nsIContent*,int)
[nsHTMLContentSink.cpp:1237]
               SinkContext::FlushText(int*,int) [nsHTMLContentSink.cpp:2021]
               SinkContext::FlushTextAndRelease(int*)
[nsHTMLContentSink.cpp:419]
               SinkContext::CloseContainer(const nsIParserNode&)
[nsHTMLContentSink.cpp:1353]
               HTMLContentSink::CloseContainer(const nsIParserNode&)
[nsHTMLContentSink.cpp:2917]
               CNavDTD::CloseContainer(const nsIParserNode*,nsHTMLTag,int)
[CNavDTD.cpp:3010]
               CNavDTD::CloseContainersTo(int,nsHTMLTag,int) [CNavDTD.cpp:3046]
               CNavDTD::CloseContainersTo(nsHTMLTag,int) [CNavDTD.cpp:3216]
         Block of 24 bytes (12 times); last block at 0xc4e308

Comment 1

[Mike Shaver (:shaver emeritus)]

I don't think we want to add a destructor.  The comment above the struct
definition shows:

  * owner of this struct is responsible for freeing any data stored in mEdges

I'll attach a patch to delete each of the elements in the mBorderEdges->mEdges
array.

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: Add DeleteBorderEdges to properly free nsBorderEdge structures.

Comment 3

[Bruce Mitchener]

This fixed the leak for me.  I didn't see other places in the tree that needed 
to free this data as well.

Comment 4

[Mike Shaver (:shaver emeritus)]

karnaze, can you review and/or apply?  Thanks!

Comment 5

[karnaze (gone)]

I'm reluctant to spend any time on this because I am planning to rewrite the 
border collapse code. If someone wants to check it in, please do so.

Comment 6

[karnaze (gone)]

Moving to M17.

Comment 7

[Mike Shaver (:shaver emeritus)]

Is that ``M17 for the rewrite'' or ``M17 for checking in the patch here that
fixed the memory leak''?

Comment 8

[karnaze (gone)]

For the rewrite. 

Comment 9

[Bernd Mielke]

Adding donttest keyword so this doesn't show up on the bugathon search page.

Comment 10

[karnaze (gone)]

Adding dependency on 41262.

Comment 11

[Marc Attinasi]

Tentatively marking beta3- because the fix for 41262 is claimed to make this a 
moot bug. Failure to fix 41262 should cause this bug to be reconsidered.

Comment 12

[Judson Valeski]

41262 is minused. opening up for re-consideration as we need to nail down leaks.

Comment 13

[Bruce Mitchener]

*** Bug 51595 has been marked as a duplicate of this bug. ***

Comment 14

[Bruce Mitchener]

Months ago, this patch was created and tested.  A few months later, yet still 
months ago, that it was asked that karnaze be given time to do his thing.

requesting a= to check this patch in.

Comment 15

[Brendan Eich [:brendan]]

a=[e-mail]

Time is money, eh?

/be

Comment 16

[karnaze (gone)]

Marking nsbeta3+. Any takers on checking this in?

Comment 17

[buster]

r=buster.  commercial tree closed, shaver, you wanna check this in?  already 
a=brendan, and a=karnaze (module owner)

Comment 18

[Jason Eager]

This has all the requirements for checkin, it should be grandfathered in.
Getting people's attention...

Comment 19

[karnaze (gone)]

Adding rtm+.

Comment 20

[Phil Peterson]

Not holding PR3 for this. Marking nsbeta3-

Comment 21

[karnaze (gone)]

Marking rtm-, removing dependency on 41462, and adding dependency on 49490. When 
49490 is fixed this will be fixed.

Comment 22

[Gervase Markham [:gerv]]

"When 49490 is fixed this will be fixed."

karnaze: you fixed bug 49490 quite a while ago. There's a patch sitting here - 
is it ready for checkin?

Gerv 

Comment 23

[Håkan Waara]

This patch has all it needs to get checked in (a=, r=), shaver - Just Do It! :)

Comment 24

[Amarendra Hanumanula]

QA contact update

Comment 25

[karnaze (gone)]

I'm marking this worksforme, because the code in question has been removed and 
will be replaced at some point.

Comment 26

[Amarendra Hanumanula]

 Since the code has been removed I am marking it verified.... If you think that 
if this bug has to be reopened please reopen this bug