137094 - Memory leak in nsBlockFrame::Reflow()

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Peter A Jonsson]

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

Code:
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.

Comment 1

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

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?

Comment 2

[Peter A Jonsson]

Try http://www.geodesic.com/monitor 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?

Comment 3

[Peter A Jonsson]

Bug 135146 was checked in 02/04/11 - the patch there touches Reflow(). Could
that have anything to do with it?

Comment 4

[Chris Waterson]

I'll take a look at the flow-of-control here.

Comment 5

[Peter A Jonsson]

Attachment: 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.

Comment 6

[Chris Waterson]

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.

Comment 7

[Chris Waterson]

Attachment: proposed fix

This adds a class nsSpaceManagerStack that handles most of the space manager
substitution magic.

Comment 8

[Marc Attinasi]

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 9

[Marc Attinasi]

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...

Comment 10

[Chris Waterson]

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.' :-)

Comment 11

[Chris Waterson]

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'?

Comment 12

[Håkan Waara]

By the way, will you update the SpaceManager documentation with this new class?

Comment 13

[kinmoz]

Comment on attachment 81564 [details] [diff] [review]
proposed fix

sr=[e-mail] 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?

Comment 14

[Chris Waterson]

Attachment: patch with kin and marc's suggestions.

Sorry to make you re-review. I took your suggestions to heart and cleaned this
up a bit.

Comment 15

[Chris Waterson]

Attachment: one more time, with feeling.

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.

Comment 16

[kinmoz]

Comment on attachment 81938 [details] [diff] [review]
one more time, with feeling.

sr=[e-mail]

Comment 17

[Marc Attinasi]

Comment on attachment 81938 [details] [diff] [review]
one more time, with feeling.

I Like it! r=attinasi

Comment 18

[Chris Waterson]

Fix checked in.

Comment 19

[Aleksey Nogin]

This also got rid of couple of "may be used uninitialized" warnings, cool!

-layout/html/base/src/nsBlockFrame.cpp:640
- `class nsSpaceManager * spaceManager' might be used uninitialized in this function
-
-layout/html/base/src/nsBlockFrame.cpp:641
- `class nsSpaceManager * oldSpaceManager' might be used uninitialized in this
function
-