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

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 #20007, #37431, #39590.

PR Close #39235

goldens/public-api/forms/forms.d.ts

@@ -237,7 +237,7 @@ export declare class FormControl extends AbstractControl {
     }): void;
 }
 
-export declare class FormControlDirective extends NgControl implements OnChanges {
+export declare class FormControlDirective extends NgControl implements OnChanges, OnDestroy {
     get control(): FormControl;
     form: FormControl;
     set isDisabled(isDisabled: boolean);
@@ -247,6 +247,7 @@ export declare class FormControlDirective extends NgControl implements OnChanges
     viewModel: any;
     constructor(validators: (Validator | ValidatorFn)[], asyncValidators: (AsyncValidator | AsyncValidatorFn)[], valueAccessors: ControlValueAccessor[], _ngModelWarningConfig: string | null);
     ngOnChanges(changes: SimpleChanges): void;
+    ngOnDestroy(): void;
     viewToModelUpdate(newValue: any): void;
 }
 
@@ -295,7 +296,7 @@ export declare class FormGroup extends AbstractControl {
     }): void;
 }
 
-export declare class FormGroupDirective extends ControlContainer implements Form, OnChanges {
+export declare class FormGroupDirective extends ControlContainer implements Form, OnChanges, OnDestroy {
     get control(): FormGroup;
     directives: FormControlName[];
     form: FormGroup;
@@ -311,6 +312,7 @@ export declare class FormGroupDirective extends ControlContainer implements Form
     getFormArray(dir: FormArrayName): FormArray;
     getFormGroup(dir: FormGroupName): FormGroup;
     ngOnChanges(changes: SimpleChanges): void;
+    ngOnDestroy(): void;
     onReset(): void;
     onSubmit($event: Event): boolean;
     removeControl(dir: FormControlName): void;

packages/core/test/bundling/forms/bundle.golden_symbols.json

@@ -770,6 +770,9 @@
   {
     "name": "classIndexOf"
   },
+  {
+    "name": "cleanUpControl"
+  },
   {
     "name": "cleanUpValidators"
   },

packages/forms/src/directives/reactive_directives/form_control_directive.ts

@@ -6,14 +6,14 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Directive, EventEmitter, forwardRef, Inject, InjectionToken, Input, OnChanges, Optional, Output, Self, SimpleChanges} from '@angular/core';
+import {Directive, EventEmitter, forwardRef, Inject, InjectionToken, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges} from '@angular/core';
 
 import {FormControl} from '../../model';
 import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators';
 import {ControlValueAccessor, NG_VALUE_ACCESSOR} from '../control_value_accessor';
 import {NgControl} from '../ng_control';
 import {ReactiveErrors} from '../reactive_errors';
-import {_ngModelWarning, isPropertyUpdated, selectValueAccessor, setUpControl} from '../shared';
+import {_ngModelWarning, cleanUpControl, isPropertyUpdated, selectValueAccessor, setUpControl} from '../shared';
 import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';
 
 
@@ -51,7 +51,7 @@ export const formControlBinding: any = {
  * @publicApi
  */
 @Directive({selector: '[formControl]', providers: [formControlBinding], exportAs: 'ngForm'})
-export class FormControlDirective extends NgControl implements OnChanges {
+export class FormControlDirective extends NgControl implements OnChanges, OnDestroy {
   /**
    * Internal reference to the view model value.
    * @nodoc
@@ -118,6 +118,10 @@ export class FormControlDirective extends NgControl implements OnChanges {
   /** @nodoc */
   ngOnChanges(changes: SimpleChanges): void {
     if (this._isControlChanged(changes)) {
+      const previousForm = changes['form'].previousValue;
+      if (previousForm) {
+        cleanUpControl(previousForm, this, /* validateControlPresenceOnChange */ false);
+      }
       setUpControl(this.form, this);
       if (this.control.disabled && this.valueAccessor!.setDisabledState) {
         this.valueAccessor!.setDisabledState!(true);
@@ -133,6 +137,13 @@ export class FormControlDirective extends NgControl implements OnChanges {
     }
   }
 
+  /** @nodoc */
+  ngOnDestroy() {
+    if (this.form) {
+      cleanUpControl(this.form, this, /* validateControlPresenceOnChange */ false);
+    }
+  }
+
   /**
    * @description
    * Returns an array that represents the path from the top-level form to this control.

packages/forms/src/directives/reactive_directives/form_group_directive.ts

@@ -6,14 +6,14 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {Directive, EventEmitter, forwardRef, Inject, Input, OnChanges, Optional, Output, Self, SimpleChanges} from '@angular/core';
+import {Directive, EventEmitter, forwardRef, Inject, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges} from '@angular/core';
 
 import {FormArray, FormControl, FormGroup} from '../../model';
 import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../../validators';
 import {ControlContainer} from '../control_container';
 import {Form} from '../form_interface';
 import {ReactiveErrors} from '../reactive_errors';
-import {cleanUpControl, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
+import {cleanUpControl, cleanUpFormContainer, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
 import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';
 
 import {FormControlName} from './form_control_name';
@@ -53,7 +53,7 @@ export const formDirectiveProvider: any = {
   host: {'(submit)': 'onSubmit($event)', '(reset)': 'onReset()'},
   exportAs: 'ngForm'
 })
-export class FormGroupDirective extends ControlContainer implements Form, OnChanges {
+export class FormGroupDirective extends ControlContainer implements Form, OnChanges, OnDestroy {
   /**
    * @description
    * Reports whether the form submission has been triggered.
@@ -66,6 +66,12 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
    */
   private _oldForm: FormGroup|undefined;
 
+  /**
+   * Callback that should be invoked when controls in FormGroup or FormArray collection change
+   * (added or removed). This callback triggers corresponding DOM updates.
+   */
+  private readonly _onCollectionChange = () => this._updateDomValue();
+
   /**
    * @description
    * Tracks the list of added `FormControlName` instances
@@ -104,6 +110,23 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
     }
   }
 
+  /** @nodoc */
+  ngOnDestroy() {
+    if (this.form) {
+      cleanUpValidators(this.form, this, /* handleOnValidatorChange */ false);
+
+      // Currently the `onCollectionChange` callback is rewritten each time the
+      // `_registerOnCollectionChange` function is invoked. The implication is that cleanup should
+      // happen *only* when the `onCollectionChange` callback was set by this directive instance.
+      // Otherwise it might cause overriding a callback of some other directive instances. We should
+      // consider updating this logic later to make it similar to how `onChange` callbacks are
+      // handled, see https://github.com/angular/angular/issues/39732 for additional info.
+      if (this.form._onCollectionChange === this._onCollectionChange) {
+        this.form._registerOnCollectionChange(() => {});
+      }
+    }
+  }
+
   /**
    * @description
    * Returns this directive's instance.
@@ -161,6 +184,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
    * @param dir The `FormControlName` directive instance.
    */
   removeControl(dir: FormControlName): void {
+    cleanUpControl(dir.control || null, dir, /* validateControlPresenceOnChange */ false);
     removeListItem(this.directives, dir);
   }
 
@@ -170,17 +194,18 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
    * @param dir The `FormGroupName` directive instance.
    */
   addFormGroup(dir: FormGroupName): void {
-    const ctrl: any = this.form.get(dir.path);
-    setUpFormContainer(ctrl, dir);
-    ctrl.updateValueAndValidity({emitEvent: false});
+    this._setUpFormContainer(dir);
   }
 
   /**
-   * No-op method to remove the form group.
+   * Performs the necessary cleanup when a `FormGroupName` directive instance is removed from the
+   * view.
    *
    * @param dir The `FormGroupName` directive instance.
    */
-  removeFormGroup(dir: FormGroupName): void {}
+  removeFormGroup(dir: FormGroupName): void {
+    this._cleanUpFormContainer(dir);
+  }
 
   /**
    * @description
@@ -193,22 +218,23 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
   }
 
   /**
-   * Adds a new `FormArrayName` directive instance to the form.
+   * Performs the necessary setup when a `FormArrayName` directive instance is added to the view.
    *
    * @param dir The `FormArrayName` directive instance.
    */
   addFormArray(dir: FormArrayName): void {
-    const ctrl: any = this.form.get(dir.path);
-    setUpFormContainer(ctrl, dir);
-    ctrl.updateValueAndValidity({emitEvent: false});
+    this._setUpFormContainer(dir);
   }
 
   /**
-   * No-op method to remove the form array.
+   * Performs the necessary cleanup when a `FormArrayName` directive instance is removed from the
+   * view.
    *
    * @param dir The `FormArrayName` directive instance.
    */
-  removeFormArray(dir: FormArrayName): void {}
+  removeFormArray(dir: FormArrayName): void {
+    this._cleanUpFormContainer(dir);
+  }
 
   /**
    * @description
@@ -281,8 +307,31 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
     this.form._updateTreeValidity({emitEvent: false});
   }
 
+  private _setUpFormContainer(dir: FormArrayName|FormGroupName): void {
+    const ctrl: any = this.form.get(dir.path);
+    setUpFormContainer(ctrl, dir);
+    // NOTE: this operation looks unnecessary in case no new validators were added in
+    // `setUpFormContainer` call. Consider updating this code to match the logic in
+    // `_cleanUpFormContainer` function.
+    ctrl.updateValueAndValidity({emitEvent: false});
+  }
+
+  private _cleanUpFormContainer(dir: FormArrayName|FormGroupName): void {
+    if (this.form) {
+      const ctrl: any = this.form.get(dir.path);
+      if (ctrl) {
+        const isControlUpdated = cleanUpFormContainer(ctrl, dir);
+        if (isControlUpdated) {
+          // Run validity check only in case a control was updated (i.e. view validators were
+          // removed) as removing view validators might cause validity to change.
+          ctrl.updateValueAndValidity({emitEvent: false});
+        }
+      }
+    }
+  }
+
   private _updateRegistrations() {
-    this.form._registerOnCollectionChange(() => this._updateDomValue());
+    this.form._registerOnCollectionChange(this._onCollectionChange);
     if (this._oldForm) {
       this._oldForm._registerOnCollectionChange(() => {});
     }

packages/forms/src/directives/shared.ts

@@ -30,6 +30,13 @@ export function controlPath(name: string|null, parent: ControlContainer): string
   return [...parent.path!, name!];
 }
 
+/**
+ * Links a Form control and a Form directive by setting up callbacks (such as `onChange`) on both
+ * instances. This function is typically invoked when form directive is being initialized.
+ *
+ * @param control Form control instance that should be linked.
+ * @param dir Directive that should be linked with a given control.
+ */
 export function setUpControl(control: FormControl, dir: NgControl): void {
   if (typeof ngDevMode === 'undefined' || ngDevMode) {
     if (!control) _throwError(dir, 'Cannot find control with');
@@ -48,9 +55,22 @@ export function setUpControl(control: FormControl, dir: NgControl): void {
   setUpDisabledChangeHandler(control, dir);
 }
 
-export function cleanUpControl(control: FormControl|null, dir: NgControl) {
+/**
+ * Reverts configuration performed by the `setUpControl` control function.
+ * Effectively disconnects form control with a given form directive.
+ * This function is typically invoked when corresponding form directive is being destroyed.
+ *
+ * @param control Form control which should be cleaned up.
+ * @param dir Directive that should be disconnected from a given control.
+ * @param validateControlPresenceOnChange Flag that indicates whether onChange handler should
+ *     contain asserts to verify that it's not called once directive is destroyed. We need this flag
+ *     to avoid potentially breaking changes caused by better control cleanup introduced in #39235.
+ */
+export function cleanUpControl(
+    control: FormControl|null, dir: NgControl,
+    validateControlPresenceOnChange: boolean = true): void {
   const noop = () => {
-    if (typeof ngDevMode === 'undefined' || ngDevMode) {
+    if (validateControlPresenceOnChange && (typeof ngDevMode === 'undefined' || ngDevMode)) {
       _noControlError(dir);
     }
   };
@@ -146,16 +166,22 @@ export function setUpValidators(
  * @param dir Directive instance that contains validators to be removed.
  * @param handleOnValidatorChange Flag that determines whether directive validators should also be
  *     cleaned up to stop handling validator input change (if previously configured to do so).
+ * @returns true if a control was updated as a result of this action.
  */
 export function cleanUpValidators(
     control: AbstractControl|null, dir: AbstractControlDirective,
-    handleOnValidatorChange: boolean): void {
+    handleOnValidatorChange: boolean): boolean {
+  let isControlUpdated = false;
   if (control !== null) {
     if (dir.validator !== null) {
       const validators = getControlValidators(control);
       if (Array.isArray(validators) && validators.length > 0) {
         // Filter out directive validator function.
-        control.setValidators(validators.filter(validator => validator !== dir.validator));
+        const updatedValidators = validators.filter(validator => validator !== dir.validator);
+        if (updatedValidators.length !== validators.length) {
+          isControlUpdated = true;
+          control.setValidators(updatedValidators);
+        }
       }
     }
 
@@ -163,8 +189,12 @@ export function cleanUpValidators(
       const asyncValidators = getControlAsyncValidators(control);
       if (Array.isArray(asyncValidators) && asyncValidators.length > 0) {
         // Filter out directive async validator function.
-        control.setAsyncValidators(
-            asyncValidators.filter(asyncValidator => asyncValidator !== dir.asyncValidator));
+        const updatedAsyncValidators =
+            asyncValidators.filter(asyncValidator => asyncValidator !== dir.asyncValidator);
+        if (updatedAsyncValidators.length !== asyncValidators.length) {
+          isControlUpdated = true;
+          control.setAsyncValidators(updatedAsyncValidators);
+        }
       }
     }
   }
@@ -175,6 +205,8 @@ export function cleanUpValidators(
     registerOnValidatorChange<ValidatorFn>(dir._rawValidators, noop);
     registerOnValidatorChange<AsyncValidatorFn>(dir._rawAsyncValidators, noop);
   }
+
+  return isControlUpdated;
 }
 
 function setUpViewChangePipeline(control: FormControl, dir: NgControl): void {
@@ -220,6 +252,13 @@ function setUpModelChangePipeline(control: FormControl, dir: NgControl): void {
   });
 }
 
+/**
+ * Links a FormGroup or FormArray instance and corresponding Form directive by setting up validators
+ * present in the view.
+ *
+ * @param control FormGroup or FormArray instance that should be linked.
+ * @param dir Directive that provides view validators.
+ */
 export function setUpFormContainer(
     control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName) {
   if (control == null && (typeof ngDevMode === 'undefined' || ngDevMode))
@@ -227,6 +266,18 @@ export function setUpFormContainer(
   setUpValidators(control, dir, /* handleOnValidatorChange */ false);
 }
 
+/**
+ * Reverts the setup performed by the `setUpFormContainer` function.
+ *
+ * @param control FormGroup or FormArray instance that should be cleaned up.
+ * @param dir Directive that provided view validators.
+ * @returns true if a control was updated as a result of this action.
+ */
+export function cleanUpFormContainer(
+    control: FormGroup|FormArray, dir: AbstractFormGroupDirective|FormArrayName): boolean {
+  return cleanUpValidators(control, dir, /* handleOnValidatorChange */ false);
+}
+
 function _noControlError(dir: NgControl) {
   return _throwError(dir, 'There is no FormControl instance attached to form control element with');
 }