Closed Bug 84136 Opened 23 years ago Closed 23 years ago

Closing windows does not free all memory they have allocated

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.2

People

(Reporter: ezh, Assigned: harishd)

References

Details

(4 keywords, Whiteboard: pdt+)

Attachments

(7 files)

Bloat logs for ~25 window open/closes (all blank)
73.68 KB, text/plain
Details
Bloat logs for things we lost per-window (26 windows)
4.48 KB, text/plain
Details
gdb log for assertions
27.69 KB, text/plain
Details
Break strong ref to parser from XML document
4.37 KB, patch
Details | Diff | Splinter Review
Purify run [ Used CTRL-N on the browser window ]
27.72 KB, text/plain
Details
Patch v1.2 [ A getting into document-parser-sink circularity by delaying StartDocumentLoad ] ]
3.34 KB, patch
Details | Diff | Splinter Review
nsXBLService::FetchBindingDocument)
4.90 KB, text/plain
Details 
OK I started the thred "mem usage" in mozilla's performance newsgroup and we had
such a test:

Blank page:                            17 484K
Go to bluesnews.com:                   21 492K
Open the news archive in a new window: 24 356K    +2 864K
close the news archive window:         23 924K    -  432K
open a random link in a new window:    24 572K    +  648K
close that window:                     24 512K    -   60K
open random link in new window:        25 364K    +  852K
close that window:                     25 180K    -  184K

It appears, with every new window, to be adding ram on, and when you
close that window, it frees up a small percentage of the ram used.  I
can understand it eating up a little ram the first few times, as it's
recording the recent history and after a while it should taper off, but
those numbers seem rather large.

Tested at 2001053004
Changing OS to All since I see this on win98 and the test was done on w2k.
OS: Windows 98 → All
Summary: closeing window does not freeing all memory they have allocated → closeing windows does not freeing all memory they have allocated
Added perf, upped severity.
Who should be handling this bug?  Adding waterson to cc.
Severity: normal → major
Keywords: perf
Adding footprint keyword since this is a memory use issue.
Keywords: footprint
This could be any number of things, but is most likely heap fragmentation due 
to the allocator. If someone can demonstrate that there is really a leak here, 
then we should change the bug to mean ``fix that leak''. Otherwise, I'm not 
really sure what we can do about this..
This test should be redone with the Lea allocator (as per my messages in
performance).  600-800K of fragmentation loss per window opened is rather
large....

I'll try to do a lea build and post the results here later today.
If you're going to compile with lea, be careful of bug 73053 and bug 72497. 
Perhaps we should add a dependency, though I'm not certain that Lea is the only
issue here (probably not, in fact).
If the original test was done with Win2K then using lea is not really a 
solution for that platform (since we can only use lea on *nix right?). Though 
that would be no reason _not_ to use lea where avalible of corse.

Don't we have tools for checking if this really is a fragmentation loss only or 
if we infact hold more memory allocated?
We certainly have tools to determine if it's a leak. Get a debug build, set 
XPCOM_MEM_BLOAT_LOG=<some-file>, run, and look at the contents of <some-file>.
I run mozilla on a PII 266 with 64 MB and this is a major problem for me
Everything is fine when I start using mozilla but after 1 or 2 hrs of intense
browsing (with several windows opened) mozilla takes 60-70 MB and the machine 
slows down to the point where I have to quit mozilla... and return to IE (no 
prb with it)

just a thought:

On NT (I think even on Win9x) there are heaps (functions like HeapCreate() 
HeapAlloc() HeapDestroy() and HeapCompact() to give the memory back to the OS)

I searched through LXR and I did not find any use of those functions

It's strange because it is one of the nice NT feature and one of the key to 
have good multithread/multiprocessor performance because there's less 
contention with several heaps.

It would be nice if all memory specific to one window could be allocated in 
such a heap. That way the heap could be destroyed when the window is closed 
leading to less fragmentation (if no leak, but I can't belive there can be so 
much memory leaked :-)) in the 'main' heap used by malloc()

Always looking at the code I see many references to 'arenas' (don't know what 
it is)
I don't know how they relate to 'heaps'

reseting os. Please don't change from Windows X to All unless you can reproduce 
on something that isn't Windows.
OS: All → Windows 98
resetting OS to all.  Just ran a test (FreeBSD 4.1  Mozilla 20010605xx debug
build).  Start, go to mozilla.org.  Open new window (blank), click on
mozilla.org, close.  Repeat a dozen times.  Average memory loss is ~1.5-2MB per
window open, ~300K per click on mozilla.org (measured by watching Size in top). 
Memory usage never goes down.  Note: this is the system (BSD) allocator, that
tends to allocate a lot of pages and not give them back.  However, BSD usually
stabilizes and stops using more.  This has not stabilized, both RES and Size
increase for as long as I tried it.
OS: Windows 98 → All
BTW, Chris, this does NOT seem to just be heap fragmentation, or the lossage
would decline, not stay at a fairly steady amount over a loss of 50MB.
I just redid my test without browsing to mozilla.org (just opening/closing blank
windows).  Same loss as best I can tell, or very close.  Lost 50MB without any
slowdown (~1.4-1.5MB per open/close), and RES tracked the increase in Size (this
is a 512MB machine).  
Bloat logs show lots of XUL Elements/attributes lost, and 2.6 million nsStr's.

The amount reported lost (13MB) is WAY lower than the increase in size seen
(~40MB that run).
Things like nsStr and nsTextNode point to text buffers that aren't counted in
the bloat logging (which is a rather blunt tool and easy to misinterpret).

I've been seeing similar content leaks.
Keywords: mlk
The leaked GlobalWindowImpl and nsDocument look more like roots than anything else.

I wonder if the patch on bug 81289 helps at all.  (It fixes at least one
GlobalWindowImpl leak, but I don't *think* it could fix a GlobalWindowImpl +
nsDocument leak.)
_Nice_! Ok, so can we figure out how to make _one_ nsDocument leak? (These leaks
clearly don't appear on tinderbox, so they must be the result of some user
interaction.) For example,

- Do we leak when closing a single window using ``File->Exit''? If not...

- Do we leak when opening a second window, then using ``file->exit'' to
  shut down the whole app? If not...

- Do we leak when opening a second window, then closing it using
  ``file->close'', then shutting down the app?

etc. This is the game that dbaron and others are ever so familiar with. (At one
point, just _mousing over_ the URL bar would cause the entire window to leak.)
To the extent that we can minimize the interaction required to kill the leak, we
win...

thanks for your help, Randell!
I've applied dbaron's patch and am rebuilding.  Once it's done, I'll try.

In particular, when sequence I used before was: Open mozilla (blank).  ^n (new
window, blank).  Mouse to close box, click.  Note: sidebar was open, not
displaying anything interesting.

NOTE: I was getting these every so often when opening/closing:

###!!! ASSERTION: root element not in document: 'doc != nsnull', file
nsXULContentBuilder.cpp, line 1523

I do not know if this was directly related to leaks; I got rather more of these
than i opened windows which was why I didn't jump on it.  AhA!  looking at it
more closely, on each open-window, I got one of those assertions for each window
I had perviously opened! Hmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmmm

Also, as I opened each window I got this:

nsWidget::~nsWidget() of toplevel: 278 widgets still exist.
WEBSHELL+ = 98
WEBSHELL+ = 99
Enabling Quirk StyleSheet
WEBSHELL+ = 100
WEBSHELL+ = 101
Enabling Quirk StyleSheet
Enabling Quirk StyleSheet

With the webshell numbers going up and up...
Hrm.  I haven't seen leaked webshells for a long time (at least not when paying
attention).
I do have a few mods in my tree to make nsIViewManager *'s into
nsCOMPtr<nsIViewManager>'s, and test for null viewmanagers.  I should also note
I'm trying to track a problem with View's being freed and reallocated (80203). 
I can provide a cvs diff of any modified files if anyone thinks my mods could be
the cause.

BTW, dbaron's patch didn't seem to help.  I'm backtracing the assertion right
now.
Please do provide a diff of your modified files, although that doesn't *seem*
like it should be the problem since the view manager only has weak raw pointer
back to its observer.
BTW, the asserts are due to (deep within) this:
nsWindowMediator::UnregisterWindow()

I'll attach a GDB session with details.

I'll also grab a diff

I thought we got rid of the parser / content sink / document circular references
long ago, but apparently we haven't.

When I start mozilla, open a new window with ^N, and close the two windows with
the X in the corner, I see 3 nsDocument leaked.  The first two are XBL-related,
and the third is the real one.

This nsDocument (#24 to me) has one outstanding reference, from an nsXMLContentSink.

The nsXMLContentSink has two outstanding references, one from an nsParser and
one from an nsScriptLoader.

The nsParser has two outstanding references, one from an nsXMLDocument and one
from an nsXMLContentSink.

So the ownership model that's leaking looks something like this:

                  <--------------
   --> nsParser   -------------->  nsXMLContentSink   <----- nsScriptLoader
  /                                /
  |                               /
   \                             /
     -------------- nsDocument <-
                          |
                          V
                 everything else (I assume)

I'm guessing this is all because some member function of the parser isn't called.
Assignee: asa → harishd
Component: Browser-General → Parser
QA Contact: doronr → bsharma
Thank you dbaron!  Adding moz 0.9.2 to keywords; hopefully this is
straightforward to fix - it's a massive mem leak.
Keywords: mozilla0.9.2
FYI: While using the "multidesk" utility for windows (from techsuperior.com) I
can get already closed windows to come back as dead frames when I switch the
desktops.
Summary: closeing windows does not freeing all memory they have allocated → Closing windows does not freeing all memory they have allocated
(if you're going to be picky about grammar... :) )
Summary: Closing windows does not freeing all memory they have allocated → Closing windows does not free all memory they have allocated
here is my take with mozilla 2001052404 on windows 2000 (512Mb ram)

This is from task manager

Preperation:
* Open browser.
* set home page to blank page
* clear disk and memory cache
* close mozilla

test run
Task                            Mem usage (kb)      VM size (kb)
Open Mozilla                    17.516              11.356
Minimize Mozilla                1.084               11.436
Restore Mozilla                 6.188               1176    	(why so little)
www.slashdot.org                15.340              13.828	(less than after startup)
open banner in nw (linux.com)   19.740              17.872
Open Mail                       25.192              22.312	
Close mail                      24.992              22.100
Close linux.com                 24.232              21.344
ctrl-n & www.theserverside.com  24.908              21.948
Minimize www.theserverside.com  7.512               21.960
minimize www.slashdot.org       3.976               21.960
restore www.slashdot.org        11.108              21.960
restore www.theserverside.com   13.200              21.960
Open mail again                 22.104              23.304
minimize mail                  11.312	            23.316
minimize all	               3.072                24.316


A few interesting things in this memory usage

1. Mozilla seems to free the memory when minimizing windows.

2. Start-up, minimize and restore result in a LOWER memory usage than just start-up

3. The VM size does not decrease when mozilla frees memory. dunno if that is a
windows thing.
FWIW, Win32 moves most of an application's pages to the standby list when the
application windows are minimized. This reduces the resident set size as
reported by the task manager (N.B., the pages aren't actually flushed to disk
until they get evicted from the standby list). So, this is not something magic
that mozilla is doing.

But, this really isn't all that important, because dbaron has already isolated
what the problem is.
From the cvs log of nsXULDocument.cpp:

revision 1.49
date: 1999/04/06 07:48:21;  author: waterson%netscape.com;  state: Exp;  lines:
+7 -9
Fixed a _big_ memory leak: circular reference between the document, content
sink, and parser. Now the document releases the parser immediately after telling
it to start parsing.


I guess the more recent stuff was with HTML.

It would make sense to me if the content sink didn't have an owning pointer to
the document.  This would also reduce the risk of leaking a document from leaks
in parser or network code.  It would mean that one would have to stop the
content sink if the document was going away, but that would seem like a
reasonable thing to do anyway (and it should have happened earlier anyway). 
Although maybe breaking the cycle some other way would be easier...
Harish is on vacation till Thursday.  Heikki has agreed to take a look at this.  
Thanks, Heikki!
The patch I just attached removes the strong reference to the parser in the XML
document. Currently there just isn't any need for it. The approach is almost
100% copy of the way XUL document does it.

This approach will not work in HTML (at least not as simply) since the HTML
document is actually using the parser for various things. I am now looking at
how to break the reference to the document from the content sink.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.2
Any further action on HTTP?  Can someone r/sr/a the partial (XML) patch that
Heikki attached?  This one is a 0.9.2 target, and a massive leak.
Heikki's approach is correct but it doesn't seem to fix the leak. I've started
working on it.
Marking pdt+ in the status whiteboard.  This means that this has been approved 
by the PDT as a rtm stopper.
Whiteboard: pdt+
FYI: I see the leak only when I use ctrl-n to open up a new browser window.
I use CTRL+Click on a link to open the link in new window.
FYI: The purify run was done without heikki's patch. 
The root of the problem, I think, seems to be in 
nsXBLService::FetchBindingDocument().  CCing hyatt.
The above comment should read:

" Avoid getting into document-parser-sink circularity by delaying 
StartDocumentLoad "

    
    

    
  
sr=hyatt


    
    

    
  
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)
Blocks: 83989

    
    

    
  
Fix is in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

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

    
    

    
  
In which build can I see the improvement? with 2001062004 is no difference to
2001061704.

Actually at the startup with rus.delfi.ee as a homepage it taked 2.5Mb mem more
(total 23.5) than the previous biuld I used...

Can anyone send a log for builds with this fix to compare with previous one?

    
    

    
  
The leak only occurred when opening/closing windows (watch SIZE (VM size) while
opening (with ^N) and closing (with close X in corner) windows.  We'd leak about
1.4MB per open/close.

    
    

    
  
I'm talking about it, actually...

I open moz with rus.delfi.ee as my homepage (~23.5Mb)

Open 5 new windows with CTRL-N (~42Mb)
Close 5 windows (~30Mb).

So this was about the same without this patch...

build 2001062204

    
    

    
  
Eugene: The patch does fix the document-parser-sink leak. I'm not sure why you
don't see any difference.

    
    

    
  
Well, you opened/closed 5 windows, and your memory usage went from 23.5MB to
30MB.  That sounds like about 1.5MB/window opened/closed, but 5 windows isn't
enough to be sure either way using tools like ps and top.

Note that measuring mozilla memory usage can be tricky, and the OS can affect
things as well.  I opened ~26 new windows (all displaying about:blank) and
closed them (one at a time), and lost about 50MB of memory.

The leak was very much real.


    
    

    
  
Can you provide mem usage with ru.delfi.ee as a homepage, please?

    
    

    
  
Sorry, I may not be able to - I leave for vacation in a few hours and am pretty
busy in the meantime.  Perhaps someone else can?

    
    

    
  
A did some tests.

Hardware: Duron 950, 384Mb RAM, IDE RAID-0 with 2*30Gb IBM DTLA HDD

Software: Win98 SE, Mozilla 2001062204, Opera 5.11, TaskInfo 2000 3.0.6.11 (beta)
(tried to test NN 4.77, but every time it crashed itself or crashed Windows :))

How I tested:

1. Opened the program with rus.delfi.ee as a homepage
2. Looked how much memory it used (inMem Kb)
3. Pressed CTRL-N 25 times (so total 26 windows were opened)
4. Looked at CPU usage
5. Looked how much time taked untill the last window showed "Done." (with wallclock)
6. Looked mem usage
7. Closed 25 windows with "X"
8. Tried to measure how much time it tooked
9. Looked how much mem is used after all is complete.

Results:

Mozilla: 

2. 21200 Kb
4. ~95% CPU were used (!!!)
5. 3 min 16 sec (!!!)
6. 115328 Kb (!!!)
8. 12 sec (!!!)
9. 38787 Kb 

Opera:

NB! I was not able to get any times since Opera does not load the homepage 
in new window (the gloab and window homepage was set to rus.delfi.ee). 

So I opened 25 new blank windows (they appeared/closed without any delay as soon 
as I pressed CTRL-N or "X") and pressed for eachone CTRL-Space (loaded the
homepage).

So I've skipping p. 4, 5, 8

2. 6256 Kb
6. 20512 Kb
9. 8988 Kb

Comments?

    
    

    
  
The times and amount of memory used can be laid at the foot of XUL partially,
and the rest at overall structural issues in Mozilla.  Something like this would
make an interesting quantify case to check.

The fact that after opening/closing 25 windows memory usage was 38MB (down from
over 110MB) means that the major leak is definitely gone.  (In the past, closing
windows brought almost no memory back.)


    
    

    
  
I found another (major?) leak involving opening and closing new windows:  bug
87466.  Fixing this reduces the amounts of memory that we *never* free to
acceptably low numbers (even for me).

However, even with that patch, if I start mozilla (debug), I see 42MB memory
usage, and if I open 6 more windows I see memory usage go up to 52MB, and it
doesn't come back down when I close those Windows.  However, according to our
leak detection tools we are freeing the memory before we exit.  So what I'm
seeing is probably the hardest type of leak to diagnose -- one where we free
memory later than we should have, but do free it.  (It's hard for a leak
detection tool to know when something *should* have been freed and detect that
it was freed later than it should have been.  Many objects are supposed to be
very short-lived and some are supposed to exist for the entire lifetime of the
program.)  Anyway, I'll look into it a bit more...

    
    

    
  
I filed bug 87472 on the general issue I mentioned in my previous comment (which
is I guess what this bug was originally filed for).

    
    

    
  
Never mind -- see why I marked bug 87472 invalid.

    
    

    
  
Verified on:
build: 2001-07-02-04-Trunk
platform: WinNT

Marking verified as per above developer comments.
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: