394709 - Memory leak with object.watch and closure

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kris Zyp]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090205 Minefield/3.0a8pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090205 Minefield/3.0a8pre

When you call Object watch method and assign a function that is not top level, that is an inner function with a non-global parent scope, it will never get GCed, and results in a large memory leak.


Reproducible: Always

Steps to Reproduce:
This leaks about 2MB every time for me:
function runtest () {
	var obj = {};
	function inner() {
	for (var i = 0; i < 50000; i ++) 
		obj[i] = {b:4};
	obj[0].watch('b',inner);
	}
	inner();
}

Actual Results:  
2MB increase in memory usage for firefox.

Expected Results:  
All objects should be garbage collected. The functions does not leave any objects that are reachable, so when it is finished, memory usage should go back down.

Comment 1

[Jesse Ruderman]

Confirmed on Mac, using the command-line js shell 
(http://developer.mozilla.org/en/docs/Introduction_to_the_JavaScript_shell).

A call to gc() shows that most of the garbage was not collected.

Comment 2

[Igor Bukanov]

Attachment: fix v1

The reason for the bug is that the closure contains the strong reference to the watched object. That breaks in a classic way the collection code that removes the watch point only when the object is finalized.

To fix this the patch uses explicit tracing of watch points associated with the object when the object itself is traced. Then the finalization code removes all the watch points with unreachable objects. 

Besides the fixing the problem, it allowed to remove AddRoot call and allows for the cycle collector to record properly the link between the object and the closure.

The drawback of the patch is that the trace point list is scanned now for each live object, not for each finalized object so for the cases with a lot of live objects and watchpoints the performance surfers. But this is just a reflection of watch point implementation that does not scale. I filed bug 394848 about this.

Comment 3

[Igor Bukanov]

Here is a test case using proposed DetectGC object for js shell, see bug 394853 comment 4 for a patch implementing it.

Without the patch:

~> cat ~/m/y.js
var detect_gc_counter = DetectGC();
runtest();
gc();
if (detect_gc_counter === DetectGC())
    throw "A leaky watch point is detected";

function runtest () {
    var obj = { leak_detector: new DetectGC(), n: 0 };
    obj.watch('b', watcher);

    function watcher(id, old, value) {
        ++obj.n;
        return value;
    }
}
~> js ~/m/y.js
before 27696, after 27696, break 092c9000
uncaught exception: A leaky watch point is detected
~>

With the patch:

~> js ~/m/y.js
before 27696, after 27696, break 08135000
~> 

Comment 4

[Igor Bukanov]

Here is a test case using gcthings introduced by the patch from bug 394853 comment 13:

~/m/trunk/mozilla/js/src $ cat ~/m/y.js

runtest();
gc();
var counter = gcthings("object");
runtest();
gc();
if (counter != gcthings("object"))
    throw "A leaky watch point is detected";

function runtest () {
    var obj = { n: 0 };
    obj.watch('b', watcher);

    function watcher(id, old, value) {
        ++obj.n;
        return value;
    }
}
~/m/trunk/mozilla/js/src $ js -f ~/m/y.js
before 27696, after 27696, break 09a0b000
before 27696, after 27696, break 09a0b000
uncaught exception: A leaky watch point is detected
~/m/trunk/mozilla/js/src $ quilt push
Applying patch watch_gc.patch
patching file js/src/jsdbgapi.c
patching file js/src/jsdbgapi.h
patching file js/src/jsgc.c
patching file js/src/jsobj.c

Now at patch watch_gc.patch
~/m/trunk/mozilla/js/src $ js -f ~/m/y.js
before 27696, after 27696, break 0945b000
before 27696, after 27696, break 0945b000
~/m/trunk/mozilla/js/src $ 

Comment 5

[Damon Sicore (:damons)]

Comment on attachment 279571 [details] [diff] [review]
fix v1

This will be approved to land during M8, only if brendan approves in time.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 279571 [details] [diff] [review]
fix v1

Looks good to me.

/be

Comment 7

[Igor Bukanov]

Here is a test case based on the countHeap function available in js shell from CVS head:

runtest();
gc();
var counter = gcthings("object");
runtest();
gc();
if (counter != gcthings("object"))
    throw "A leaky watch point is detected";

function runtest () {
    var obj = { n: 0 };
    obj.watch('b', watcher);

    function watcher(id, old, value) {
        ++obj.n;
        return value;
    }
}

I filed bug 395418 about having countHeap functionality available in jsd so countHeap can be made available in the browser as well.

Comment 8

[Igor Bukanov]

I checked in the patch from comment 2 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1189198857&maxdate=1189198980&who=igor%mir2.org

Comment 9

[Bob Clary [:bc] (inactive)]

I can't reproduce the leak on linux debug with this testcase.

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> I can't reproduce the leak on linux debug with this testcase.
> 

Hm, but do you see the bug with the test case from comment 0 run as:


runtest();
gc();
runtest();
gc();
runtest();
gc();

function runtest () {
        var obj = {};
        function inner() {
        for (var i = 0; i < 50000; i ++) 
                obj[i] = {b:4};
        obj[0].watch('b',inner);
        }
        inner();
}


That should show the leak via increasing heap usage reported by gc().

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #10)

yes. before patch.

js> runtest();
js> gc();
before 1892560, after 1892560, break 09566000
js> runtest();
js> gc();
before 3748192, after 3748192, break 09992000
js> runtest();
js> gc();
before 5613056, after 5613056, break 09d82000

after patch.

js> runtest();
js> gc();
before 1892560, after 27696, break 088ac000
js> runtest();
js> gc();
before 1892560, after 27696, break 0890f000
js> runtest();
js> gc();
before 1892560, after 27696, break 0890f000

Comment 12

[Igor Bukanov]

Sorry for the wrong test case, here is the right one:

runtest();
gc();
var counter = countHeap();
runtest();
gc();
if (counter != countHeap())
    throw "A leaky watch point is detected";

function runtest () {
    var obj = { b: 0 };
    obj.watch('b', watcher);

    function watcher(id, old, value) {
        ++obj.n;
        return value;
    }
}

When given and is not null, the first argument to countHeap specifies to count only GC things reachable from the argument, not all live GC things in the heap. It is the second argument that restricts the kind of things to count. Thus simple countHeap() should be used for the test. countHeap("anystring") always gives 1 as the tree of things reachable from "anystring" contains exactly one node, which is the string itself.

Here is a session that checks out cvs.mozilla.org at 2007-09-07 20:00UTC, the time when countHeap has been already available on the trunk but the fix were not yet committed:

~> cd /tmp
/tmp> rm -rf mozilla
/tmp> cvs -d [e-mail]:/cvsroot co -D "2007-09-07 20:00UTC" mozilla/js/src > /dev/null 2>&1 
/tmp> cd mozilla/js/src/
/tmp/mozilla/js/src
/tmp/mozilla/js/src> make -f Makefile.ref > /dev/null 2>&1
/tmp/mozilla/js/src> cat ~/m/y.js
runtest();
gc();
var counter = countHeap();
runtest();
gc();
if (counter != countHeap())
    throw "A leaky watch point is detected";

function runtest () {
    var obj = { b: 0 };
    obj.watch('b', watcher);

    function watcher(id, old, value) {
        ++obj.n;
        return value;
    }
}

/tmp/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/y.js
before 27696, after 27696, break 0891f000
before 27696, after 27696, break 0891f000
uncaught exception: A leaky watch point is detected
/tmp/mozilla/js/src> cvs upd -D "2007-09-07 22:00UTC"
P jsdbgapi.c
P jsdbgapi.h
P jsgc.c
P jsobj.c
/tmp/mozilla/js/src> make -f Makefile.ref clean > /dev/null 2>&1
/tmp/mozilla/js/src> make -f Makefile.ref > /dev/null 2>&1
/tmp/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/m/y.js
before 27696, after 27696, break 0a015000
before 27696, after 27696, break 0a015000
/tmp/mozilla/js/src> 

Comment 13

[Igor Bukanov]

Regarding countHeap() usage: I suggest to use it exactly as in the test and not like in:

gc();
expected = countHeap();
....
actual = countHeap();

compare(expected, actual);


The reason for that is to avoid extra live GC things that compare may creates during reporting.

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> Regarding countHeap() usage: I suggest to use it exactly as in the test and not
> like in:
> 
> gc();
> expected = countHeap();
> ....
> actual = countHeap();
> 
> compare(expected, actual);
> 
> 
> The reason for that is to avoid extra live GC things that compare may creates
> during reporting.
 
Ignore this comment, of cause the pattern 
gc();
expected = countHeap();
....
actual = countHeap();

would work as the counting happens during the call, not in the compare.

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-394709.js;
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-394709.js,v  <--  regress-394709.js
initial revision: 1.1

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows