Skip to content
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

fix: use function reference to add & remove listener #219

Merged

Conversation

billerr
Copy link
Contributor

@billerr billerr commented Jul 4, 2023

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)

@DamianGlowala
Copy link
Contributor

@samuelOsborne, could this PR be reviewed and merged, please? This is causing issues community would really like to be addressed.

@samuelOsborne samuelOsborne merged commit 151b863 into LottieFiles:master Mar 4, 2024
@samuelOsborne
Copy link
Member

merged @DamianGlowala Available in v2.0.4

@SevrainChea
Copy link

Hi @samuelOsborne I see that this PR has been merged and you said it would be available in. v2.0.4. We are currently in v.2.0.1 with the last release being 8 June 2023
Do you have a timeline on when this fix will be available please ?
Thanks for your hard work 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants