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

Tooltip Memory Leak #4499

Closed
rosslavery opened this issue May 12, 2017 · 11 comments · Fixed by #5144
Closed

Tooltip Memory Leak #4499

rosslavery opened this issue May 12, 2017 · 11 comments · Fixed by #5144
Assignees
Labels
P2 The issue is important to a large percentage of users, with a workaround

Comments

@rosslavery
Copy link

Bug, feature request, or proposal:

Bug

What is the expected behavior?

For tooltip to not leak memory.

What is the current behavior?

Tooltip seems to leak memory.

What are the steps to reproduce?

Plunker with tooltip:
https://plnkr.co/edit/IFpa3W9EXGG1Mch7TBmm?p=preview

Plunker without tooltip
https://plnkr.co/edit/vFdyEmedUYGCavMl8eEN?p=preview

Which versions of Angular, Material, OS, browsers are affected?

Used plunker template provided in issue template, so latest.

Is there anything else we should know?

I performed a devtools timeline recording and tried to keep it apples-to-apples. I performed GC at the start and end of each recording.

Strangely, both recordings seem to indicate leaking DOM nodes, which aren't cleaned up after a GC. The version with the tooltip however, doesn't clean up after itself nearly as nicely as the version without it.

Timeline Without Tooltip
without-tooltip

Timeline With Tooltip
with-toolitp

@kara kara added the P2 The issue is important to a large percentage of users, with a workaround label May 12, 2017
@rosslavery
Copy link
Author

Hey guys, any movement on this one? I have an app in production getting killed by this right now.

@andrewseguin
Copy link
Contributor

Working on a PR now that should fix this by removing the logic of dynamically adding/removing the tooltip. Instead it'll sit in the DOM for the life of the host element.

@rosslavery
Copy link
Author

@andrewseguin I'm not certain that is the cause, as the leak can be observed without ever actually hovering over the buttons that have the tooltip applied to them. In the plunker, you can run your mouse up and down the left side of the list items, without ever hovering over the buttons on the right that trigger the tooltips to show,

@andrewseguin
Copy link
Contributor

Can you confirm that removing HammerJS from the plunker helps resolve the issue? Looks to me that the CPU usage returns to normal if it's not imported.

@rosslavery
Copy link
Author

rosslavery commented Jun 1, 2017

@andrewseguin I can confirm that removing hammer makes a significant, measurable difference. Removing it fixes the DOM node leaks, cpu usage, and the effectiveness of garbage collection.

With Hammer:
with-hammer

Without Hammer:
without-hammer

Nice find...where do we go from here?

@michastreppel
Copy link
Contributor

The tooltip component starts listening for mouseenter and mouseleave events on construction. When hammerjs is present there will be a reference between the tooltip and hammerjs because of this (see screenshots of the heap snapshots). Due to this reference the tooltip cannot be garbage collected after the destruction of the component and memory will be leaked.

screen shot 2017-06-07 at 10 42 20
screen shot 2017-06-07 at 10 41 17

To let the tooltip be garbage collected on destruction either hammerjs should not be used or (preferably) the reference between the tooltip and hammerjs should be removed. Removing the reference is done by stop listening to the mouseenter and mouseleave events when the tooltip is destroyed (see my PR of May 26th).

@rntsn
Copy link

rntsn commented Jun 7, 2017

Just want to add that this affects md-slide-toggle as well.

mmalerba pushed a commit that referenced this issue Jul 6, 2017
Component should clean up events it has registered to.

Closes #4499
mmalerba pushed a commit that referenced this issue Jul 7, 2017
* fix(tooltip): remove native event listener on component destroy

Component should clean up events it has registered to.

Closes #4499

* Break at higher syntactic level

* Undo accidental formatting
amcdnl pushed a commit to amcdnl/material2 that referenced this issue Jul 8, 2017
…lar#5144)

* fix(tooltip): remove native event listener on component destroy

Component should clean up events it has registered to.

Closes angular#4499

* Break at higher syntactic level

* Undo accidental formatting
@Gil-Epshtain
Copy link

Gil-Epshtain commented Aug 12, 2018

I have the same issue (Angular and Material v6.x) and removing HummerJS from my project solve the memory leak in my case. However not I keep getting warning messages in the log:
Could not find HammerJS. Certain Angular Material components may not work correctly. [core.es5.js:170 ]
Hammer.js is not loaded, can not bind 'longpress' event. [core.js:3263]
How can I tell Material I'm not interested in HummerJS any more??? (so I won't see the warnings)

@jo-me
Copy link

jo-me commented Aug 24, 2018

@mmalerba @andrewseguin Is it possible that the PR did not actually resolve this issue?
I'm on Angular 6.1.4 with Material 6.4.6 and still have tooltip memleak issues..

@liquidcowgithub
Copy link

liquidcowgithub commented Sep 19, 2018

Hey

I'm getting this tooltip memleak issue as well with Angular 6.1.7, Material 6.4.7, and HammerJs 2.0.8.

Profile on Chrome (Version 68.0.3440.106) with HammerJs imported:
profile with hammerjs

Profile on Chrome without HammerJs imported:
profile without hammerjs

Should I open a new issue for this?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet
8 participants