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: light dom custom element with comment node causes retainer #28189

Open
3 tasks done
terslada opened this issue Sep 17, 2023 · 27 comments
Open
3 tasks done

bug: light dom custom element with comment node causes retainer #28189

terslada opened this issue Sep 17, 2023 · 27 comments
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc)

Comments

@terslada
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When navigating between pages that should not be reused, there are leaks in both nodes and listeners, which makes the application unusable after a while.

Expected Behavior

Pages should be disposed correctly

Steps to Reproduce

Reproducible even inside your ionic conference app example. Simply navigate between schedule and support pages and watch the performance monitor.
Untitled_ Sep 17, 2023 11_13 AM

Code Reproduction URL

https://github.com/ionic-team/ionic-conference-app

Ionic Info

  • ionic info from conference app

Ionic:

Ionic CLI : 6.19.0
Ionic Framework : not installed
@angular-devkit/build-angular : 16.0.0
@angular-devkit/schematics : 16.0.0
@angular/cli : 16.0.0
@ionic/angular-toolkit : 6.1.0

Capacitor:

Capacitor CLI : 4.6.3
@capacitor/android : 4.6.3
@capacitor/core : 4.6.3
@capacitor/ios : 4.6.3

Utility:

cordova-res : 0.15.4
native-run : 1.7.1

System:

NodeJS : v18.16.1 (C:\Program Files\nodejs\node.exe)
npm : 9.7.2
OS : Windows 10

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Sep 17, 2023
@liamdebeasi liamdebeasi self-assigned this Sep 18, 2023
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Sep 18, 2023

Thanks for the report. I can reproduce this on the latest version of Ionic, but I need to take a closer look to understand why this is happening.

Is this happening in an app you are building?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 18, 2023
@terslada
Copy link
Author

Yes, it does.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 18, 2023
@liamdebeasi
Copy link
Contributor

Do you have a minimal reproduction of the issue happening in that app?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 18, 2023
@terslada
Copy link
Author

It is a production application that I can not really share, but It happens consistently every time I close a page that should be destroyed. Same as in the example app.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 18, 2023
@liamdebeasi
Copy link
Contributor

Any minimal reproduction you can provide will make it easier for us to diagnose and potentially fix the issue. Otherwise, I'll keep working through the conference app. However, it is a big app so it will likely delay any results.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 18, 2023
@terslada
Copy link
Author

Tried to recreate the behavior on a new app, but no luck there. My application is pretty similar to the conference app, but couldnt figure out what is the root cause.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 18, 2023
@liamdebeasi
Copy link
Contributor

It might be best to start with your production application since you know the bug reproduces there. Then you can start removing code until you have a minimal working example. There does seem to be an issue in the Ionic conference app, but at the moment I have no way of knowing if this is the same bug that is impacting your app.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 18, 2023
@terslada
Copy link
Author

terslada commented Sep 18, 2023

My application is bigger then the conference one, but I will give it a try once I have time for it... I would be very surprised if it were two different bugs though.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 18, 2023
@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 18, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 18, 2023
@liamdebeasi liamdebeasi removed their assignment Sep 20, 2023
@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 25, 2023
@liamdebeasi
Copy link
Contributor

I'm not able to reproduce any leaks:

image

Here is a video of me performing the steps to reproduce:

repro.mov

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 28, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 28, 2023
@terslada
Copy link
Author

Well, this is how it looks on my side. The same repository. What am I missing?

image
screen-capture.1.webm

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 28, 2023
@liamdebeasi
Copy link
Contributor

What version of Chrome are you using? I'm on 117.0.5938.92.

@terslada
Copy link
Author

terslada commented Sep 28, 2023

117.0.5938.132

Tested it also in Firefox and also on different PC. All results the same.

How come that even the heap size is so much different on the first snapshot I wonder

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Sep 28, 2023

Do you have an example of this happening in Firefox? (i.e. the heap snapshot)

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Sep 28, 2023
@ionitron-bot ionitron-bot bot removed the triage label Sep 28, 2023
@terslada
Copy link
Author

Not rly familiar with Firefox devtools. But this is how it looks. Could not find GC trigger, but hopefully it runs automatically before heap snapshot as in chrome.

screen-capture.2.webm

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Sep 28, 2023
@terslada
Copy link
Author

Got some interesting findings now.

Chrome behavior:

  • every time I navigate to the page and back, there are more and more nodes leaked.

Firefox behavior:

  • first time I navigate to the page and back, the same nodes are leaked. But with each consequent navigation, there are no more leaked nodes.

If I remove the footer component that I mentioned before, the Chrome starts to behave same way as Firefox. Leaking only for the first time and stay consistent after.

@liamdebeasi liamdebeasi changed the title bug: Leaks when navigating between pages bug: light dom custom element with comment node causes retainer Dec 8, 2023
@liamdebeasi
Copy link
Contributor

Hey there! Apologies for the delay. I can confirm that certain components are being retained in memory when they should not be. I was able to determine that ion-footer is being retained by the comment node inside of it. Stencil renders this comment node for Light DOM components that use <slot></slot>. I've sent this over to the Stencil team to take a closer look at. I'll follow up here when I have more to share.

@liamdebeasi liamdebeasi added the bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc) label Dec 8, 2023
@ionitron-bot ionitron-bot bot removed the triage label Dec 8, 2023
@terslada
Copy link
Author

terslada commented Dec 8, 2023

Great, I got rid of the footer because of this. looking forward to a fix.
Thanks

@terslada
Copy link
Author

terslada commented Feb 6, 2024

Any news on this?

@liamdebeasi
Copy link
Contributor

Can you give this a try in Ionic 7.7.0? The underlying Stencil bug should have been resolved in Stencil 4.11.0.

@terslada
Copy link
Author

terslada commented Feb 6, 2024

Sadly still behaves the same way

@liamdebeasi
Copy link
Contributor

Thanks for testing. I spoke with the Stencil team, and it sounds like there may be multiple bugs in Stencil that need resolving. I'll work with them to see if we can find a good resolution here.

@terslada
Copy link
Author

Lately I look a bit into the memory leaks again. There are leaks everywhere caused by using ionic components. I dont rly know what to do, because the application gets unusable after a while. It is ment to be running for many hours, but now it has to be restarted often to be usable.

image
image

@aeharding
Copy link
Contributor

Thanks for testing. I spoke with the Stencil team, and it sounds like there may be multiple bugs in Stencil that need resolving. I'll work with them to see if we can find a good resolution here.

@liamdebeasi, are there Github issues for these Stencil bugs?

@liamdebeasi
Copy link
Contributor

The issues that are potentially related are ionic-team/stencil#5181 and ionic-team/stencil#5172. The Stencil team is still investigating these issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: external Bugs in non-Ionic software that impact Ionic apps (I.e. WebKit, Angular, Android etc)
Projects
None yet
Development

No branches or pull requests

3 participants