Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

[BZ 5651900] Binders not getting destroyed when using composite.execute and calling mp.destroyChildren #219

Closed
goonieiam opened this issue Jun 20, 2012 · 6 comments

Comments

@goonieiam
Copy link

Use case:

Mojits being created on demand via composite.execute(), then destroying them via mp.destroyChildren()

The first execution does the right thing, but upon doing the full cycle again(composite.execute then mp.destroyChildren()), the newly created child doesn't get destroyed.

The problem seems to be in destroyMojitProxy (mojito-client.client.js):

On a second run, this._mojits contains duplicated ids and this condition never evaluates to true, therefore never destroying it.

One thing I also noted is that this line :

this._mojits[id].proxy._destroy(retainNode);

creates a recursion, not sure if it has anything to do with the issue, but thought I'd mention.

if (this._mojits[id]) {
// TODO: activate call to unbindNode below:
// unbindNode(this._mojits[id].proxy._binder,
// this._mojits[id].handles);
this._mojits[id].proxy._destroy(retainNode);
delete this._mojits[id];
// We don't manage binder children automatically, but any time a
// new child is added or removed, we should at least give the
// application code a chance to stay up to date if they want to.
// The only gap is when a mojit destroys itself.
// onChildDestroyed is called whenever a binder is destroyed so
// any parents can be notified.
parent = findParentProxy(this._mojits, id);
if (parent && parent._binder.onChildDestroyed &&
Y.Lang.isFunction(parent._binder.onChildDestroyed)) {
parent._binder.onChildDestroyed({ id: id });
}
}

@rwaldura
Copy link
Contributor

goonieiam: is this something you can provide a patch for? Do you know exactly what needs to be fixed?

@goonieiam
Copy link
Author

I'd have to spend some time digging around, I don't have a clear solution I can provide. If anyone is able to point me in the right direction, I might be able to help.

@rwaldura
Copy link
Contributor

this._mojits contains duplicated ids

Can you view the IDs? Do they look like YUI_3_4_1_blahblah or YUI_3_5_1_blahblah?

1 similar comment
@rwaldura
Copy link
Contributor

this._mojits contains duplicated ids

Can you view the IDs? Do they look like YUI_3_4_1_blahblah or YUI_3_5_1_blahblah?

@rwaldura
Copy link
Contributor

BZ 5651900

@isao
Copy link
Contributor

isao commented Jun 23, 2012

Big thanks to Gonzalo Cordero for finding this memory leak, AND identifying the offending code. The leak affects dynamic mojits, and has existed for quite some time. Pull #225 was submitted for mojito's mainline, develop.

Note I made two branches:

  • mojit-leak off HEAD of develop which has currently has a broken mojito test app and uses YUI 3.5.1
  • mojit-leak-036 back port on top of the last mojito 0.3.26 release

@isao isao closed this as completed Jun 23, 2012
isao pushed a commit to isao/mojito that referenced this issue Jun 23, 2012
- record dynamically created mojits w/ their parent
  so they can be removed properly
- minor, rm redundant loop, lint
isao pushed a commit to isao/mojito that referenced this issue Jun 23, 2012
- record dynamically created mojits w/ their parent
  so they can be removed properly
- minor, rm redundant loop, lint
isao pushed a commit to isao/mojito that referenced this issue Jun 26, 2012
[fix YahooArchive#219 bz5651900] stop mojit data memory leak
@isao isao mentioned this issue Jul 16, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants