#233 Memory leak in DynElement.deleteChild()

[Andrew Gillett]

The DynElement.deleteChild() function is implemented
thus:

p.deleteChild = function(c) {
c.removeFromParent();
c._delete();
};

Although the _delete() function is documented in
quickref.dynelement.html, it is not implemented
_anywhere_ in the DynAPI source code, except for
being set to dynapi.functions.Null in event.js.

If you have a page that repeatedly adds and removes
DynElements, it will leak memory. If you replace the call
to _delete() with a call to _destroy(), the memory leak
goes away:

p.deleteChild = function(c) {
c.removeFromParent();
c._destroy();
};

[Raymond Irving]

Logged In: YES
user_id=696242

Are you using the lastest snapshot?

What browsers are experiencing this memory leak?

You can get the lastest snapshot here:
http://dynapi.sourceforge.net/snapshot/?N=D

[Andrew Gillett]

Memory leak example

[Andrew Gillett]

Logged In: YES
user_id=134108

> Are you using the lastest snapshot?
Yes, I check out the code directly from CVS.
> What browsers are experiencing this memory leak?
IE6 & Mozilla1.4, but that is not really relevant.

On 29/07/2003 12:26 AM (my time), in response to my post
about a bug in deleteAllChildren(), you wrote:
>To fix the memory and layer problem use the following:
>
>p.deleteAllChildren = function() {
> var c,l = this.children.length;
> for(var i=0;i<l;i++) {
> c=this.children[i];
> if(c) {
> c._destroy();
> c._created = c.isChild = false;
> };
> delete this.children[i];
> }
> this.children.length = 0;
>};
>
This fix worked perfectly. The _destroy() method is called on
each child of the layer.

However this is not what was put into CVS. In the current
version of event.js, deleteAllChildren() calls deleteFromParent
() which calls deleteChild() which calls _delete() instead of
_destroy().

It is the call to _delete() that I have a problem with. There
is simply no such function. In line 184 of event.js there is:

... = p._delete = p._destroy = dynapi.functions.Null;

but nowhere else in the entire source tree is _delete()
defined.

Try the attached example. On my Windows XP system, IE
grows by about 6MB each time the "Memory Leak" button is
pressed. But if I replace the call to _delete() with a call to
_destroy() in the deleteChild function, then IE doesn't leak at
all.

[Raymond Irving]

Logged In: YES
user_id=696242

I see what's happening. IMO deleteChild() and removeChild
means the same thing, correct?

When a child layer is removed from the parent it's actually
deleted from the parent, correct? The layer is not completly
destroyed as a reference to it still exists within DynObject.all
collection.

Calling _destroy() during a deleteChild() function will not only
delete the child from its parent but it will also dispose of the
child object.

I think we should change deleteChild/AllChildren/etc to
disposeChild/AllChildren/etc wihich will clearly define what is
been done to the object. The dispose functions will complete
remove and destroy the object.

Are we all in agreement with changing delete to dispose? Or
should we add the _destroy() function to deleteChild and
keep the delete functions?

(Note: If we keep the delete functions we'll have to
document that it will completely destroy and remove the layer
from the system.)

Logged In: YES
user_id=706287

My view on the matter.

I think the naming convention would be clearer if the
function that removes children from parents was called
removeChild / removeFromParent, but not destroying the
layer. I think this is how it works now. I think the
function that removes the child from the parent AND destroys
the layer should be called deleteChild / deleteFromParent or
maybe destroyChild / destroyFromParent. I think it's
confusing to add a new name "dispose" when it doesn't
intuitively correspond to an underlying "dispose" function.
It's a little vague.

[Raymond Irving]

Logged In: YES
user_id=696242

Thanks for the feedback

Ok. Will keep the delete functions them _destroy() the layers
as well.

[Andrew Gillett]

Logged In: YES
user_id=134108

Hmm, we could have: remove, delete, destroy and dispose.

> I see what's happening. IMO deleteChild() and removeChild
> means the same thing, correct?
No. To my way of thinking, 'delete' should be like the C++
keyword - once deleted an object no longer exists. And why
have both deleteChild and removeChild if they do the same
thing?

IMO we should just add the _destroy() function to deleteChild
and update the documentation accordingly.

[Raymond Irving]

Logged In: YES
user_id=696242

I believe the lastest snapshot should have fixed this problem.
I did add _destroy() to the deleteChild() function and made
some other changes that fixed the problem.

Keep up the good work.

[Doug Melvin]

Logged In: YES
user_id=184788

Confirmed: _delete() is depecriated code.
The correct call is _destroy()

Please implement in cvs copy and and close this bug.

Thanks (I'll check if i have cvs write access this evening)

The change:
Line 159 of event.js:
change from: c._delete();
to: c._destroy();

Logged In: YES
user_id=706287

The change has been applied in the dynapi-3.0.0-beta2
release. However I still get a fair amount of memory
leaking in FireFox 1.0.6 with the provided bug script. Can
someone else verify in other browsers? Either there's
another memory leak in the api, or the test case code, or
it's a Mozilla problem.

Logged In: YES
user_id=706287

The memory still leaks in IE6, although much slower than in
FireFox.