Closed Bug 33175 Opened 24 years ago Closed 23 years ago

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

Categories

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

Product:
Core
Component:
Layout: Tables
Type:
defect

Tracking

()

Status:
VERIFIED WORKSFORME
Milestone:
Future

People

(Reporter: bruce, Assigned: karnaze)

References

(
URL
)

Details

(Keywords: memory-leak, Whiteboard: [nsbeta3-][rtm-])

Attachments

(1 file)

Add DeleteBorderEdges to properly free nsBorderEdge structures.
2.03 KB, patch
Details | Diff | Splinter Review 
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
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.

This fixed the leak for me.  I didn't see other places in the tree that needed 
to free this data as well.
karnaze, can you review and/or apply?  Thanks!

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.
Status: NEW → ASSIGNED
Target Milestone: --- → M16
Moving to M17.
Target Milestone: M16 → M17
Is that ``M17 for the rewrite'' or ``M17 for checking in the patch here that
fixed the memory leak''?

For the rewrite. 
Adding donttest keyword so this doesn't show up on the bugathon search page.
Keywords: donttest
Adding dependency on 41262.
Depends on: 41262
Keywords: nsbeta3
Keywords: patch
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.
Whiteboard: [nsbeta3-]
Target Milestone: M17 → Future
41262 is minused. opening up for re-consideration as we need to nail down leaks.
Whiteboard: [nsbeta3-]
*** Bug 51595 has been marked as a duplicate of this bug. ***
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.

a=brendan@mozilla.org

Time is money, eh?

/be
Marking nsbeta3+. Any takers on checking this in?
Whiteboard: [nsbeta3+]
r=buster.  commercial tree closed, shaver, you wanna check this in?  already 
a=brendan, and a=karnaze (module owner)
This has all the requirements for checkin, it should be grandfathered in.
Getting people's attention...
Keywords: approval
Adding rtm+.
Keywords: rtm
Whiteboard: [nsbeta3+] → [nsbeta3+][rtm+]
Not holding PR3 for this. Marking nsbeta3-
Whiteboard: [nsbeta3+][rtm+] → [nsbeta3-][rtm+]
Marking rtm-, removing dependency on 41462, and adding dependency on 49490. When 
49490 is fixed this will be fixed.
Depends on: 49490
No longer depends on: 41262
Whiteboard: [nsbeta3-][rtm+] → [nsbeta3-][rtm-]
"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
Keywords: mozilla0.9
This patch has all it needs to get checked in (a=, r=), shaver - Just Do It! :)
QA contact update
QA Contact: chrisd → amar
I'm marking this worksforme, because the code in question has been removed and 
will be replaced at some point.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → WORKSFORME
 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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: