Skip to content
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

bug: Navigating between pages inside an Ionic app is leaking DOM nodes #19242

Closed
vially opened this issue Aug 31, 2019 · 10 comments
Closed

bug: Navigating between pages inside an Ionic app is leaking DOM nodes #19242

vially opened this issue Aug 31, 2019 · 10 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@vially
Copy link

vially commented Aug 31, 2019

Bug Report

Ionic version:

[x] 4.8.1

Current behavior:

Navigating between pages leaks DOM nodes.

Expected behavior:

Navigating between pages should not leak DOM nodes.

Steps to reproduce:

The easiest way to reproduce it is by repeatedly navigating between two pages while monitoring the DOM nodes count.

Simple demo app which easily reproduces the problem: Ionic navigation DOM node leaks.

Steps to reproduce inside the demo app:

  • open app in Chrome while keeping the devtools Performance monitor open
  • repeatedly perform the following steps:
    1. click the "navigation leak" link
    2. use the browser back button to navigate back to the homepage
    3. optional: press the Collect garbage button in Chrome devtools (found inside the Performance tab)

Notice how each iteration of opening and closing the demo page increases the number of DOM nodes reported by the Performance monitor:

ionic_navigation_dom_leaks

The staircase pattern seems to suggest there's a memory leak that happens while navigating between pages in an Ionic app.

Related code:

Ionic navigation DOM node leaks demo

Other information:

The content of the page linked to seems to affect the number of DOM nodes leaked. For example, if the linked page contains <ion-content>Hello world</ion-content> then the number of leaked DOM nodes after each iteration is 7. However, if the page contains a div instead of an ion-content (e.g.: <div>Hello world</div>) then the number of leaked DOM nodes decreases to 3.

Ionic info:

Ionic:

   Ionic CLI                     : 5.2.7 (/home/vially/.config/yarn/global/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.8.1
   @angular-devkit/build-angular : 0.801.3
   @angular-devkit/schematics    : 8.1.3
   @angular/cli                  : 8.1.3
   @ionic/angular-toolkit        : 2.0.0

Utility:

   cordova-res : not installed
   native-run  : not installed

System:

   NodeJS : v12.9.1 (/usr/bin/node)
   npm    : 6.11.2
   OS     : Linux 5.2
@ionitron-bot ionitron-bot bot added the triage label Aug 31, 2019
@vially
Copy link
Author

vially commented Aug 31, 2019

it looks like some event listeners are also being leaked:
ionic_navigation_js_listeners_leak

This seems to suggest there are some missing removeEventListener calls.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels Sep 3, 2019
@ionitron-bot ionitron-bot bot removed the triage label Sep 3, 2019
@lincolnthree
Copy link

lincolnthree commented Sep 3, 2019

Confirmed this in my app. Memory references appear to be retained across page refreshes as well. It does appear if you leave the app idling for a while that GC occurs and references are cleaned up, but it does take a while for this to happen.

@lincolnthree
Copy link

lincolnthree commented Nov 20, 2019

@liamdebeasi

I believe I have finally traced the memory leak down to the ion-img component. I do not believe it is cleaning up its IntersectionObserver references properly. After hours of pouring over Chrome heap snapshots, this is where I've landed. I'm still tracing down a few other leaks but this one was by far the worst as images often appear in repeated list items / virtual-scroll elements.

Here the observer is created:
https://github.com/ionic-team/ionic/blob/master/core/src/components/img/img.tsx#L46

But there is no "componentDidDestroy" callback to clean up the listener, nor is there any code that would perform such cleanup elsewhere.

I do not know if Stencil supports such a concept, but... without it, (as I understand it) this will never be cleaned up as the Observer will not clean itself up, and the observer contains a reference to the component/element being observed.

I verified this by using the following image component:

<ion-img *ngIf="!native" [src]="_imgUrl" type="image/png">
</ion-img>
<img *ngIf="native" [src]="_imgUrl">

When using 'native' images, DOM elements did not leak across page transitions where images were displayed. Garbage collection properly cleans up the Detached Dom elements and the pages can be destroyed.

Using ion-img, dom nodes continued to grow.

@liamdebeasi
Copy link
Member

liamdebeasi commented Nov 20, 2019

Thanks for investigating. Is this the same issue as the one OP posted about? The demo app provided does not seem to make use of ion-img.

@lincolnthree
Copy link

@liamdebeasi Possibly not the same as the OP's issue. I just remembered this one was still here and seemed on topic.

@vially
Copy link
Author

vially commented Nov 20, 2019

Great find @lincolnthree! My demo did not use any ion-img so I would expect there must be some other leaks as well.

However, after using Ionic in a medium sized app for a while now I get the feeling that there are multiple Ionic components that are leaking (or maybe just a few leaks in some heavily used components like the ion-img that you've just identified).

It would be nice if the Ionic team tried to systematically identify and fix all of them at once rather than just try to fix the ones that get reported in various issues (not sure if that's possible though).

@ramikhafagi96
Copy link

Sorry to bring you this guys but this issue still persists and using ion-img causes memory leaks, in order to avoid this issue I had to implement lazy loading myself using IntersectionObserverAPI to make sure that I disconnect from the IntersectionObserver on the component destroy.

@liamdebeasi
Copy link
Member

liamdebeasi commented May 12, 2022

Hi everyone,

I am digging into this, and this appears to be a bug in Angular not Ionic.

Here is a reproduction of the issue in Angular (without Ionic): https://github.com/liamdebeasi/ng-router-demo

The issue is more pronounced in the Ionic app provided in the original post. However, that is to be expected as the Ionic app is more complex than my repro with a handful of elements.

I am reaching out to the Angular team for clarification on this and will provide updates as I have them.

See #19242 (comment)

@liamdebeasi
Copy link
Member

liamdebeasi commented May 16, 2022

Hi everyone,

I looked into this more and I can confirm that there is no memory leak in the reproduction provided. After upgrading the repo in #19242 (comment) to Angular 13 + Ionic 6, I was able to reproduce the reported behavior. However, the DOM nodes + event listeners do get garbage collected after a while. For example, this is the behavior I get after routing and going back several times:

image

There are a couple of things to point out:

  1. Notice that content does get garbage collected. If the charts for node or listeners increased indefinitely, that would be a sign of a memory leak. That is not the case here, which is good. The content is not garbage collected immediately, but that is to be expected.
  2. There is a saw tooth pattern, but a saw tooth pattern is not always a definite indicator of a memory leak. In this case, we are simply adding many nodes to memory, releasing them, and then adding more (I.e. routing back and forth multiple times). Notice that when content is garbage collected, the number of nodes/listeners always returns to the same baseline. If the baseline continued to increase, that would be a sign of a possible memory leak. This is not the case either, which is good. I added red lines to the image below to place emphasis on the consistent baseline:

Group 1


I am going to close this as this is not a bug in Ionic. Thank you!

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 15, 2022

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

4 participants