From 1bc53eb303435e34d292ad4ea2e7c5bfccfa7527 Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Mon, 9 Nov 2020 22:58:19 -0800 Subject: [PATCH] fix(forms): more precise control cleanup (#39623) 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 --- goldens/circular-deps/packages.json | 126 ++++++++++-------- .../bundling/forms/bundle.golden_symbols.json | 4 +- .../directives/abstract_control_directive.ts | 24 ++++ packages/forms/src/directives/ng_form.ts | 4 +- .../form_group_directive.ts | 4 +- packages/forms/src/directives/shared.ts | 44 ++++-- packages/forms/src/model.ts | 16 ++- .../forms/test/reactive_integration_spec.ts | 64 ++++++++- 8 files changed, 209 insertions(+), 77 deletions(-) diff --git a/goldens/circular-deps/packages.json b/goldens/circular-deps/packages.json index 2a042db5e6..8c26c0a489 100644 --- a/goldens/circular-deps/packages.json +++ b/goldens/circular-deps/packages.json @@ -1002,6 +1002,27 @@ "packages/core/testing/src/r3_test_bed.ts", "packages/core/testing/src/test_bed.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_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/control_container.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/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/control_container.ts", @@ -1009,7 +1030,10 @@ ], [ "packages/forms/src/directives/abstract_form_group_directive.ts", - "packages/forms/src/directives/form_interface.ts" + "packages/forms/src/directives/control_container.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", @@ -1017,59 +1041,13 @@ ], [ "packages/forms/src/directives/abstract_form_group_directive.ts", - "packages/forms/src/directives/shared.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/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/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/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/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/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", @@ -1087,6 +1065,14 @@ "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" @@ -1102,6 +1088,13 @@ "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" + ], [ "packages/forms/src/directives/reactive_directives/form_group_directive.ts", "packages/forms/src/directives/reactive_directives/form_group_name.ts" @@ -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/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/model.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/forms/src/directives/validators.ts", - "packages/forms/src/validators.ts", - "packages/forms/src/model.ts" - ], [ "packages/language-service/src/completions.ts", "packages/language-service/src/template.ts", diff --git a/packages/core/test/bundling/forms/bundle.golden_symbols.json b/packages/core/test/bundling/forms/bundle.golden_symbols.json index 4e692ada58..89c55a2bcc 100644 --- a/packages/core/test/bundling/forms/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms/bundle.golden_symbols.json @@ -1455,10 +1455,10 @@ "name": "remove" }, { - "name": "removeDir" + "name": "removeFromArray" }, { - "name": "removeFromArray" + "name": "removeListItem" }, { "name": "renderComponent" diff --git a/packages/forms/src/directives/abstract_control_directive.ts b/packages/forms/src/directives/abstract_control_directive.ts index e8f07b55ad..e1fb2c6e02 100644 --- a/packages/forms/src/directives/abstract_control_directive.ts +++ b/packages/forms/src/directives/abstract_control_directive.ts @@ -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. diff --git a/packages/forms/src/directives/ng_form.ts b/packages/forms/src/directives/ng_form.ts index ffb1593250..b91cf56786 100644 --- a/packages/forms/src/directives/ng_form.ts +++ b/packages/forms/src/directives/ng_form.ts @@ -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 = { @@ -217,7 +217,7 @@ export class NgForm extends ControlContainer implements Form, AfterViewInit { if (container) { container.removeControl(dir.name); } - removeDir(this._directives, dir); + removeListItem(this._directives, dir); }); } diff --git a/packages/forms/src/directives/reactive_directives/form_group_directive.ts b/packages/forms/src/directives/reactive_directives/form_group_directive.ts index 050f244a76..b6a8f8f148 100644 --- a/packages/forms/src/directives/reactive_directives/form_group_directive.ts +++ b/packages/forms/src/directives/reactive_directives/form_group_directive.ts @@ -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'; @@ -161,7 +161,7 @@ export class FormGroupDirective extends ControlContainer implements Form, OnChan * @param dir The `FormControlName` directive instance. */ removeControl(dir: FormControlName): void { - removeDir(this.directives, dir); + removeListItem(this.directives, dir); } /** diff --git a/packages/forms/src/directives/shared.ts b/packages/forms/src/directives/shared.ts index 9b7ccf545e..3a69960447 100644 --- a/packages/forms/src/directives/shared.ts +++ b/packages/forms/src/directives/shared.ts @@ -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) { @@ -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(validators: (V|Validator)[], onChange: () => void): void { @@ -76,6 +75,28 @@ function registerOnValidatorChange(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. @@ -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); }); } @@ -287,7 +315,7 @@ export function selectValueAccessor( return null; } -export function removeDir(list: T[], el: T): void { +export function removeListItem(list: T[], el: T): void { const index = list.indexOf(el); if (index > -1) list.splice(index, 1); } diff --git a/packages/forms/src/model.ts b/packages/forms/src/model.ts index 40d45d39a6..a4cc280c2f 100644 --- a/packages/forms/src/model.ts +++ b/packages/forms/src/model.ts @@ -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'; @@ -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); } /** @@ -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 */ diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 170ed7bb6c..389042c4ba 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/packages/forms/test/reactive_integration_spec.ts @@ -2594,7 +2594,7 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec'; expectValidatorsToBeCalled(newValidatorSpy, newAsyncValidatorSpy, {ctx: control, count: 1}); }); - it('should cleanup validators on a control used for multiple `formControlName` directive', + it('should cleanup validators on a control used for multiple `formControlName` directives', () => { const fixture = initTest(NgForFormControlWithValidators, MyCustomValidator, MyCustomAsyncValidator); @@ -2643,6 +2643,40 @@ import {MyInput, MyInputForm} from './value_accessor_integration_spec'; // of controls (one for each element in the `logins` array). expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: newControl, count: 6}); }); + + it('should cleanup directive-specific callbacks only', () => { + const fixture = initTest(MultipleFormControls, MyCustomValidator, MyCustomAsyncValidator); + fixture.detectChanges(); + + const sharedControl = fixture.componentInstance.control; + + const validatorSpy = validatorSpyOn(MyCustomValidator); + const asyncValidatorSpy = validatorSpyOn(MyCustomAsyncValidator); + + sharedControl.setValue('b'); + fixture.detectChanges(); + + // Check that validators were called for each `formControlName` directive instance + // (2 times total). + expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: sharedControl, count: 2}); + + // Replace formA with a new instance. This will trigger destroy operation for the + // `formControlName` directive that is bound to the `control` FormControl instance. + const newFormA = new FormGroup({login: new FormControl('new-a')}); + fixture.componentInstance.formA = newFormA; + fixture.detectChanges(); + + validatorSpy.calls.reset(); + asyncValidatorSpy.calls.reset(); + + // Update control with a new value. + sharedControl.setValue('d'); + fixture.detectChanges(); + + // We should still see an update to the second . + expect(fixture.nativeElement.querySelector('#login').value).toBe('d'); + expectValidatorsToBeCalled(validatorSpy, asyncValidatorSpy, {ctx: sharedControl, count: 1}); + }); }); }); } @@ -2920,6 +2954,34 @@ class FormControlWithValidators { form = new FormGroup({login: new FormControl('INITIAL')}); } +@Component({ + selector: 'ngfor-form-controls-with-validators', + template: ` +
+ +
+
+ +
+ ` +}) +class MultipleFormControls { + control = new FormControl('a'); + formA = new FormGroup({login: this.control}); + formB = new FormGroup({login: this.control}); +} + @Component({ selector: 'ngfor-form-controls-with-validators', template: `