bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Memory leak with object.watch and closure

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
major
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Kris Zyp, Assigned: Igor Bukanov)

Tracking

({memory-leak, testcase})

Trunk
memory-leak, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Assignee: nobody → general
Component: General → JavaScript Engine
Keywords: mlk, testcase
Product: Firefox → Core
QA Contact: general → general

Updated

11 years ago
Version: unspecified → Trunk

Comment 1

11 years ago
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)

Updated

11 years ago
Assignee: general → igor
(Assignee)

Comment 2

11 years ago
Created attachment 279571 [details] [diff] [review]
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.
Attachment #279571 - Flags: review?(brendan)
Attachment #279571 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Depends on: 394853
(Assignee)

Comment 3

11 years ago
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
(Assignee)

Comment 4

11 years ago
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+
(Assignee)

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 9

11 years ago
I can't reproduce the leak on linux debug with this testcase.
(Assignee)

Comment 10

11 years ago
(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

11 years ago
(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


(Assignee)

Comment 12

11 years ago
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> 
(Assignee)

Comment 13

11 years ago
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.
(Assignee)

Comment 14

11 years ago
(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

11 years ago
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+

Comment 16

11 years ago
verified fixed 1.9.0 linux/mac*/windows
Status: RESOLVED → VERIFIED

Updated

11 years ago
Depends on: 396936

Updated

11 years ago
No longer depends on: 396936
You need to log in before you can comment on or make changes to this bug.