Repository: angular
Issue: NgFor, NgForOf (DefaultIterableDiffer + IterableChangeRecord) can leak memory
Labels: type: bug/fix,memory leak,area: core,state: confirmed,core: differs,P3

# 🐞 bug report

### Affected Package
<!-- Can you pin-point one or more @angular/* packages as the source of the bug? -->
<!-- ✍️edit: --> The issue is caused by package @angular/common


### Is this a regression?

<!-- Did this behavior use to work in the previous version? -->
<!-- ✍️--> No. I think this has been an issue for a long time.


### Description

See this simple [ng-run demo](https://ng-run.com/edit/RrVTKa66CWbkDTbi5ofB).

The code is basically just this:
```ts
@Component({
  //...
})
export class AppComponent {
  testClassInstances = [new TestClass(), new TestClass()];

  deleteLast(){
    this.testClassInstances.pop();
  }
}

var count = 0;
class TestClass {
  id = ++count;
}
```

and the HTML template being:
```html
<button (click)="deleteLast()">deleteLast</button>

<p *ngFor="let test of testClassInstances">
  {{test.id}}
</p>
```

So two `TestClass` instances are being created and clicking the "delete Last" button pops the last one off. Important to note is that once it is popped off, there are no more user variables holding a reference to this class instance, meaning it should be garbage collected.

Click the "delete Last" button once.
`ngFor` picks up the change and no longer displays the second `TestClass` id, as expected.

Now, take a memory heap snapshot of `ng-run-preview.firebaseapp.com` and search for `TestClass`.

![image](https://user-images.githubusercontent.com/23387905/80746519-03472600-8af0-11ea-8728-d8a6bfb568a2.png)

Two instances still in memory = memory leak = not good.
Looking at the object reference tree clearly indicates a reference to this object still being held in an `IterableChangeRecord` inside the `DefaultIterableDiffer`.

***Problem***

In the [NgForOf directive](https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_for_of.ts#L204)...

```ts
ngDoCheck(): void {
    //...
    if (this._differ) {
      const changes = this._differ.diff(this._ngForOf);
      if (changes) this._applyChanges(changes);
    }
  }
```

...changes are calculated and then "applied". Calling `this._differ.diff()` mutates the `this._differ` object with the changes. The problem is that these changes are not being cleared after they are calculated and used (applied).

There is a `_reset()` method (called basically immediately during `this._differ.diff()`) on the `DefaultIterableDiffer` class that is stated in comments to _"Reset the state of the change objects to show no changes"_...but this comment is somewhat misleading because it doesn't actually clear out all the changes.

Changing the above snippet to...

```ts
ngDoCheck(): void {
    //...
    if (this._differ) {
      const changes = this._differ.diff(this._ngForOf);
      if (changes) {
        this._applyChanges(changes);
        this._differ._reset(); // <--- NEW
      }
    }
  }
```
 
...and running the exact same test as above results in the following heap snapshot:

![image](https://user-images.githubusercontent.com/23387905/80746906-9c763c80-8af0-11ea-9e4f-dc1b512037f5.png)

Still a memory leak, and the object is being referenced in the `_nextAdded` property of the `IterableChangeRecord` item corresponding to the first `TestClass` instance.

In order to fix this memory leak, something like the following needs to be added:

```ts
ngDoCheck(): void {
    //...
    if (this._differ) {
      const changes = this._differ.diff(this._ngForOf);
      if (changes) {
        this._applyChanges(changes);
        this._differ._reset(); // <--- NEW
      }

      // <-- NEW -->
      this._differ.forEachItem((item: IterableChangeRecord<any>) => {
        item._nextAdded = null;
        item._nextMoved = null;
        item._nextIdentityChange = null;
      });
    }
  }
```

Resulting heap snapshot of the same test with these new changes:

![image](https://user-images.githubusercontent.com/23387905/80747065-d6474300-8af0-11ea-9de7-88581510373c.png)

One instance in memory, which is expected behavior.
These changes can be tested in the above linked demo. Just change `*ngFor` to `*ngForr` (two 'r') in the HTML template and reload the preview pane.

(Note: The code snippets above cause TypeScript compiler errors, but we're ignoring that)

***Solution***

I'm not intimately familiar with these data structures, but I feel like the following changes should occur:

1) `_reset()` should additionally ensure that every record item's `_nextAdded`, `_nextMoved`, `_nextIdentityChange`, etc. properties are set to null.
2) `_reset()` be made public (I guess part of the `IterableDiffer<T>` interface).
3) Any code interacting with IterableDiffers should call this reset method after diffing and inspecting/acting on the changes. Similar to the ` this._differ._reset(); // <--- NEW` line in the above snippets.

The example in this issue post specifically references the `NgFor` directive, but the same problem is also apparent in the `NgClass` directive (and maybe `NgStyle`? - I haven't looked into the `KeyValueDiffer` to see if a similar problem exists).

## 🌍  Your Environment

**Angular Version:**
9.1.4 and earlier