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

Ionic 4 - Memory leak of Pages via lifecycle event listeners #16285

Closed
KevinKelchen opened this issue Nov 9, 2018 · 7 comments · Fixed by #16806
Closed

Ionic 4 - Memory leak of Pages via lifecycle event listeners #16285

KevinKelchen opened this issue Nov 9, 2018 · 7 comments · Fixed by #16806
Assignees
Labels
package: angular @ionic/angular package type: bug a confirmed bug report

Comments

@KevinKelchen
Copy link

Bug Report

Ionic Info
Run ionic info from a terminal/cmd prompt and paste the output below.

Ionic:

   ionic (Ionic CLI)             : 4.3.1 (/usr/local/lib/node_modules/ionic)
   Ionic Framework               : @ionic/angular 4.0.0-beta.15
   @angular-devkit/build-angular : 0.8.6
   @angular-devkit/schematics    : 0.8.6
   @angular/cli                  : 6.2.6
   @ionic/angular-toolkit        : 1.1.0

System:

   NodeJS : v10.13.0 (/usr/local/bin/node)
   npm    : 6.4.1
   OS     : macOS

Describe the Bug
I think I'm observing a memory leak in @ionic/angular code that is holding onto a reference of a Page instance.

This is having a more significant impact in our enterprise app as it causes numeorus Page instances to leak even after the Page has been destroyed. While I haven't been able to reproduce more than two Page instances leaking in a simple sample app, I've been able to reproduce what I believe is the cause of the leak in a vanilla starter template project.

Steps to Reproduce
Steps to reproduce the behavior:

  1. Start a new project with ionic start myLeak sidemenu --type=angular.
  2. Path into the project directory and run ionic serve.
  3. Open Chrome DevTools.
  4. On the sidemenu click List and then click Home.
  5. In the DevTools go to the Memory tab.
  6. Click the Collect garbage button.
  7. Click the Take heap snapshot button.
  8. In the Class filter search box type homepage to find leaking instances of the HomePage.
  9. Expand the HomePage node.
  10. There should be 2 instances of HomePage. Click on one of them. In the Retainers window at the bottom there should be a retainer that traces through angular-delegate.js:94 and __zone_symbol__ionViewWillEnterfalse in Detached HTMLElement. You may need to expand and collapse objects in the Retainers window or try the other HomePage instance to find it.
    image

Related Code
Follow the simple instructions in Steps to Reproduce to create a project that will reproduce the issue. Note the Ionic Info section for version info.

Expected Behavior
I did not expect instances of the HomePage to leak due to a lingering reference.

Additional Context
Using the file and line number info from the Retainers window (angular-delegate.js:94), I investigated. The source of the leak may be in angular-delegate in the bindLifecycleEvents() function where it is adding lifecycle-related event listeners which reference the Page instance in the callback function. I did not see these listeners get removed by Ionic when leaving the Page.

I experimented by commenting out the body of bindLifecycleEvents() and that seemed to make the leak go away. I also hacked in unbinding the lifecycle events when leaving the Page and that, too, seemed to make the leak go away.

I think perhaps the issue may be due in part to the lifecycle event listener callback having a reference to the Page instance via a closure and that the event is not being unbound.

Despite this issue, thank you for making a great framework! 😀

@ionitron-bot ionitron-bot bot added the triage label Nov 9, 2018
@KevinKelchen
Copy link
Author

KevinKelchen commented Nov 26, 2018

I still observe this with Beta 16.

With some of the Pages in our app having memory-heavy objects (such as a ChangeDetectorRef) each Page instance is leaking about 1 MB. So in our case every time you visit a Page you leak in total around 1 MB. This doesn't seem like it will scale as users use the application. While we could add our own code to null out properties on the Page on ngOnDestroy() to lessen the blow of the leak or use a forked version of Ionic Angular, this would be only working around the core problem in Ionic.

Are there any updates on this issue from the Ionic team? Thanks!

@KevinKelchen KevinKelchen changed the title Memory leak of Pages via lifecycle event listeners Ionic 4 - Memory leak of Pages via lifecycle event listeners Nov 26, 2018
@ionitron-bot ionitron-bot bot removed the triage label Nov 30, 2018
@adamdbradley adamdbradley added package: angular @ionic/angular package type: bug a confirmed bug report labels Nov 30, 2018
@manucorporat manucorporat self-assigned this Dec 6, 2018
@KevinKelchen
Copy link
Author

Thank you so much, @manucorporat! I can't wait to try it out in the next release! 😀

@KevinKelchen
Copy link
Author

KevinKelchen commented Dec 17, 2018

This memory leak is still present in Beta 19, @manucorporat. 🙁

I know that changes were made to mitigate this, but it still seems to be an issue. Following the steps to reproduce in the OP still demonstrates the issue.

Should this issue be re-opened or should I create a new one?

Thank you very much! 🙂

@dylanvdmerwe
Copy link
Contributor

Woop. I love the fixing of memory leaks 🍾

@KevinKelchen
Copy link
Author

The leak pertaining to lifecycle event handlers appears to be fixed! 👍

Thanks so much, @manucorporat! 😀

@sasos90
Copy link

sasos90 commented Dec 20, 2018

Resolving the routes with resolvers is the biggest issue am I right?

@ionitron-bot
Copy link

ionitron-bot bot commented Jan 19, 2019

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 Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: angular @ionic/angular package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants