Skip to content

Commit

Permalink
fix(forms): more precise control cleanup (#39623)
Browse files Browse the repository at this point in the history 
Currently when an instance of the `FormControlName` directive is destroyed, the Forms package invokes
the `cleanUpControl` to clear all directive-specific logic (such as validators, onChange handlers,
etc) from a bound control. The logic of the `cleanUpControl` function should revert all setup
performed by the `setUpControl` function. However the `cleanUpControl` is too aggressive and removes
all callbacks related to the onChange and disabled state handling. This is causing problems when
a form control is bound to multiple FormControlName` directives, causing other instances of that
directive to stop working correctly when the first one is destroyed.

This commit updates the cleanup logic to only remove callbacks added while setting up a control
for a given directive instance.

The fix is needed to allow adding `cleanUpControl` function to other places where cleanup is needed
(missing this function calls in some other places causes memory leak issues).

PR Close #39623
  • Loading branch information
@AndrewKushnir @atscott
AndrewKushnir authored and atscott committed Nov 12, 2020
1 parent 978f081 commit 050cea9
Show file tree
Hide file tree
Showing 8 changed files with 195 additions and 63 deletions.
98 changes: 54 additions & 44 deletions goldens/circular-deps/packages.json
View file
Expand Up @@ -1003,73 +1003,51 @@
"packages/core/testing/src/test_bed.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/ng_control.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/control_container.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/abstract_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/directives/ng_control.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/abstract_form_group_directive.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/control_container.ts",
"packages/forms/src/directives/form_interface.ts"
"packages/forms/src/directives/form_interface.ts",
"packages/forms/src/directives/ng_control.ts"
],
[
"packages/forms/src/directives/ng_form.ts",
Expand All @@ -1087,17 +1065,32 @@
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/directives/reactive_directives/form_control_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_control_name.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/reactive_directives/form_group_directive.ts"
Expand All @@ -1111,23 +1104,40 @@
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_directive.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts"
],
[
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/validators.ts",
"packages/forms/src/directives/reactive_directives/form_group_name.ts",
"packages/forms/src/model.ts",
"packages/forms/src/directives/shared.ts"
],
[
"packages/forms/src/directives/shared.ts",
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/shared.ts",
"packages/forms/src/validators.ts",
"packages/forms/src/directives/validators.ts",
"packages/forms/src/validators.ts"
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/validators.ts",
"packages/forms/src/directives/shared.ts",
"packages/forms/src/validators.ts",
"packages/forms/src/model.ts"
],
[
"packages/forms/src/directives/validators.ts",
"packages/forms/src/validators.ts"
],
[
"packages/language-service/src/completions.ts",
"packages/language-service/src/template.ts",
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/bundling/forms/bundle.golden_symbols.json
View file
Expand Up @@ -1455,10 +1455,10 @@
"name": "remove"
},
{
"name": "removeDir"
"name": "removeFromArray"
},
{
"name": "removeFromArray"
"name": "removeListItem"
},
{
"name": "renderComponent"
Expand Down
24 changes: 24 additions & 0 deletions packages/forms/src/directives/abstract_control_directive.ts
View file
Expand Up @@ -230,6 +230,30 @@ export abstract class AbstractControlDirective {
return this._composedAsyncValidatorFn || null;
}

/*
* The set of callbacks to be invoked when directive instance is being destroyed.
*/
private _onDestroyCallbacks: (() => void)[] = [];

/**
* Internal function to register callbacks that should be invoked
* when directive instance is being destroyed.
* @internal
*/
_registerOnDestroy(fn: () => void): void {
this._onDestroyCallbacks.push(fn);
}

/**
* Internal function to invoke all registered "on destroy" callbacks.
* Note: calling this function also clears the list of callbacks.
* @internal
*/
_invokeOnDestroyCallbacks(): void {
this._onDestroyCallbacks.forEach(fn => fn());
this._onDestroyCallbacks = [];
}

/**
* @description
* Resets the control with the provided value if the control is present.
Expand Down
4 changes: 2 additions & 2 deletions packages/forms/src/directives/ng_form.ts
View file
Expand Up @@ -16,7 +16,7 @@ import {Form} from './form_interface';
import {NgControl} from './ng_control';
import {NgModel} from './ng_model';
import {NgModelGroup} from './ng_model_group';
import {removeDir, setUpControl, setUpFormContainer, syncPendingControls} from './shared';
import {removeListItem, setUpControl, setUpFormContainer, syncPendingControls} from './shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from './validators';

export const formDirectiveProvider: any = {
Expand Down Expand Up @@ -217,7 +217,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit {
if (container) {
container.removeControl(dir.name);
}
removeDir<NgModel>(this._directives, dir);
removeListItem(this._directives, dir);
});
}

Expand Down
4 changes: 2 additions & 2 deletions packages/forms/src/directives/reactive_directives/form_group_directive.ts
View file
Expand Up @@ -13,7 +13,7 @@ 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, removeDir, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {cleanUpControl, cleanUpValidators, removeListItem, setUpControl, setUpFormContainer, setUpValidators, syncPendingControls} from '../shared';
import {AsyncValidator, AsyncValidatorFn, Validator, ValidatorFn} from '../validators';

import {FormControlName} from './form_control_name';
Expand Down Expand Up @@ -161,7 +161,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan
* @param dir The `FormControlName` directive instance.
*/
removeControl(dir: FormControlName): void {
removeDir<FormControlName>(this.directives, dir);
removeListItem(this.directives, dir);
}

/**
Expand Down
44 changes: 36 additions & 8 deletions packages/forms/src/directives/shared.ts
View file
Expand Up @@ -47,11 +47,7 @@ export function setUpControl(control: FormControl, dir: NgControl): void {

setUpBlurPipeline(control, dir);

if (dir.valueAccessor!.setDisabledState) {
control.registerOnDisabledChange((isDisabled: boolean) => {
dir.valueAccessor!.setDisabledState!(isDisabled);
});
}
setUpDisabledChangeHandler(control, dir);
}

export function cleanUpControl(control: FormControl|null, dir: NgControl) {
Expand All @@ -66,7 +62,10 @@ export function cleanUpControl(control: FormControl|null, dir: NgControl) {

cleanUpValidators(control, dir, /* handleOnValidatorChange */ true);

if (control) control._clearChangeFns();
if (control) {
dir._invokeOnDestroyCallbacks();
control._registerOnCollectionChange(() => {});
}
}

function registerOnValidatorChange<V>(validators: (V|Validator)[], onChange: () => void): void {
Expand All @@ -76,6 +75,28 @@ function registerOnValidatorChange<V>(validators: (V|Validator)[], onChange: ()
});
}

/**
* Sets up disabled change handler function on a given form control if ControlValueAccessor
* associated with a given directive instance supports the `setDisabledState` call.
*
* @param control Form control where disabled change handler should be setup.
* @param dir Corresponding directive instance associated with this control.
*/
export function setUpDisabledChangeHandler(control: FormControl, dir: NgControl): void {
if (dir.valueAccessor!.setDisabledState) {
const onDisabledChange = (isDisabled: boolean) => {
dir.valueAccessor!.setDisabledState!(isDisabled);
};
control.registerOnDisabledChange(onDisabledChange);

// Register a callback function to cleanup disabled change handler
// from a control instance when a directive is destroyed.
dir._registerOnDestroy(() => {
control._unregisterOnDisabledChange(onDisabledChange);
});
}
}

/**
* Sets up sync and async directive validators on provided form control.
* This function merges validators from the directive into the validators of the control.
Expand Down Expand Up @@ -185,12 +206,19 @@ function updateControl(control: FormControl, dir: NgControl): void {
}

function setUpModelChangePipeline(control: FormControl, dir: NgControl): void {
control.registerOnChange((newValue: any, emitModelEvent: boolean) => {
const onChange = (newValue: any, emitModelEvent: boolean) => {
// control -> view
dir.valueAccessor!.writeValue(newValue);

// control -> ngModel
if (emitModelEvent) dir.viewToModelUpdate(newValue);
};
control.registerOnChange(onChange);

// Register a callback function to cleanup onChange handler
// from a control instance when a directive is destroyed.
dir._registerOnDestroy(() => {
control._unregisterOnChange(onChange);
});
}

Expand Down Expand Up @@ -287,7 +315,7 @@ export function selectValueAccessor(
return null;
}

export function removeDir<T>(list: T[], el: T): void {
export function removeListItem<T>(list: T[], el: T): void {
const index = list.indexOf(el);
if (index > -1) list.splice(index, 1);
}
Expand Down
16 changes: 12 additions & 4 deletions packages/forms/src/model.ts
View file
Expand Up @@ -9,6 +9,7 @@
import {EventEmitter} from '@angular/core';
import {Observable} from 'rxjs';

import {removeListItem} from './directives/shared';
import {AsyncValidatorFn, ValidationErrors, ValidatorFn} from './directives/validators';
import {composeAsyncValidators, composeValidators, toObservable} from './validators';

Expand Down Expand Up @@ -1269,12 +1270,11 @@ export class FormControl extends AbstractControl {
}

/**
* Internal function to unregister a change events listener.
* @internal
*/
_clearChangeFns(): void {
this._onChange = [];
this._onDisabledChange = [];
this._onCollectionChange = () => {};
_unregisterOnChange(fn: Function): void {
removeListItem(this._onChange, fn);
}

/**
Expand All @@ -1286,6 +1286,14 @@ export class FormControl extends AbstractControl {
this._onDisabledChange.push(fn);
}

/**
* Internal function to unregister a disabled event listener.
* @internal
*/
_unregisterOnDisabledChange(fn: (isDisabled: boolean) => void): void {
removeListItem(this._onDisabledChange, fn);
}

/**
* @internal
*/
Expand Down

0 comments on commit 050cea9

Please sign in to comment.