Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ngRepeat "track by" possible memory leak #16776

Closed
1 of 3 tasks
bensgroi opened this issue Nov 28, 2018 · 7 comments · Fixed by #16777
Closed
1 of 3 tasks

ngRepeat "track by" possible memory leak #16776

bensgroi opened this issue Nov 28, 2018 · 7 comments · Fixed by #16777

Comments

@bensgroi
Copy link

bensgroi commented Nov 28, 2018

I'm submitting a ...

  • bug report
  • feature request
  • other

Current behavior:

Adding a track by to an ngRepeat causes a memory leak, so that the component's controller is retained in memory.

Expected / new behavior:

Using track by on an ngRepeat should not cause any side effects.

Minimal reproduction of the problem with instructions:

Plunker
To see the memory leak in action, you'll need to run the demo in its own tab: https://run.plnkr.co/plunks/pToAURpUzO2pdOq6H0Yf/

It contains two child components, a "leaky" version and a "non-leaky" version, the only difference between them being the addition of a "track by" on the leaky one.

  1. Click on "Leaky Component", then "Non-Leaky Component", then "Reset"
  2. Take a heap snapshot. Filter on "leaky". You'll see that the "leaky" component is still in memory, though not reachable. The "non-leaky" component has been GC'ed and is no longer on the heap.
    image

The presence of "track by" seems to cause a leak. Using track by $index has the same effect. This seems like a really obvious bug, but I haven't been able to find anything related to it here yet. Am I missing something?

AngularJS version: 1.7.5

Also reproduced in 1.5.8

Browser: Chrome 70

@gkalpak
Copy link
Member

gkalpak commented Nov 28, 2018

Thx for reporting this. Have you manually triggered GC before taking the heap snapshot? If the element is not reachable (as you said), then it would be a GC bug not to get rid of it.

@bensgroi
Copy link
Author

@gkalpak Taking a heap snapshot automatically runs GC beforehand.

@bensgroi
Copy link
Author

I updated the Plunker and repro steps to make it a bit clearer.

@petebacondarwin
Copy link
Member

I might not be reading this right but as far as I can tell only one instance is ever created, even if you toggle the ng-if many times. I appreciate that this doesn't mean there is no memory leak but it does give us some more information for debugging this...

@bensgroi
Copy link
Author

@petebacondarwin True, the scope of the leak is limited. But if the controller were to have enough data in it and/or have $scope injected (which could lead to the whole scope tree leaking), the impact could be noticeable on memory usage.

@jbedard
Copy link
Contributor

jbedard commented Nov 29, 2018

Good catch @bensgroi! Turns out there's actually 2 problems:

  1. The trackByIdExpFn references the scope (link level) but is cached at the compilation level, so the scope leaks like you pointed out. It will get cleaned along with the compilation data, but that is a little too late.
  2. The trackByIdExpFn is only created on the very first link of an ng-repeat, so if the repeat is repeated, all but the first will have the wrong $scope.

jbedard added a commit to jbedard/angular.js that referenced this issue Nov 29, 2018
This also fixes a leak of that scope across all further instances of the
repeated element.

Fixes angular#16776
jbedard added a commit to jbedard/angular.js that referenced this issue Nov 29, 2018
This also fixes a leak of that scope across all further instances of the
repeated element.

Fixes angular#16776
jbedard added a commit to jbedard/angular.js that referenced this issue Nov 29, 2018
This also fixes a leak of that scope across all further instances of the
repeated element.

Fixes angular#16776
Closes angular#16777
jbedard added a commit to jbedard/angular.js that referenced this issue Nov 30, 2018
Also fixes a leak of that scope across all further instances of the
repeated element.

Fixes angular#16776
Closes angular#16777
@Narretz Narretz added this to the 1.7.x milestone Dec 5, 2018
jbedard added a commit to jbedard/angular.js that referenced this issue Dec 6, 2018
Also fixes a leak of that scope across all further instances of the
repeated element.

Fixes angular#16776
Closes angular#16777
jbedard added a commit to jbedard/angular.js that referenced this issue Dec 6, 2018
@jbedard
Copy link
Contributor

jbedard commented Dec 6, 2018

For reference, another issue we found which will also be fixed:

  1. If track by is used then the last value from the collection is leaked to the compilation level, so if the element+scope gets removed the value is still referenced. Basically the same as the first issue but with only the last value leaked and not the full scope.

jbedard added a commit that referenced this issue Dec 6, 2018
Also fixes a leak of that scope across all further instances of the
repeated element.

Fixes #16776
Closes #16777
mgol pushed a commit that referenced this issue Dec 6, 2018
Also fixes a leak of that scope across all further instances of the
repeated element.

Fixes #16776
Closes #16777
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants