Closed Bug 394709 Opened 17 years ago Closed 17 years ago

Memory leak with object.watch and closure

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED

People

(Reporter: kriszyp, Assigned: igor)

References

Details

(Keywords: memory-leak, testcase)

Attachments

(1 file)

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.
Assignee: nobody → general
Component: General → JavaScript Engine
Keywords: mlk, testcase
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Summary: Memory Leak when using Object:watch → Memory leak with object.watch and closure
Assignee: general → igor
Attached patch fix v1Splinter Review
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.
Attachment #279571 - Flags: review?(brendan)
Attachment #279571 - Flags: approval1.9?
Depends on: 394853
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
~> 

Status: NEW → ASSIGNED
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 on attachment 279571 [details] [diff] [review]
fix v1

This will be approved to land during M8, only if brendan approves in time.
Comment on attachment 279571 [details] [diff] [review]
fix v1

Looks good to me.

/be
Attachment #279571 - Flags: review?(brendan)
Attachment #279571 - Flags: review+
Attachment #279571 - Flags: approval1.9?
Attachment #279571 - Flags: approval1.9+
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.
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I can't reproduce the leak on linux debug with this testcase.
(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().
(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


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 igor%mir2.org@cvs.mozilla.org:/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> 
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.
(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.
Checking in regress-394709.js;
/cvsroot/mozilla/js/tests/js1_8/extensions/regress-394709.js,v  <--  regress-394709.js
initial revision: 1.1
Flags: in-testsuite+
verified fixed 1.9.0 linux/mac*/windows
Status: RESOLVED → VERIFIED
Depends on: 396936
No longer depends on: 396936
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: