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

Popover directive with delay can cause a memory leak #6709

Open
EvyatarBHA opened this issue Nov 17, 2024 · 6 comments
Open

Popover directive with delay can cause a memory leak #6709

EvyatarBHA opened this issue Nov 17, 2024 · 6 comments

Comments

@EvyatarBHA
Copy link

In Tooltip directive there's _delaySubscription member, which is unsubscribed in ngOnDestroy method.
However, in Popover directive there's const _timer which isn't being handled in ngOnDestroy method.
This can cause issues in case directive is being destroyed while subscription is active.
In our case, it happened when we used the Popover directive to show additional row details in an ag-grid cell renderer after hovering for 500ms, but it was destroyed when the grid was scrolled.

In addition, both directives contains an old and unused member _delayTimeoutId, which better be deleted regardless.

Versions of ngx-bootstrap, Angular, and Bootstrap:
ngx-bootstrap: 12.0.0 (but exists in latest code too)
Angular: 17.3.0
Bootstrap: 5.3.3

Expected behavior
Delay timer subscription should be unsubscribed if needed in ngOnDestroy method of Popover directive

@lexasq
Copy link
Contributor

lexasq commented Dec 19, 2024

Tooltip unsubscribe triggers on close trigger, could you please share reproduction or maybe you could use this event and avoid leaks.

@EvyatarBHA
Copy link
Author

Issue is in Popover directive. My solution for now was using Tooltip directive instead, as it doesn't contain this issue, but I assume Popover should be fixed regardless.
Essentially same member behavior can be copied from Tooltip to Popover directive.
Simply compare the directives' code and see the difference in regards to delayed showing.
When using Popover, in case you try destroying the component with the directive while the timer subscription is alive, it will not be destroyed and popover would display despite not needed anymore.

@lexasq
Copy link
Contributor

lexasq commented Dec 19, 2024

My mistake, Popover close trigger clears timer.
Popover and Tooltip are not the same, if you like to provide reproduction there might be a way to show you how it should be done.

@EvyatarBHA
Copy link
Author

EvyatarBHA commented Dec 19, 2024

The problem is with this subscription -

const _timer = timer(this.delay).subscribe(() => {
        showPopover();
        cancelDelayedTooltipShowing();
      });

If component gets destroyed before reaching delay timeout and close trigger is somehow missed.
It would be complex to show our scenario, as it was a rich delayed tooltip (using Popover directive) for displaying additional data inside ag-grid cell renderer.
We had triggers="mouseenter:mouseleave" and a short delay defined.

@lexasq
Copy link
Contributor

lexasq commented Dec 19, 2024

Without a sample that would be hard to help you.

@EvyatarBHA
Copy link
Author

Again, I don't need help here.
For me, the solution was switching to Tooltip directive, where the bug doesn't exist.
However, this is a widely used project and the solution seems to pretty straight forward -
Instead of in-function const, use class member like in Tooltip directive, where same functionality is implemented slightly better.

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

No branches or pull requests

2 participants