primefaces / primevue

Next Generation Vue UI Component Library
https://primevue.org
MIT License
8.77k stars 1.09k forks source link

PrimeVue Tooltip: memory leak #5856

Open csakis opened 3 weeks ago

csakis commented 3 weeks ago

Describe the bug

The tooltip component causes serious memory leak if the tooltip is attached to a re-rendered component. I created a minimal application to demonstrate the issue. The application was monitored with Chrome task manager. When the application started up the memory usage was this: image After a couple of hours running, JS memory usage increased to 44MB: image

Reproducer

https://stackblitz.com/~/github.com/csakis/prime-test?file=vite.config.ts

PrimeVue version

3.52.0

Vue version

3.x

Language

TypeScript

Build / Runtime

Vite

Browser(s)

Chrome

Steps to reproduce the behavior

  1. Run project
  2. Monitor JS memory usage

Expected behavior

Memory usage should be constant, however, it keeps growing steadily,

DrJonki commented 3 weeks ago

Most likely a duplicate of https://github.com/primefaces/primevue/issues/5629.

csakis commented 3 weeks ago

True, but I provided a reproducible repo.

emp-mas commented 3 weeks ago

The Tooltip component leaks event listeners (click, keydown and mouseleave) on each execution of the updated hook. The updated hooks calls unbindEvents() followed by bindEvents().

In bindEvents() of Tooltip.js the event listeners are created like this: el.addEventListener('click', this.onClick.bind(this)); Function.prototype.bind() returns a new bound function, not a reference to the original function.

Therefore the removal of the event listeners in unbindEvents() using el.removeEventListener('click', this.onClick.bind(this)); is not working. Calling this.onClick.bind(this) returns again a new function and not a reference to the handler function used in bindEvents().

Demo: https://stackblitz.com/edit/zafh9e?file=src%2FApp.vue

To be able to remove the event listeners afterwards, it's necessary to store a reference to the handler function like it's done for the mouseenter event: el.$_mouseenterevent = (event) => this.onMouseEnter(event, options); el.addEventListener('mouseenter', el.$_mouseenterevent); Removal: el.removeEventListener('mouseenter', el.$_mouseenterevent); el.$_mouseenterevent = null;

andersnm commented 3 weeks ago

Hi, searching for more occurrences of @emp-mas 's finding, the ripple directive has the same issue

https://github.com/primefaces/primevue/blob/917ef22d9a4afcea0c59d1e4644218b3f0dcd222/packages/primevue/src/ripple/Ripple.js#L25-L30

This affects a bunch of components, but only if enabled (...so its not the leak I'm looking for)

ZiadJ commented 3 weeks ago

Perhaps a WeakMap would be a better alternative. So instead of adding a property to the targeted element, it would be added to the WeakMap instead, using the element as key. The benefit over Map is that it allows the value to be garbage collected once the key is removed in unbindEvents. The downside it that it's not iteratable which is fine in this case.