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.

Sign up for GitHub

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

Jump to bottom

fix(forms): clean up connection between FormControl/FormGroup and corresponding directive instances #39235

Closed
wants to merge 1 commit into from
Closed

fix(forms): clean up connection between FormControl/FormGroup and corresponding directive instances #39235

wants to merge 1 commit into from

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Oct 13, 2020 •
edited

Prior to this commit, removing FormControlDirective and FormGroupName directive instances didn't clear
the callbacks previously registered on FromControl/FormGroup class instances. As a result, these callbacks
were executed even after FormControlDirective and FormGroupName directive instances was destroyed. That was
also causing memory leaks since these callbacks also retained references to DOM elements.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@ngbot ngbot bot modified the milestone: needsTriage Oct 13, 2020
@google-cla google-cla bot added the cla: yes label Oct 13, 2020
@AndrewKushnir AndrewKushnir mentioned this pull request Oct 13, 2020
Closed
3 tasks
@AndrewKushnir
Copy link
Contributor Author

Presubmit.

@AndrewKushnir AndrewKushnir mentioned this pull request Nov 4, 2020
Closed
3 tasks
@kallebjork kallebjork mentioned this pull request Nov 4, 2020
Closed
@crisbeto crisbeto mentioned this pull request Nov 6, 2020
Closed
@blyndcide blyndcide mentioned this pull request Nov 6, 2020
Closed
This was referenced Nov 6, 2020
Closed
Closed
@AndrewKushnir
Copy link
Contributor Author

Note: this PR should be rebased once #39623 lands. PR #39623 contains cleanUpControl function fixes also (partially) present in this PR.

@AndrewKushnir AndrewKushnir mentioned this pull request Nov 12, 2020
Closed
3 tasks
@AndrewKushnir AndrewKushnir force-pushed the forms_cva_cleanup branch 4 times, most recently from 07def58 to 90fe4e5 Compare November 13, 2020 02:54
@AndrewKushnir AndrewKushnir changed the title fix(forms): clean up ControlValueAccessor connection when FormControl is destroyed fix(forms): clean up connection between FormControl/FormGroup and corresponding directive instances Nov 13, 2020
@AndrewKushnir AndrewKushnir added the target: patch This PR is targeted for the next patch release label Nov 13, 2020
@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Nov 13, 2020 •
edited

Presubmit #2 + Global Presubmit.

@AndrewKushnir AndrewKushnir force-pushed the forms_cva_cleanup branch 2 times, most recently from a7669c9 to 68097e6 Compare November 13, 2020 03:21
@kallebjork
Copy link

🥳 Kudos, Andrew!

@AndrewKushnir
Copy link
Contributor Author

Quick update: while testing this change I verified that the memory is released when formControl and formControlName directives are used with *ngIfs (or otherwise removed and re-added to the DOM), however there is still a problem with the formGroup. Investigation revealed that the problem was caused by the onCollectionChange callbacks retained on FormGroup instances after formGroup is destroyed. This is causing the formGroup instance to be present in memory and holding a reference to the nested formControl instances (which in turn refer to DOM elements via ElementRefs). In order to release the memory, the onCollectionChange callbacks should be cleaned up too. The problem with the onCollectionChange callbacks is that there is no way to clear a particular callback (attached by a given directive instance that is being removed), internal API only allows to replace current callback with a new one (thus we may damage FormGroup in case it's used in multiple places). I've created a new PR #39685 to extend internal APIs to allow adding/removing individual callbacks and that PR should unblock further tests for the formGroup scenarios in this PR.

@ngbot ngbot bot removed this from the Backlog milestone Dec 9, 2020
@pullapprove pullapprove bot added area: core Issues related to the framework runtime comp: docs/api area: forms labels Dec 10, 2020
@ngbot ngbot bot added this to the Backlog milestone Dec 10, 2020
This was referenced Dec 16, 2020
Closed
Closed
@AndrewKushnir
fix(forms): clean up connection between FormControl/FormGroup and cor…
1e746f1 
…responding directive instances

Prior to this commit, removing `FormControlDirective` and `FormGroupName` directive instances didn't clear
the callbacks previously registered on FromControl/FormGroup class instances. As a result, these callbacks
were executed even after `FormControlDirective` and `FormGroupName` directive instances were destroyed. That was
also causing memory leaks since these callbacks also retained references to DOM elements.

This commit updates the cleanup logic to take care of properly detaching FormControl/FormGroup/FormArray instances
from the view by removing view-specific callback at destroy time.

Closes angular#20007, angular#37431, angular#39590.
@AndrewKushnir
Copy link
Contributor Author

Global Presubmit.

@AndrewKushnir AndrewKushnir added risk: medium action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 5, 2021
@AndrewKushnir
Copy link
Contributor Author

Global TAP run is successful, other checks are "green" too. The fix was also manually verified with a number of use-cases, so this PR is ready to go.

Note to Caretaker: it'd be great to land this change as a separate sync CL so that it's easier to rollback if needed.

@maxime1992
Copy link

@AndrewKushnir thanks a lot for all the work you've put in this, it'll be super helpful 🙏!
Muuuuuch appreciated!

@AndrewKushnir AndrewKushnir mentioned this pull request Jan 6, 2021
Closed
@SebastianStehle SebastianStehle mentioned this pull request Jan 12, 2021
Closed
@AndrewKushnir AndrewKushnir mentioned this pull request Jan 12, 2021
Closed
This was referenced Jan 19, 2021
Closed
Closed
@danzrou danzrou mentioned this pull request Jan 21, 2021
Closed
AndrewKushnir added a commit to AndrewKushnir/angular that referenced this pull request Jan 22, 2021
@AndrewKushnir
fix(forms): properly cleanup in cases when FormControlName has no CVA
795601e 
PR angular#39235 introduced additional cleanup logic for form controls and directives. The cleanup logic relies
on the presence of ControlValueAccessor instances on FormControlName and FormControl directives. In general
these fields are present and there are also checks to make sure that the mentioned directive instances are
created with CVAs. However some scenarios (primarily tests) may invoke the logic in a way that the directive
instance would not be fully initialized, thus causing CVA to be absent. As a result, the cleanup logic fails
while trying to call some methods on associated CVA instances.

This commit updates the cleanup logic to take into account the situation when CVA is not present.

Fixes angular#40521.
This was referenced Jan 22, 2021
Closed
Closed
jessicajaniuk pushed a commit that referenced this pull request Jan 25, 2021
@AndrewKushnir @thePunderWoman
fix(forms): properly cleanup in cases when FormControlName has no CVA (…
72fc6aa 
…#40526)

PR #39235 introduced additional cleanup logic for form controls and directives. The cleanup logic relies
on the presence of ControlValueAccessor instances on FormControlName and FormControl directives. In general
these fields are present and there are also checks to make sure that the mentioned directive instances are
created with CVAs. However some scenarios (primarily tests) may invoke the logic in a way that the directive
instance would not be fully initialized, thus causing CVA to be absent. As a result, the cleanup logic fails
while trying to call some methods on associated CVA instances.

This commit updates the cleanup logic to take into account the situation when CVA is not present.

Fixes #40521.

PR Close #40526
jessicajaniuk pushed a commit that referenced this pull request Jan 25, 2021
@AndrewKushnir @thePunderWoman
fix(forms): properly cleanup in cases when FormControlName has no CVA (…
4d66185 
…#40526)

PR #39235 introduced additional cleanup logic for form controls and directives. The cleanup logic relies
on the presence of ControlValueAccessor instances on FormControlName and FormControl directives. In general
these fields are present and there are also checks to make sure that the mentioned directive instances are
created with CVAs. However some scenarios (primarily tests) may invoke the logic in a way that the directive
instance would not be fully initialized, thus causing CVA to be absent. As a result, the cleanup logic fails
while trying to call some methods on associated CVA instances.

This commit updates the cleanup logic to take into account the situation when CVA is not present.

Fixes #40521.

PR Close #40526
@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 Feb 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@atscott atscott atscott approved these changes

@jelbourn jelbourn jelbourn approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime area: forms cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: minor This PR is targeted for the next minor release type: bug/fix
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@AndrewKushnir @kallebjork @maxime1992 @petebacondarwin @atscott @jelbourn