Closed Bug 137094 Opened 18 years ago Closed 18 years ago

Memory leak in nsBlockFrame::Reflow()


(Core :: Layout, defect, P3)






(Reporter: pj, Assigned: waterson)




(Keywords: memory-leak)


(2 files, 2 obsolete files)

PR_Malloc prmem.c:433 0x40418db1
nsMemoryImpl::Alloc <...> nsMemoryImpl.cpp:320 0x4034bfc4 
nsMemory::Alloc(unsigned int) nsMemory.cpp:77 0x403869e4 
nsSpaceManager::operator new <...> nsSpaceManager.cpp:144 0x807f411c 
nsBlockFrame::Reflow <...> nsBlockFrame.cpp:718 0x80644dc7 
nsContainerFrame::ReflowChil <...> nsContainerFrame.cpp:784
nsTableCellFrame::Reflow <...> nsTableCellFrame.cpp:939 0x807612e3 
nsContainerFrame::ReflowChil <...> nsContainerFrame.cpp:784
nsTableRowFrame::ReflowChild <...> nsTableRowFrame.cpp:1046
nsTableRowFrame::Reflow <...> nsTableRowFrame.cpp:1455 0x80781da2

718 spaceManager = new nsSpaceManager(shell, this);

dbaron was the last one to touch this line according to cvsblame, bug 102453.
The last time someone at geodesic ran mozilla under their tool (2002.3.26) it
did not have this leak though, something has changed recently I believe.
Keywords: mlk
Steps to reproduce?  What tests does geodesic run?  That page doesn't show any
useful data for me -- perhaps it requires a cookie beyond just what's in that URL?
Try instead maybe. After the newest
mozilla-binary is selected click on a link with a name similar to "Leaks". That
should show truncated call-stacks. 

The page is there to demonstrate their software (seems to do the same job as
Purify more or less), according to what I have gathered  they "just use it" for
a while under it and it pops out line numbers and a call-path. 

Knowing that |spaceManager| is leaked which was allocated in
nsBlockFrame.cpp:718 should give a hint about what to look for though?
Bug 135146 was checked in 02/04/11 - the patch there touches Reflow(). Could
that have anything to do with it?
I'll take a look at the flow-of-control here.
Assignee: attinasi → waterson
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla1.0
Attached file Callsequence.
If one goes to the section "Settings", scroll to the bottom there is a box
where one can set how many functioncalls deep to show. "Submit changes" and go
back to selecting the program again and wait for some time and it should show
up. This does not work in Netscape 4.76 for me, does in mozilla however. This
file is for 30 calls deep, the other leaks in the same place seem to have a
similar calling sequence.
The only way that this should leak is if ReflowDirtyLines fails (the
Prepare*Reflow all return NS_OK unconditionally), or if we've blown the maximum
frame tree depth. I'm dubious that ReflowDirtyLines can really fail in such a
way that we ought to abort all reflow, but...I'm not going to dig right now.

We probably ought to convert the Prepare*Reflow methods to return |void| instead
of |nsresult|, and then look in-depth at ReflowDirtyLines and its descendants to
see if return codes are really warranted here.

In the meantime, I'll attach a patch that uses an automatic helper class to do
most of the space manager juggling.
Attached patch proposed fix (obsolete) — Splinter Review
This adds a class nsSpaceManagerStack that handles most of the space manager
substitution magic.
Keywords: review
Nice Chris - I like the automatic class a lot, it should help cleanup the
management of the spaceManager a lot.

BTW: I have seen pages that cause the ASSERT about the ReflowDirtyLines failing
before - I just cannot remember where yet. I think the assertion is a mistake -
we should handle it (it is pretty bold to assume that something as complicated
as Reflow can never fail).
Comment on attachment 81564 [details] [diff] [review]
proposed fix

hmm. This doesn't look like it really handles the 'stacking' part correctly. If
Push(newSM) is called twice on the same SpaceManagerStack but with different
spaceManager instances, then the first one is leaked. Do you want to protect
against this? I have not traced through it, but from looking at the destructor,
it looks like only mNew is deleted...
Attachment #81564 - Flags: review+
Yeah, I'm just wondering if unwinding out all the way to the top-level via the
dreaded `if (NS_FAILED(rv)) return rv;' paradigm is the right way to propagate
`failure'. If so, then `failure' should be restricted to mean `something has
gone so horribly, terribly, unrecoverably wrong that it's not worth trying to do
anything else, ever. Please, I beg of you, shoot me now.' :-)
Well, this class is by no means general. One nsSpaceManagerStack object is
created per activation of nsBlockFrame::Reflow, which will call Push at most one
time on that object. Maybe `nsSpaceManagerStack' is a poor name for this
class...maybe `nsAutoSpaceManager'?
By the way, will you update the SpaceManager documentation with this new class?
Comment on attachment 81564 [details] [diff] [review]
proposed fix with some suggestions/comments:

- The editor uses Auto in the name for stack-based utility classes like this.

- Like attinasi, when I hear Push() I think stack. Where's the Pop()? ;-P Why
not just call it Init() if it's not really a stack?

- Maybe it's just me, but it seems odd to allocate the space manager directly
in the method and pass responsibility to some other object for destruction. Why
not just have nsAutoSpaceManager.Init() do the allocation for you? That way the
allocation and deletion is managed by the same object?
Attachment #81564 - Flags: superreview+
Sorry to make you re-review. I took your suggestions to heart and cleaned this
up a bit.
Attachment #81564 - Attachment is obsolete: true
kin pointed out that I'd removed the code that clears the block reflow state's
pointer to the space manager. The BRS's dtor restores the translation of an
existing space manager -- not necessary (and dangerous re: order of
destruction) when the space manager is being destroyed.
Attachment #81906 - Attachment is obsolete: true
Attachment #81938 - Flags: superreview+
Comment on attachment 81938 [details] [diff] [review]
one more time, with feeling.
Comment on attachment 81938 [details] [diff] [review]
one more time, with feeling.

I Like it! r=attinasi
Attachment #81938 - Flags: review+
Fix checked in.
Closed: 18 years ago
Resolution: --- → FIXED
This also got rid of couple of "may be used uninitialized" warnings, cool!

- `class nsSpaceManager * spaceManager' might be used uninitialized in this function
- `class nsSpaceManager * oldSpaceManager' might be used uninitialized in this
You need to log in before you can comment on or make changes to this bug.