From 1d1304c70c2f968068f15bd156b611e6e88b7e4a Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@abv.bg>
Date: Sun, 17 Jan 2021 12:07:26 +0100
Subject: [PATCH] fix(forms): error if control is removed as a result of
 another one being reset (#40462)

When a form is reset, it goes through `_forEachChild` to call `reset` on each of its children.
The problem is that if a control is removed while the loop is running (e.g. by a subscription),
the form will throw an error, because it built up the list of available control before the loop
started.

These changes fix the issue by adding a null check before invoing the callback.

Fixes #33401.

PR Close #40462
---
 packages/forms/src/model.ts                   |  8 +++-
 .../forms/test/reactive_integration_spec.ts   | 48 +++++++++++++++++--
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/packages/forms/src/model.ts b/packages/forms/src/model.ts
index 7fc2bb9ea4..d6392dcb94 100644
--- a/packages/forms/src/model.ts
+++ b/packages/forms/src/model.ts
@@ -1690,7 +1690,13 @@ export class FormGroup extends AbstractControl {
 
   /** @internal */
   _forEachChild(cb: (v: any, k: string) => void): void {
-    Object.keys(this.controls).forEach(k => cb(this.controls[k], k));
+    Object.keys(this.controls).forEach(key => {
+      // The list of controls can change (for ex. controls might be removed) while the loop
+      // is running (as a result of invoking Forms API in `valueChanges` subscription), so we
+      // have to null check before invoking the callback.
+      const control = this.controls[key];
+      control && cb(control, key);
+    });
   }
 
   /** @internal */
diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts
index ceb6425060..379ef229b9 100644
--- a/packages/forms/test/reactive_integration_spec.ts
+++ b/packages/forms/test/reactive_integration_spec.ts
@@ -7,13 +7,13 @@
  */
 
 import {ɵgetDOM as getDOM} from '@angular/common';
-import {Component, Directive, forwardRef, Input, Type} from '@angular/core';
+import {Component, Directive, forwardRef, Input, OnDestroy, Type} from '@angular/core';
 import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing';
 import {expect} from '@angular/core/testing/src/testing_internal';
 import {AbstractControl, AsyncValidator, AsyncValidatorFn, COMPOSITION_BUFFER_MODE, ControlValueAccessor, DefaultValueAccessor, FormArray, FormControl, FormControlDirective, FormControlName, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, ReactiveFormsModule, Validator, Validators} from '@angular/forms';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {dispatchEvent, sortedClassList} from '@angular/platform-browser/testing/src/browser_util';
-import {merge, NEVER, of, timer} from 'rxjs';
+import {merge, NEVER, of, Subscription, timer} from 'rxjs';
 import {map, tap} from 'rxjs/operators';
 
 import {MyInput, MyInputForm} from './value_accessor_integration_spec';
@@ -1129,6 +1129,48 @@ const ValueAccessorB = createControlValueAccessor('[cva-b]');
           expect(control.dirty).toBe(false, 'Expected pending dirty value to reset.');
         });
 
+        it('should be able to remove a control as a result of another control being reset', () => {
+          @Component({
+            template: `
+              <form [formGroup]="form">
+                <input formControlName="name">
+                <input formControlName="surname">
+              </form>
+            `
+          })
+          class App implements OnDestroy {
+            private _subscription: Subscription;
+
+            form = new FormGroup({
+              name: new FormControl('Frodo'),
+              surname: new FormControl('Baggins'),
+            });
+
+            constructor() {
+              this._subscription = this.form.controls.name.valueChanges.subscribe(value => {
+                if (!value) {
+                  this.form.removeControl('surname');
+                }
+              });
+            }
+
+            ngOnDestroy() {
+              this._subscription.unsubscribe();
+            }
+          }
+
+          const fixture = initTest(App);
+          fixture.detectChanges();
+          expect(fixture.componentInstance.form.value).toEqual({name: 'Frodo', surname: 'Baggins'});
+
+          expect(() => {
+            fixture.componentInstance.form.reset();
+            fixture.detectChanges();
+          }).not.toThrow();
+
+          expect(fixture.componentInstance.form.value).toEqual({name: null});
+        });
+
         it('should not emit valueChanges or statusChanges until blur', () => {
           const fixture = initTest(FormControlComp);
           const control = new FormControl('', {validators: Validators.required, updateOn: 'blur'});
@@ -4392,4 +4434,4 @@ class MultipleFormControls {
 class NgForFormControlWithValidators {
   form = new FormGroup({login: new FormControl('a')});
   logins = ['a', 'b', 'c'];
-}
\ No newline at end of file
+}