🐞 bug report

Affected Package

Is this a regression?

Description

See this simple ng-run demo.

The code is basically just this:

@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:

<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.

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...

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...

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:

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:

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:

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