Closed Bug 319980 Opened 19 years ago Closed 18 years ago

javascript garbage collector not run when supposed to, leading to "memory leak"

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: diablonhn, Assigned: brendan)

Details

(4 keywords)

Attachments

(3 files, 2 obsolete files)

testcase from comment 0
404 bytes, text/html
Details
Modified test case
3.12 KB, text/html
Details
oops, the right patch this time
5.74 KB, patch
mrbkap
: review+
bzbarsky
: superreview+
dveditz
: approval-branch-1.8.1+
dveditz
: approval1.8.0.4+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

when creating a new array, setting it to a global variable, and populating the array with literal in a loop, the garbage collector will not collect this array, this results in the browser eating all available system memory

on the other hand the array will be collected if it was populated with objects created with "new"

IE6 does not have this problem

this happens everytime WHEN:

1. new tabs/windows are not created
2. old tabs/windows are not refreshed/closed

Reproducible: Always

Steps to Reproduce:
1. call function to create array, set to a global variable, populate array
2. repeat function every x seconds
3. meanwhile, not creating/closing/refreshing tabs or windows

<html><head><script type="text/javascript">

var a;
function update()
{
	a = new Array(100000);
	for( var i = 0; i < 100000; i++ )
	{
		a[i] = i;	//a[i] = new Number(i) has no problem
	}
	//delete a;	//does not help
	//a = null;	//does not help
}
setInterval( "update()", 1000 );

</script></head><body></body></html>


Actual Results:  
FUBARed system
Keywords: mlk, testcase
Version: unspecified → Trunk
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
I don't have a trunk build at the moment I can test with but I don't see the memory growth or fubarredness in Firefox 1.5/Winxp. Is this trunk only?
This probably isn't a JS Engine bug.

/be
shows itself in 1.5 also, on all WinXP SP2 machines tested
Attached file Modified test case — 
Test case in Comment #1 is slightly modified for ease of test.
 (A) setInterval is changed to setTimeout
 (B) "Start loop" button and "Stop loop" button are added.
 (C) Case-2('a[i]=new Number()' just before 'a[i]=i') is added

Observation result of Task Manager display.
(Seamonkey 2005120309-trunk/Win-2K, with new profile).
[Case-1] (Same as original test case in comment 1) 
  (1-1)Virtual stotage size increases infinitely while update loop.
       ==> Problem is re-creatable with Seamonkey nightly.
  (1-2)Virtual stotage size doesn't decrease when update loop is stopped.
  (1-3)Virtual stotage size DOES decrease when "New Tab" is opened.
       This is true even when Tab is opened during update loop execution.
[Case-2]
  (2-1)Virtual stotage size increases but decreases while loop execution,
       as comment 0 says.
Additonal result.
 (1-4) Virtual storage size decreases when tab(where loop is executed) is closed.
Blocks: mlk1.8
Confirming based on my comment #5.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Presumably additional calls to JS_MaybeGC are needed somewhere.
While this is a memory related bug, it's not the type of large real-world leak we need to address in the 1.8 leak campaign.
Assignee: general → general
No longer blocks: mlk1.8
Component: JavaScript Engine → DOM
QA Contact: general → ian
So in my humble opinion this is totally a JS engine bug.  Here's what's going on:

1)  We don't call JS_GC while executing this testcase, as expected.
2)  We make tons of calls to JS_MaybeGC from the branch callback.

When we call JS_MaybeGC, it looks at rt->gcBytes and rt->gcLastBytes and only actually does GC if it has:

        if ((bytes > 8192 && bytes > lastBytes + lastBytes / 2) ||
            rt->gcMallocBytes > rt->gcMaxBytes) {

The typical numbers I'm seeing for these valus are:

(gdb) p bytes
$33 = 110196
(gdb) p lastBytes
$34 = 105831
(gdb) p rt->gcMallocBytes
$35 = 43197
(gdb) p  rt->gcMaxBytes
$36 = 4294967295

so we never actually GC.

The reason rt->gcBytes is so low, as far as I can tell, is that only GCThings are counted in that number.  In this case, the only GCThings we have around are array objects, so rt->gcBytes increases by 8 bytes or so for every Array we allocate.  All the memory use in this testcase comes from property storage, and I don't see ChangeScope poking rt->gcBytes or even rt->gcMallocBytes (since it uses stdlib's calloc directly to allocate its table).
Assignee: general → general
Component: DOM → JavaScript Engine
OS: Windows XP → All
QA Contact: ian → general
Hardware: PC → All
Attached patch Proposed patch (obsolete) — — Splinter Review 
Not like I know much about this code... but this fixes the bug for me!
Attachment #213711 - Flags: review?(brendan)
Attached patch counter-proposal (obsolete) — — Splinter Review 
The fix for bug 317865 was missing a crucial change to JS_MaybeGC.  The only calloc in jsscope.c that needed accounting in rt->gcMallocBytes was the one in CreateScopeTable.  Fixing that required fussing a bit to conserve a non-fatal OOM handling case.

/be
Assignee: general → brendan
Attachment #213711 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #213716 - Flags: superreview?
Attachment #213716 - Flags: review?(mrbkap)
Attachment #213711 - Flags: review?(brendan)
This seems worth trying to get into 1.8.0.2.

/be
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
Attachment #213716 - Flags: superreview? → superreview?(bzbarsky)
To say a bit more, the idea behind gcBytes is that GC-heap allocation sizes proxy for total entrained memory, chaotically but usably.  But this was never as good as hoped, so gcMallocBytes was added.  Then JS_malloc turnover also helped pace self-triggered GCs.  But that too didn't proxy nearly linearly for all entrained memory.  If we need to racily bump rt->gcMallocBytes for other naked calloc or malloc calls, file new bugs.

/be

        Attachment #213716 -
        Attachment is obsolete: true

        Attachment #213717 -
        Flags: superreview?(bzbarsky)

        Attachment #213717 -
        Flags: review?(mrbkap)

        Attachment #213716 -
        Flags: superreview?(bzbarsky)

        Attachment #213716 -
        Flags: review?(mrbkap)

    
    

    
  
(In reply to comment #3)
> This probably isn't a JS Engine bug.

Watch me try to blame the DOM ;-).  Thanks, bz!

/be

    
    

    
  
Comment on attachment 213717 [details] [diff] [review]
oops, the right patch this time

Yeah, looks good.  sr=bzbarsky

        Attachment #213717 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
Comment on attachment 213717 [details] [diff] [review]
oops, the right patch this time

r=mrbkap

        Attachment #213717 -
        Flags: review?(mrbkap) → review+

    
    

    
  
How large is the leak?  Can we get some guidance about how critical this is?  

If you are happy with the patch please land on the trunk and 1.8 branch and look for regressions.

    
    

    
  
> How large is the leak?

Depends on the situation.  The testcase here only runs the function once a second; it was growing about 5 megs every 3-4 seconds over here.  I killed the process the first time after it had run for about 10 minutes (and gotten to 3 gigabytes of ram usage).

So in a situation that's _trying_ to produce this leak (eg running every 10ms, not every 1000ms), it's huge.  ;)

In real life, I dunno.  Keep in mind that the "leak" is only until the next time a tab or window is closed, so it's only a problem when interacting with a single page for a long time (eg sites that use AJAX).

    
    

    
  
timr: the bug's subject puts "memory leak" in quotes because the leak is not permanent -- not a true leak in the sense commonly used.  We call this kind of bug "bloat" rather than "leak".  It's as bad as the script author can make it, and Murphy was an optimist.

Practically speaking, this bug is an interoperation hazard with respect to IE and probably other browsers, so a competitive ding.

Landed on trunk and 1.8 branch.

/be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

    
    

    
  
It is too late for a non-critical fix in 1.8.0.2, the workaround is open and close a tab -> 1.8.0.3
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2-
Flags: blocking1.8.0.3? → blocking1.8.0.3+

        Attachment #213717 -
        Flags: approval1.8.0.3?

    
    

    
  
Comment on attachment 213717 [details] [diff] [review]
oops, the right patch this time

approved for 1.8.0 branch, a=dveditz for drivers

        Attachment #213717 -
        Flags: approval1.8.0.3?

        Attachment #213717 -
        Flags: approval1.8.0.3+

        Attachment #213717 -
        Flags: approval-branch-1.8.1+

    
    

    
  
Fixed on the 1.8.0 branch.

/be
Keywords: fixed1.8.0.3

    
    

    
  
verified fixed by running test cases in 1.5.0.2 (which showed the bloat) and 1.8.0.4, 1.8, 1.9 (which did not) on windows xp.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.4, 
          
            fixed1.8.1 → verified1.8.0.4, 
          
            verified1.8.1

    
    

    
  
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-319980-01.js,v
done
Checking in regress-319980-01.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-319980-01.js,v  <--  regress-319980-01.js
initial revision: 1.1
done

Flags: in-testsuite+

    
  
Keywords: fixed1.8.1

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: