lottie-player

lottie-player copied to clipboard

This pull request fixes a memory leak from incorrect usage of removeEventListener - see reproduction steps below.

The problem is that _onVisibilityChange is not provided as a reference to addEventListener / removeEventListener (so that it can be matched and removed). The inline arrow functions that are provided as event handlers are actually different references so they don't get removed. Every time a lottie-player element gets removed from the DOM, its visibilitychange listener (the arrow function & its closure) doesn't get cleaned and leads to memory leaks.

Reproduction

You can reproduce it with this sample app: https://codesandbox.io/s/summer-brook-szmng7

1. Download the project (File > Export to ZIP) and install / run locally (see README)
2. Open the project page in Chrome (http://localhost:8080/) and press the "Toggle" button many times
3. Open Chrome DevTools on the "Memory" panel, and take a heap snapshot
4. Look for detached HTML elements in the snapshot and you will find several lottie instances there.

Solutions:

1. (I implemented this one based on TS docs) Convert _onVisibilityChange to an arrow function (so this gets bound correctly to declaration context and always refers to the "LottiePlayer" instance) and provide its reference as an event handler. This keeps the reference the same and it can get removed properly.
2. (Alternative if we want to avoid arrow function declarations) We can keep _onVisibilityChange as is, but we need to bind this to it in the constructor with this._onVisibilityChange = this._onVisibilityChange.bind(this);. Again, the arrow functions should be removed from event handling and use this._onVisibilityChange as a reference there.

---

Note: readonly isn't needed for this fix, it was added to satisfy an eslint warning (@typescript-eslint/prefer-readonly)