Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

dialog: generating detached DOM nodes when closed #11207

Open
vinaynb opened this issue Apr 4, 2018 · 12 comments
Open

dialog: generating detached DOM nodes when closed #11207

vinaynb opened this issue Apr 4, 2018 · 12 comments
Assignees
Labels
P3: important Important issues that really should be fixed when possible. severity: performance This issue causes a significant performance degradation type: performance This issue is related to performance
Milestone

Comments

@vinaynb
Copy link

vinaynb commented Apr 4, 2018

Bug, feature request, or proposal:

Bug

What is the expected behavior?

Using $mdDialog with custom template should not result in detached DOM nodes.

What is the current behavior?

When using $mdDialog and after closing it via $mdDialog.hide(), the heap snapshots show lot of detached DOM nodes which point to instanceCache in $mdDialog service. Check out the heap snapshot screenshot

screenshot from 2018-04-04 16 06 41

I tried to figure out the internals and i think mdDialog is setting up some kind of internal cache which re-uses things when same modal's are opened multiple times but i failed to identify it concretely and clear those detached nodes from memory. I target to run my application on raspberry pi and it has quite a few modals hence increasing detached dom node count is warning signal for me leading to memory exhaustion over time. Hence i guess i need someone to familiar with internals to point me in right direction and if at all there is something leaking with mdDialog ?

CodePen and steps to reproduce the issue:

CodePen Demo which shows your issue:

I will try to reproduce this issue on codepen but as of now i don't have a pen.

Detailed Reproduction Steps:

I am using ui router and while switching between states if i open a modal with custom template and close it and then check the snapshot it shows detached nodes. Those nodes don't show if i don't open modal at all.

Which versions of AngularJS, Material, OS, and browsers are affected?

Angular js - 1.6.8
OS - linux/ubuntu 14.04
browser - chrome 65

@Splaktar Splaktar added needs: demo A CodePen demo or GitHub repository is needed to demonstrate the reproduction of the issue needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Apr 5, 2018
@Splaktar Splaktar changed the title Leaking dom nodes when using $mdDialog dialog: leaking detached DOM nodes Apr 5, 2018
@Splaktar
Copy link
Member

Splaktar commented Apr 5, 2018

What version of AngularJS Material is in use?

Please provide a demo and reproduction steps for checking the heap and observing detached nodes. Also, are the nodes eventually garbage collected over time or do they keep accumulating?

@vinaynb
Copy link
Author

vinaynb commented Apr 6, 2018

Angular material version - 1.1.9 (forked)
I have created a basic repro here.

I must note that although there are detached nodes in the pen i mentioned above, the retainer graph is for them is not the same as i have in my angular application. But maybe this example might help in debugging.

Steps to reproduce

  • open pen and take a heap snapshot
  • open dialog by clicking TEST button and close it by pressing ESC or area outside the modal
  • again take a heap snapshot and notice the newly added detached dom nodes (more than 500 nodes approx show up)
  • also check out Scope tree in last heapshot and we find the whole controller for mdDialog is present even though it is destroyed. Check out the images below showcasing both the issues.

Also, are the nodes even garbage collected over time or do they keep accumulating?

From what i see they are not being gc'ed even though i fired it manually but at the same time when they do not keep accumulating if it open the same dialog multiple times. Maybe its caching at play here.

screenshot from 2018-04-06 12 28 23
screenshot from 2018-04-06 12 29 03

@Splaktar Splaktar added type: performance This issue is related to performance needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community severity: performance This issue causes a significant performance degradation and removed needs: demo A CodePen demo or GitHub repository is needed to demonstrate the reproduction of the issue needs: more info The issue does not contain enough information for the team to determine if it is a real bug labels Apr 7, 2018
@Splaktar Splaktar added this to the 1.1.10 milestone Apr 7, 2018
@Splaktar Splaktar added for: external contributor P3: important Important issues that really should be fixed when possible. labels Apr 7, 2018
@Splaktar Splaktar modified the milestones: 1.1.10, 1.1.11 Jun 19, 2018
@Splaktar Splaktar modified the milestones: 1.1.11, 1.1.12 Sep 10, 2018
@Splaktar
Copy link
Member

Splaktar commented Oct 4, 2018

#10851 also mentions that $mdDialog is leaking memory.

@Splaktar Splaktar self-assigned this Oct 4, 2018
@Splaktar Splaktar added P2: required Issues that must be fixed. and removed P3: important Important issues that really should be fixed when possible. labels Oct 4, 2018
@Splaktar Splaktar added P1: urgent Urgent issues that should be addressed in the next minor or patch release. and removed P2: required Issues that must be fixed. labels Oct 25, 2018
@Splaktar
Copy link
Member

#11493 has some additional debugging information that is related to this issue.

@Splaktar Splaktar modified the milestones: 1.1.12, 1.1.13 Jan 3, 2019
@Splaktar Splaktar removed this from the 1.1.13 milestone Feb 10, 2019
@Splaktar Splaktar added this to the 1.1.22 milestone Sep 30, 2019
@Splaktar Splaktar modified the milestones: 1.1.22, 1.1.23 Oct 22, 2019
@Splaktar Splaktar modified the milestones: 1.1.23, 1.2.0 Jun 8, 2020
@Splaktar Splaktar modified the milestones: 1.2.0, 1.2.1 Jul 29, 2020
@Splaktar
Copy link
Member

In angular/angular.js#4864 (comment), it is mentioned that ngAnimate causes this same behavior of Nodes and listeners increasing over time, but the JS Head remaining stable. We of course do use ngAnimate.

@Splaktar
Copy link
Member

Splaktar commented Sep 13, 2020

Screen Shot 2020-09-13 at 02 40 46

Here's a profiling session with 1.2.0 that was about 50 seconds long. You can see that, like the JS Heap, the Nodes and listeners are garbage collected away (less often than for the JS).

I've found a minor issue where we may leak two listeners per dialog closing, but fixing it doesn't really change the behavior in these profiling sessions.

Splaktar added a commit that referenced this issue Sep 13, 2020
- improve JSDoc and Closure types
- fix typos
- simplify `isNodeOneOf()` and ensure it returns a `boolean`

Relates to #11207
@Splaktar Splaktar added severity: memory leak Issues where a memory leak is triggered. and removed for: external contributor needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community labels Sep 13, 2020
Splaktar added a commit that referenced this issue Sep 13, 2020
- improve JSDoc and Closure types
- fix typos
- simplify `isNodeOneOf()` and ensure it returns a `boolean`
- fix location of comments in dialog demo

Relates to #11207
@Splaktar
Copy link
Member

I tried with ngAnimate disabled via $animate.enabled(false); but it doesn't seem to have a significant impact:

Screen Shot 2020-09-13 at 04 22 09

@Splaktar
Copy link
Member

I'm going to lower the priority on this because I think that most of the leaks related to this have been resolved. The Nodes and Listeners tha increase, do seem to be garbage collected.

I would be happy to dig into this some more if someone could point out a specific use case/repro with AngularJS Material 1.2.0 (or hopefully 1.2.1 soon) along with some of their investigation findings.

@Splaktar Splaktar added P3: important Important issues that really should be fixed when possible. and removed P1: urgent Urgent issues that should be addressed in the next minor or patch release. severity: memory leak Issues where a memory leak is triggered. labels Sep 13, 2020
@Splaktar Splaktar changed the title dialog: leaking detached DOM nodes dialog: generating detached DOM nodes when closed Sep 13, 2020
Splaktar added a commit that referenced this issue Sep 13, 2020
- improve JSDoc and Closure types
- fix typos
- simplify `isNodeOneOf()` and ensure it returns a `boolean`
- fix location of comments in dialog demo
- apply some npm audit fixes to `package-lock.json`

Relates to #11207. Closes #12010.
Splaktar added a commit that referenced this issue Sep 13, 2020
- improve JSDoc and Closure types
- fix typos
- simplify `isNodeOneOf()` and ensure it returns a `boolean`
- fix location of comments in dialog demo
- apply some npm audit fixes to `package-lock.json`

Relates to #11207. Closes #12010.
Splaktar added a commit that referenced this issue Sep 14, 2020
- improve JSDoc and Closure types
- fix typos
- simplify `isNodeOneOf()` and ensure it returns a `boolean`
- fix location of comments in dialog demo
- apply some npm audit fixes to `package-lock.json`

Relates to #11207. Closes #12010.
@Splaktar Splaktar modified the milestones: 1.2.1, 1.2.2 Sep 14, 2020
@Splaktar Splaktar modified the milestones: 1.2.2, - Backlog Dec 16, 2020
@Splaktar Splaktar removed the g3: sync label Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3: important Important issues that really should be fixed when possible. severity: performance This issue causes a significant performance degradation type: performance This issue is related to performance
Projects
None yet
Development

No branches or pull requests

2 participants