fix(forms): clean up connection between FormControl/FormGroup and corresponding directive instances by AndrewKushnir · Pull Request #39235 · angular/angular

AndrewKushnir · 2020-10-13T05:34:56Z

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

AndrewKushnir · 2020-10-15T01:42:33Z

Presubmit.

AndrewKushnir · 2020-11-10T07:02:17Z

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

AndrewKushnir · 2020-11-13T03:09:36Z

Presubmit #2 + Global Presubmit.

kallebjork · 2020-11-13T22:24:35Z

🥳 Kudos, Andrew!

AndrewKushnir · 2020-11-14T08:10:49Z

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.

…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 · 2021-01-04T06:32:55Z

Global Presubmit.

AndrewKushnir · 2021-01-05T17:31:46Z

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 · 2021-01-06T11:30:44Z

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

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.

…#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 · 2021-02-06T16:33:40Z

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

AndrewKushnir added type: bug/fix state: WIP area: forms labels Oct 13, 2020

ngbot Bot modified the milestone: needsTriage Oct 13, 2020

google-cla Bot added the cla: yes label Oct 13, 2020

AndrewKushnir mentioned this pull request Oct 13, 2020

fix(forms): clean up ControlValueAccessor connection when FormControlDirective is destroyed #37566

Closed

2 of 3 tasks

AndrewKushnir mentioned this pull request Nov 4, 2020

fix(forms): remove validators while cleaning up a control #39234

Closed

2 of 3 tasks

kallebjork mentioned this pull request Nov 4, 2020

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

Closed

crisbeto mentioned this pull request Nov 6, 2020

bug(MatDateRangePicker): Date range picker incorrectly has matStartDateInvalid error after hiding component angular/components#20980

Closed

blyndcide mentioned this pull request Nov 6, 2020

bug(forms): Date range picker incorrectly has matStartDateInvalid error after hiding component #39590

Closed

AndrewKushnir force-pushed the forms_cva_cleanup branch from d559073 to a199a8f Compare November 6, 2020 18:12

This was referenced Nov 6, 2020

Form control directive clean up #29335

Closed

Added the method addValidators and addAsyncValidators #37263

Closed

AndrewKushnir mentioned this pull request Nov 12, 2020

fix(forms): more precise control cleanup #39623

Closed

2 of 3 tasks

AndrewKushnir force-pushed the forms_cva_cleanup branch 4 times, most recently from 07def58 to 90fe4e5 Compare November 13, 2020 02:54

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 added the target: patch This PR is targeted for the next patch release label Nov 13, 2020

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

AndrewKushnir force-pushed the forms_cva_cleanup branch from c0b1870 to e5a2d31 Compare November 14, 2020 00:34

pullapprove Bot removed the area: forms label Dec 9, 2020

ngbot Bot removed this from the Backlog milestone Dec 9, 2020

pullapprove Bot added area: core Issues related to the framework runtime comp: docs/api area: forms labels Dec 10, 2020

ngbot Bot added this to the Backlog milestone Dec 10, 2020

This was referenced Dec 16, 2020

Directives don't clear provided NG_VALIDATORS when destroyed #34538

Closed

Angular Reactive forms custom form controls inconsistency in validations after conditionally recreated by angular #26792

Closed

AndrewKushnir force-pushed the forms_cva_cleanup branch from 5152d0f to 1e746f1 Compare January 4, 2021 06:12

josephperrott closed this in a384961 Jan 5, 2021

AndrewKushnir mentioned this pull request Jan 6, 2021

AbstractControl maintains validator after corresponding element is removed from view #14955

Closed

SebastianStehle mentioned this pull request Jan 12, 2021

FormControlDirective never unsubscribes. #22294

Closed

AndrewKushnir mentioned this pull request Jan 12, 2021

FormControls don't fully unbind from dom when swapped out #23473

Closed

This was referenced Jan 19, 2021

component implements ControlValueAccessor still connected to ngForm after destroy #27803

Closed

Multiple form controls are bound to the same input element, when I use ngIf directive #26890

Closed

This was referenced Jan 22, 2021

fix(forms): properly cleanup in cases when FormControlName has no CVA #40526

Closed

regression "Cannot read property 'registerOnChange' of null at cleanUpControl" / "Error during cleanup of component" #40521

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Feb 6, 2021

Navigation Menu

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

fix(forms): clean up connection between FormControl/FormGroup and corresponding directive instances#39235
AndrewKushnir wants to merge 1 commit into
angular:masterfrom
AndrewKushnir:forms_cva_cleanup

AndrewKushnir commented Oct 13, 2020 •

edited

Loading

Uh oh!

AndrewKushnir commented Oct 15, 2020

Uh oh!

AndrewKushnir commented Nov 10, 2020

Uh oh!

AndrewKushnir commented Nov 13, 2020 •

edited

Loading

Uh oh!

kallebjork commented Nov 13, 2020

Uh oh!

AndrewKushnir commented Nov 14, 2020

Uh oh!

AndrewKushnir commented Jan 4, 2021

Uh oh!

AndrewKushnir commented Jan 5, 2021

Uh oh!

maxime1992 commented Jan 6, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Feb 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

AndrewKushnir commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Type

Does this PR introduce a breaking change?

Uh oh!

AndrewKushnir commented Oct 15, 2020

Uh oh!

AndrewKushnir commented Nov 10, 2020

Uh oh!

AndrewKushnir commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallebjork commented Nov 13, 2020

Uh oh!

AndrewKushnir commented Nov 14, 2020

Uh oh!

AndrewKushnir commented Jan 4, 2021

Uh oh!

AndrewKushnir commented Jan 5, 2021

Uh oh!

maxime1992 commented Jan 6, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Feb 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AndrewKushnir commented Oct 13, 2020 •

edited

Loading

AndrewKushnir commented Nov 13, 2020 •

edited

Loading