ember-animation / liquid-fire

Animations & transitions for ambitious Ember applications.
Other
1.15k stars 199 forks source link

Massive DOM leaks when animating multiple items on page #310

Open nonmanifold opened 9 years ago

nonmanifold commented 9 years ago

PR #309 fixes DOM leakage due to view nodes being removed when velocity haven't detached its $.data() from view root node. Please review PR and move $.removeData() calls into more appropriate location. And please check cleanup of animationContext when animation run.

dmoreno commented 9 years ago

I also see a memory leak inside liquid-with because jQuery cache keeps a reference of liquid-child.

In my component, I had to do a weird workaround after animation:

if (this._prevLiquid)
{
  this._prevLiquid.remove();
}
this._prevLiquid = this.$(".liquid-child");
this.$(".liquid-child").removeData();
this.$(".liquid-container").removeData("velocity");
simonihmig commented 8 years ago

I can see this, too.

By diffing Chrome heap snapshots I could see that there are a couple of detached divs leaked, as well as some promises. See the following screenshots from my real world app:

bildschirmfoto 2016-03-15 um 19 00 14

bildschirmfoto 2016-03-15 um 19 01 35

The retainer path shows it comes from $.data. As soon as I remove the transitions in transition.js, the leaks are gone, the heap diff shows a delta of zero!

I did a workaround for my app by wrapping the default transitions in a custom transition, that calls $.removeData() on the oldElement after the transition has finished. This removed the leaks as well:

// transitions/clean.js
export default function clean(animationName, ...args) {
  return this.lookup(animationName)
    .apply(this, args)
    .then(() => {
      this.oldElement.removeData();
    });
}

// transitions.js
export default function(){
  this.transition(
      this.use('clean', 'fade', {duration: 500})
  );
}

@ef4 as you already mentioned in https://github.com/ember-animation/liquid-fire/pull/309#issuecomment-113651959, the point is probably that the old elements get removed from DOM by Ember and not jQuery, thus keeping references to the detached DOM elements in $.data preventing GC.

Might be neglectable for desktop systems, but this and some other memory leak issues causes us some serious stability problems on mobile platforms, so looking forward to a fix! :)

winne42 commented 4 years ago

@ef4 If https://github.com/ember-animation/liquid-fire/pull/309 could be closed, can this issue be closed, too?

ef4 commented 4 years ago

@winne42 I don't really have time to prioritize grooming the bug backlog here, you are probably correct that all these issues you have been commenting on can be closed, but I'm not able to help you with that right now. If you are volunteering to help do triage, that would be great and I can give you permission to close issues.