New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reorder remove() and _leaveChildren() in leave method under composite_view.js #32

Open
Kufert opened this Issue Mar 2, 2017 · 2 comments

Comments

Projects
None yet
2 participants
@Kufert

Kufert commented Mar 2, 2017

I'm having some memory leaks caused by components added to the screen via various jQuery plugins. After few hours of debugging, I found that those plugins have a destroy API I can all from the view before detaching it to prevent the memory leak. I tried to call this from leave() on the view, but that did not work.

It seems like the issue is now with the order of the calls remove() and _leaveChildren() in the code:

  leave: function() {
    this.trigger('leave');
    this.unbind();
    this.stopListening();
    this.remove();
    this._leaveChildren();
    this._removeFromParent();
  },

because remove() removes the view, and only then the view leave() method is called by running _leaveChildren(), the item is not really destroyed as it is no longer on the scree, resulting in memory leak.

My Q - can I change the order? what is the reason behind the current order?
If you think it's OK, I'll create PR with that change.

Thanks!

@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Mar 6, 2017

Collaborator

To confirm I understand the problem, I tried to call this from leave() on the view, but that did not work. means code similar to the following?

var NodeView = CompositeView.extend({
  render: function() {
    this.$el.plugin();
  },

  leave: function() {
    this.$el.plugin("destroy");
    CompositeView.prototype.leave.call(this);
  }
});

var parent = new NodeView();
var child = new NodeView();
parent.appendChild(child);

  // `.plugin("destroy")` in custom `leave()` does not work when called by `child`
parent.leave();

If so, we can change the order. From git history the current order is how the methods were added.

Collaborator

ecbypi commented Mar 6, 2017

To confirm I understand the problem, I tried to call this from leave() on the view, but that did not work. means code similar to the following?

var NodeView = CompositeView.extend({
  render: function() {
    this.$el.plugin();
  },

  leave: function() {
    this.$el.plugin("destroy");
    CompositeView.prototype.leave.call(this);
  }
});

var parent = new NodeView();
var child = new NodeView();
parent.appendChild(child);

  // `.plugin("destroy")` in custom `leave()` does not work when called by `child`
parent.leave();

If so, we can change the order. From git history the current order is how the methods were added.

@Kufert

This comment has been minimized.

Show comment
Hide comment
@Kufert

Kufert Mar 8, 2017

@ecbypi yes that's the case. The issue is that jQuery plugins, such as typeahead, bootstrap-timepicker and many others, are not destroyed properly.

Reason is that when adding anything to the DOM using jQuery, jQuery saves a reference to that item event listeners. That data is stored under $.cache (what's $.cache?).
The problem is that even when I try to use destroy on the leave method, the event listeners are not removed as their bound element is not in the DOM anymore, which it turn create a memory leak.

When calling _leaveChildren() before remove(), the leak is prevented.

  leave: function() {
    this.trigger('leave');
    this.unbind();
    this.stopListening();
    this._leaveChildren();
    this.remove();
    this._removeFromParent();
  },

make it all makes sense :)

Kufert commented Mar 8, 2017

@ecbypi yes that's the case. The issue is that jQuery plugins, such as typeahead, bootstrap-timepicker and many others, are not destroyed properly.

Reason is that when adding anything to the DOM using jQuery, jQuery saves a reference to that item event listeners. That data is stored under $.cache (what's $.cache?).
The problem is that even when I try to use destroy on the leave method, the event listeners are not removed as their bound element is not in the DOM anymore, which it turn create a memory leak.

When calling _leaveChildren() before remove(), the leak is prevented.

  leave: function() {
    this.trigger('leave');
    this.unbind();
    this.stopListening();
    this._leaveChildren();
    this.remove();
    this._removeFromParent();
  },

make it all makes sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment