Using ngIf, DOM nodes cannot be garbage collected because of form control

[kallebjork]

🐞 bug report

Is this a regression?

No

Description

Not sure if I am using this in the wrong way. I get a leak when I switch a container (using ngIf) which has reference to form control inside it.

I provided a stackblitz for reproducing the issue.

I also provided a stackblitz where I remove the control from the form group when I toggle the container. That resolves the issue but is that what I am expected to do or how do I go about this?

🔬 Minimal Reproduction

To reproduce:
https://stackblitz.com/edit/angular-9-ngif-leak

Example where I remove the form control on every toggle:
https://stackblitz.com/edit/angular-9-ngif-remove-control

🔥 Exception or Error

The full container with children will be left in detached mode on each switch. I think the DefaultValueAccessor is the retainer.

🌍 Your Environment

Angular Version:
9.0.1

[petebacondarwin]

I simplified the reproduction to remove all the unnecessary Angular Material components.

https://stackblitz.com/edit/angular-9-ngif-leak-bms7sb?file=src%2Fapp%2Fapp.component.html

I can see in heap snapshot after the ngIf has been set to false that the undelying HTMLTextAreaElement is being retained by the valueAccessor in the Reactive Forms.
While I expect the FormControl object to be retained, we are holding on to the underlying DOM element.

I think that we need to do some work to disconnect the underlying element from the FormControl when the element containing the FormControlName directive is destroyed.

[petebacondarwin]

Possibly a duplicate of #20007

[kallebjork]

Thanks for the quick triage @petebacondarwin!

[kallebjork]

Will likely be solved by #39235 ?

[AndrewKushnir]

Hi @kallebjork, I believe this issue should be resolved by #39235. If you get a chance to verify your use-case with the changes from #39235 (see #20007 (comment) for more info) - that's be very helpful. Thank you.

[kallebjork]

Thank you @AndrewKushnir! I think this might be tricky to test in our code right now since we publish a lot of libs and all of them need to be upgraded to 11. We will of course want to do this anyway but it might take some time. In the meantime, I tried it out on a stackblitz and it is looking really good (red marker is a forced garbage collect):

Nice work @AndrewKushnir and whoever else has been involved, much appreciated.

[AndrewKushnir]

@kallebjork thanks for testing the fix and sharing the results with us!

I think this might be tricky to test in our code right now since we publish a lot of libs and all of them need to be upgraded to 11. We will of course want to do this anyway but it might take some time.

If you're using relatively up-to-date version of Angular (v10), you may still be able to test the fix from #39235 (I just wanted to make sure that you don't come across the issues resolved in v11). I'd be curious to hear if the fix from #39235 resolves the problem that you are seeing in your real app. Thank you.

[kallebjork]

@AndrewKushnir Applying it to our current version worked (10.2.3).

We have a test page with almost all of our components and a button for toggling that whole template. I toggled the whole template once and then 10 times, to see how much nodes and heap built up for those 10 times. That scenario does not leak any more. Running that test first without the patch and then with the patch resulted in a huge diff:

We do track our nodes and heap during protractor runs and I would like to see what the patch does there (more of a real use case) but currently there are some issues preventing me from running those with the patch. I might get back with that result. That would give us an understanding of the impact of the fix. With that said, I think this answers the question whether the reported issue has been fixed. 🥇

[AndrewKushnir]

@kallebjork, thanks for testing the changes and sharing the results! 👍
Glad to hear the the fix resolves the memory leak you're seeing in the app at this moment. Let us know if you have more results available in the future.

We'll keep this thread updated about the next steps for PR #39235.

[kallebjork]

@AndrewKushnir Ok, I finally got the chance to test this in our memory stress test. The result looks so good that I worry something is wrong 😬

The test interacts with pretty much all of our components and repeats those interactions 100 times and compare the heap size and nodes with the initial numbers (after forcing garbage collection and letting things settle).

The test starts at 42mb so the heap size went from 54mb to 7mb and the dom nodes from adding 23290 to only adding 595.

Like I said, I hope there is nothing wrong with this data, it is looking almost too good to be true!

[AndrewKushnir]

Thanks for the update @kallebjork, the before/after delta looks really great 👍

If there are no critical issues found in PR #39235, we'll try to land the changes in the next minor version (v11.1.0).

[AndrewKushnir]

Quick update: the fix (PR #39235) was merged and should be available in v11.1.0 release later this month.

[andreialecu]

11.1.0 has been released now and I can confirm that on my forms everything seems better.

One other thing I discovered though. Some browser extensions create memory leaks of their own in pages. See false alarm here.

I confirmed LastPass and React Developer Tools to both retain form controls or add other memory bloat to pages. I have since set most of my extensions to only activate when I click them.

If you still have memory issues, make sure to run without any browser extensions first.

[maxime1992]

Hi @AndrewKushnir :)

I wasn't sure where to ping you as you asked here so while this thread is still alive, here we are :). Let me know if you want me to open a new issue instead 👍

We finally manage to upgrade to ng-version="11.2.0" (taken on the root component). I've also made sure from our yarn.lock that we're not using any other angular version and we're not. Unfortunately, it looks like the fix is still here. I checked in a private window with all my extensions turned off:

Looks like our heavy pages containing loads of forms (built using ControlValueAccessors) are still not correctly garbage collected in the DOM 🤔

Let me know how you want me to proceed as the app is not public 👍

EDIT: I also waited for quite a long time to make sure and tried to manually request a garbage collection from the dev tool but it didn't change anything

[gkalpak]

I haven't looked at this issue at all, so it could be totally unrelated, but there seems to be a memory link in recent versions of Chrome related to <input> elements: #40614

(For reference, it has been reported here.)

[AndrewKushnir]

Hi @maxime1992, thanks for sharing the results after testing your app with the most recent versions of Angular packages.

We've received feedback from users in open source community as well as some internal teams that the mentioned Forms fix resolved memory leaks in many situations. So I suspect that the memory leak that you observe might have a different root cause (for ex. as @gkalpak mentioned, it might be related to <input> elements).

You mentioned that the page you are using for testing contains a lot of forms. I'd be very helpful if you could narrow down the problem (for ex. by removing parts of the application and checking if the mem leak is gone) and try to create a minimal repro (without sharing the code of your app), so that we can investigate the problem further.

Thank you.

[angular-automatic-lock-bot]

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.}