Skip to content

Potentiel leak: NativeScript views not cleaned up on removal #1738

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
m-abs opened this issue Feb 17, 2019 · 3 comments · Fixed by #1740
Closed

Potentiel leak: NativeScript views not cleaned up on removal #1738

m-abs opened this issue Feb 17, 2019 · 3 comments · Fixed by #1740

Comments

@m-abs
Copy link
Contributor

m-abs commented Feb 17, 2019

Environment
Provide version numbers for the following components (information can be retrieved by running tns info in your project folder or by inspecting the package.json of the project):

  • CLI: 5.2.0
  • Cross-platform modules:
  • Android Runtime: 5.2.0
  • iOS Runtime:5.2.0
  • Plugin(s):
  • NativeScript-Angular: ~7.2.1
  • Angular: ~7.2.0

Describe the bug

While working on #1728 I think I’ve found a couple of memory leaks in nativescript-angular.

ListViewComponent/TemplatedItemsComponent items.
In onItemLoading(args) https://github.com/NativeScript/nativescript-angular/blob/master/nativescript-angular/directives/templated-items-comp.ts#L141-L181
The viewRef is bound to the args.view[NG_VIEW].
This is never cleared, so when the ListViewComponent is destroyed this reference still exists.
I suggest making this a WeakRef.

The factory functions in TemplatedItemsComponent._templateMap creates a circular reference and should be cleared in ngOnDestroy()

NativeScript views lives on after the angular app have been destroyed.
The native views are destroyed as expected but according to the Chrome debugger the NativeScript views lives on.
This is caused by the AppHostView not being cleared up on exit and the firstChild, lastChild and nextSibling references are not cleaned up.
The AppHostView is reused on next launch, so it shouldn’t itself be deleted but the children should.
Sidenote: AppHostView._ngAppRoot is a reference to the last Frame even after exit, this should be solved by deleting all its children on exit.

ViewUtils.removeChild(parent, child) doesn’t remove children recursively.
ViewUtils sets up the firstChild, lastChild and nextSibling references when a view is added, but doesn’t clear them up properly when a parent view is removes.

I suggest making removeFromQueue(parent, child) and removeFromVisualTree(parent, child) recursive to clear up the references.

PageRouterOutlet doesn’t clear up it’s children.
this.activated should be destroyed in ngOnDestroy()

NSLocationStrategy don’t have an ngOnDestroy()
This leaves correntOutlet with a reference to the destroyed PageRouterOutlet.

To Reproduce

  • Create a new nativescript angular app with: tns create --ng NAME
  • Launch the app on android in debug mode and start the Chrome debugger.
  • Close the app by tappen the back button, reopen and close the app a few times.
  • Go to the memory tab in the Chrome debugger and take a heap snapshot.
  • The snapshot will have N (= number of times the app has been opened) Frame'/ActionBar/Page/PageRouterOutlet, N*8 StackLayout` etc. The native views will have been destroyed.
    • If you inspect the Views a little closer you should see they hold references to each other via firstChild, lastChild and nextSibling

Note: It doesn't seem to matter how many times I call GC() or how long I wait.

Expected behavior

  • I expect the GC() to have collected the NativeScript Views.

Sample project

https://github.com/m-abs/tns-ng-app-lifecycle

Additional context

@m-abs
Copy link
Contributor Author

m-abs commented Feb 17, 2019

I've been playing around trying to clear some of this up in this branch:
https://github.com/m-abs/nativescript-angular/tree/fix/view-utils-hieraki-not-cleaned-up

But the changes should be cleared up before I make a PR and I could use some input :)

@m-abs
Copy link
Contributor Author

m-abs commented Feb 17, 2019

I think, I found a better example:
https://github.com/m-abs/tns-ng-leak

If you run this app in debug mode on android and tap "Start flipping listviews" it will flip between two listviews.

The memory usage will keep increasing and the number of StackLayouts in the heap snapshot will keep raising.
At flip no. 300 I've ~6750 StackLayouts. Using 73.5 MB memory
At flip no. 400 I've ~8820 StackLayouts. Using 84.4 MB memory
At flip no. 500 I've ~10180 StackLayouts. Using 97 MB memory
At flip no. 621 I've ~13120 StackLayouts. Using 112 MB memory
....
At flip no. 1100 I got the first lowMemoryEvent.
At flip no. 1300 the app crashed.

If I run the same test with my changes from: https://github.com/m-abs/nativescript-angular/tree/fix/view-utils-hieraki-not-cleaned-up
Number of StackLayouts are more stable.

@m-abs m-abs changed the title Potentiel leak: NativeScript views not cleaned up on exit Potentiel leak: NativeScript views not cleaned up on removal Feb 17, 2019
@ghost ghost added the ♥ community PR label Feb 18, 2019
@m-abs
Copy link
Contributor Author

m-abs commented Feb 18, 2019

Doesn't look like NG_VIEW reference is a problem if the views are removed as expected.

However I get a lot of let over Labels as described here: NativeScript/NativeScript#6913

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant