Skip to content

fix(material): use same reference for adding and removing event listener#3588

Merged
aitboudad merged 1 commit into
ngx-formly:mainfrom
Arooba-git:fix-event-listeners-memory-leaks
Feb 13, 2023
Merged

fix(material): use same reference for adding and removing event listener#3588
aitboudad merged 1 commit into
ngx-formly:mainfrom
Arooba-git:fix-event-listeners-memory-leaks

Conversation

@Arooba-git
Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

Hello 👋
As part of our project, we are using Facebook's new Memlab tool to detect memory leaks in SPA applications and their libraries. While running the tool and analyzing the code of ngx-formly, we saw that your project does a very good job of ensuring that all async operations are cancelled when components destroy. However, as per Memlab execution results, we found some unremoved listeners, causing the memory to leak (screenshots below).

[before]
ngx-formly-before Screen Shot 2023-02-13 at 5 46 05 AM

Hence we added the fix by adding & removing the listeners with proper references, and you can see the heap size and count of leaks reducing noticeably:

formly-after-add-event Screen Shot 2023-02-13 at 6 02 17 AM

Basically .bind() creates a new function reference each time [1-5]; which means the reference passed to the add and remove events were different, and hence the listeners were not actually being removed.

We executed the test suite after the fixes and it successfully passed 56 of 56 total (in watch + non-watch mode)

You can analyze this and other potential leak sources, if you like, by running Memlab with a scenario file covering maximum count of use cases.

Following is a sample of the scenario file we used (it needs to be a .js file but attaching here in .txt form):
ngx-formly-scenario.txt

Note that some other reported leaks (in Memlab) originated from Angular's internal objects, and hence were ignored.

References:
https://kostasbariotis.com/removeeventlistener-and-this/
https://stackoverflow.com/questions/11565471/removing-event-listener-which-was-added-with-bind
https://stackoverflow.com/questions/28665784/how-to-remove-event-listener-of-a-object-with-bindthis
https://stackoverflow.com/questions/54921027/remove-event-listener-that-has-been-added-using-bindthis
https://stackoverflow.com/questions/46455624/javascript-addeventlistener-removeeventlistener-and-bind

@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 12, 2023

Deploy Preview for formly-dev ready!

Name Link
🔨 Latest commit c80bec3
🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/63e9722274bdf60008bebf2d
😎 Deploy Preview https://deploy-preview-3588--formly-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@aitboudad aitboudad merged commit 22c2ec1 into ngx-formly:main Feb 13, 2023
@aitboudad
Copy link
Copy Markdown
Member

Thank you @Arooba-git!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants