bug: chrome, complex selector causing elements to not get garbage collected

[joewoodhouse]

Prerequisites

• I have read the Contributing Guidelines.
• I agree to follow the Code of Conduct.
• I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

• v4.x
• v5.x
• v6.x
• v7.x
• Nightly

Current Behavior

When a controller modal is dismissed, it's contents are detached from the DOM but are unable to me garbage collected. This represents a resource leak, and repeated opening/closing of a modal like this will eventually lead to the app being killed by the operating system.

Expected Behavior

When a modal is dismissed, all resources associated it should be freed.

Steps to Reproduce

1. Go to the documentation page for controller modals:
   https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios
2. Open Chrome Devtools "Performance Monitor" tab
3. Take a Heap Snapshot (Memory > Record button)
4. Repeatedly open and close the modal
5. Watch the Performance Monitor tab's "DOM Nodes" number increase indefinitely
6. Force manual GC (trash icon in the Memory tab) - note the DOM nodes do not reduce
7. Take another Heap Snapshot and compare. You will see 100s of Detached Elements

A video of me doing these steps:
https://drive.google.com/file/d/1ciQxC1xSa_aiyrA2OfEIJCjXHJH_sBhx/view?usp=sharing

Code Reproduction URL

https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios

Ionic Info

Ionic:

Ionic CLI : 6.20.6 (/Users/joewoodhouse/.nvm/versions/node/v16.14.2/lib/node_modules/@ionic/cli)

Capacitor:

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

Utility:

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

System:

NodeJS : v16.14.2 (/Users/joewoodhouse/.nvm/versions/node/v16.14.2/bin/node)
npm : 8.7.0
OS : macOS

Additional Information

I believe these issues are probably related to an underlying Stencil problem, but I haven't been able to track it down. As part of my investigation I've found other issues in stencil namely
ionic-team/stencil#4070
ionic-team/stencil#4067

There are known issues around how Stencils runtime and VDOM implementation hold strong (rather than weak) references to DOM nodes that are probably also contributing to this issue.

Whilst obviously for a documentation website this isn't a problem, for us this represents a major problem as we have a mobile application that displays 100s of modals during the course of the apps lifetime. This bug means that are users regularly experience crashes after a moderate amount of use, simply because the operating system kills the WebView due to it using too many resources. Using inline modals could be an option for us but it would require a major refactor.

[liamdebeasi]

Thanks for the report. I can reproduce this behavior, but I am still digging into the source of the issue. I'll update here when I have more information.

[joewoodhouse]

Thanks @liamdebeasi. I've spent a lot of hours looking at this for the last week or so, if you'd like to chat about it in person let me know.

[liamdebeasi]

This appears to be a Chromium bug. All overlays receive an overlay-hidden class that is later removed on presentation. This lets us append elements to the DOM while avoiding a flicker. We also have a selector that prevents backdrops from stacking when multiple modals are open:

ionic-framework/core/src/css/core.scss

Line 95 in 6dcd98b

ion-modal.modal-default:not(.overlay-hidden) ~ ion-modal.modal-default {

(i.e. the backdrop does not appear to get darker every time you open a modal).

This complex selector seems to be causing the issue. Removing it causes the issue to no longer reproduce. I was able to reproduce this behavior outside of Ionic, and I have reported this issue to the Chromium team: https://bugs.chromium.org/p/chromium/issues/detail?id=1418768

I will see if there is an alternative implementation for the selector above.

[joewoodhouse]

Hi @liamdebeasi

If I go the reproduction URL (https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios) and remove the Ionic CSS element from the page, and then run the same steps, exactly the same behaviour still happens for me?

I'm also not convinced this can just be a Chromium bug as I can reproduce this easily in Safari as well?

[liamdebeasi]

It's possible there are multiple memory leaks. Can you provide Safari reproduction steps? Also, you'll want to remove the <link> element not the <style> elements since the buggy styles are in ionic.bundle.css. Removing this <link> element from https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios causes the issue to no longer reproduce on my end.

[joewoodhouse]

Yea I suspect there are multiple memory leaks, which is why I'm a bit concerned you've renamed and labeled this as a Chromium bug only - I don't want the other issues getting forgotten about.

At the least I think the underlying Stencil issues I mentioned in my original post are contributing somehow. Also I wonder if the modal holding strong references to e.g. usersElement, backdropEl etc also interfere with garbage collection? (https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/modal/modal.tsx#L76-L88)

The steps are the same in Safari, just you need to use the timeline to monitor the leak.
Here is a video of me doing it again:
https://drive.google.com/file/d/1z35_OV6ZDGbQIivCBtOdEZLxRToRiCVI/view?usp=sharing

(I run const run = () => { document.querySelector('ion-button').click(); setTimeout(() => { document.querySelector('ion-modal').dismiss(); setTimeout(run,500)}, 500) }; run() in the console to get it to constantly open/close)

[liamdebeasi]

If there are additional memory leaks in Stencil, then those issues should be tracked in ionic-team/stencil#4070 and ionic-team/stencil#4067 instead. I'll look into the Safari reproduction.

[joewoodhouse]

It's possible there are multiple memory leaks. Can you provide Safari reproduction steps? Also, you'll want to remove the <link/> element not the <style/> elements since the buggy styles are in ionic.bundle.css. Removing this <link> element from https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios causes the issue to no longer reproduce on my end.

Yes apologies I meant to write <link/> not <style/>, I removed the ionic.bundle.css as you describe. Here's a video of me doing this and watching the memory leak still happen - not sure what steps I'm doing differently to you?
https://drive.google.com/file/d/1IpLQfDjbt2FhqNmEmbTFOWCwBIrHB_Tn/view?usp=sharing

[liamdebeasi]

I see a few more retainers:

1. The createEvent function in Stencil is holding onto a reference to the component: https://github.com/ionic-team/stencil/blob/40f1aca4192687e2fef19bfb750373c5604ec179/src/runtime/event-emitter.ts#L9 This reference is then used in the emit callback, but the browser doesn't know when the callback is going to run (or if it will be used in the future), so it keeps the reference around.
2. Element refs do not appear to be removed. In particular, the wrapperEl and backdropEl references are never destroyed:

   ionic-framework/core/src/components/modal/modal.tsx

   Lines 76 to 77 in 6dcd98b

   	private wrapperEl?: HTMLElement;
   	private backdropEl?: HTMLIonBackdropElement;

   .
3. Other strong references being retained in hostRef in Stencil. These are other internal Stencil things like $onReadyResolve$, $onInstanceResolve$, and $hostElement$. It seems as though there are instances where the host ref is not being destroyed.

This issue only appears to impact the lazy loaded bundle of Stencil, so dist-custom-elements is not impacted. It looks like your two Stencil bug reports cover these, so I recommend following up with the Stencil team there.

---

There are a couple areas in Ionic where we are holding onto references. These do not appear to be contributing to the problem, but we should still fix them:

1. The keyboardOpenCallback callback is never cleared to it being added async even though it is re-added on every present:

   ionic-framework/core/src/components/modal/modal.tsx

   Line 727 in 6dcd98b

   window.removeEventListener(KEYBOARD_DID_OPEN, this.keyboardOpenCallback);

   I have created a PR for this here: fix(modal): keyboard listener removed on dismiss #26856
2. The lastFocus reference is not cleared when overlays are dismissed:

   ionic-framework/core/src/components/modal/modal.tsx

   Line 93 in 6dcd98b

   lastFocus?: HTMLElement;

   I have created a PR for this here: fix(overlays): focus trap refs cleared on dismiss #26855

---

I'll keep this ticket open until #26856 and #26855 are merged. Also it looks like the Chromium team has verified the bug I reported the other day: https://bugs.chromium.org/p/chromium/issues/detail?id=1418768

[joewoodhouse]

Hi @liamdebeasi

Nice work on finding the extra retainers.

I'm not sure I agree that the 2 stencil issues I've raised cover any of your first 3 bullet points.

When you say this only impacts the lazy loaded bundle of Stencil, I tried doing this myself in a simple example repository and I still see the same behaviour. Link here:
https://zippy-seahorse-69a6db.netlify.app/
(note I removed the dodgy CSS selector that was causing the Chromium bug from this build)

I also don't really agree that this issue should be closed once your 2 smaller fixes are merged. My original issue (before it was renamed) was that controller modals leak resources, and that will still be the case after your fixes are merged. If Stencil was a 3rd party library I could understand, but it's essentially a custom made build tool for Ionic, so whether the problems are with Ionic or Stencil to me as a user of the library doesn't really make a difference. My users are still experiencing the app crashing regularly.

[liamdebeasi]

When you say this only impacts the lazy loaded bundle of Stencil, I tried doing this myself in a simple example repository and I still see the same behaviour.

Sorry, my previous wording was not clear. Anything that causes Stencil components to be lazily loaded is impacted by this bug. In other words, if you are not explicitly importing and calling defineCustomElements on each component, these issues will appear. This does not impact Ionic React and Ionic Vue because they do not use the lazy loaded build of Stencil.

I also don't really agree that this issue should be closed once your 2 smaller fixes are merged.

I'll file some separate issues with the Stencil Team, but we typically do not keep 3rd party bugs open (the exception being browser-related bugs). We used to do it for older issues, but it makes tracking the state of the bug hard since you essentially have multiple sources of truth across multiple teams.

[joewoodhouse]

My example repository literally does import each custom element and define it, as per the documentation (I think)

import { defineCustomElement as defineModal } from '@ionic/core/components/ion-modal.js';
import { defineCustomElement as defineInput } from '@ionic/core/components/ion-input.js';
import { initialize, modalController } from '@ionic/core/components';

initialize();
defineModal();
defineInput();

const createModal = async () => {
    const component = document.createElement('ion-input');
    component.type = "text";
    const modal = await modalController.create({
        component
    });
    return modal;
}

export const runTest = async () => {

    while (true) {
        const modal = await createModal();
        await modal.present();
        await new Promise(r => setTimeout(r, 300));
        await modal.dismiss();
    }
}

[liamdebeasi]

Do you have a link to your repository? All I saw in the previous comment was https://zippy-seahorse-69a6db.netlify.app/.

[joewoodhouse]

Sorry, here's the code:
https://github.com/joewoodhouse/ionic-dist-custom-elements-test

[liamdebeasi]

I'm not able to reproduce the issue with that repo:

The number of nodes does increase (because the demo creates multiple modals), but the browser eventually garbage collects old ones.

[joewoodhouse]

Hmm that's odd, video of me running it here:
https://drive.google.com/file/d/1f0bnxC53Mx9fAQYcfDOsreLDGTeQcPwe/view?usp=share_link

[liamdebeasi]

Are you still able to reproduce the issue at https://ionicframework.com/docs/usage/v6/modal/controller/demo.html?ionic:mode=ios? The two references I noted in #26833 (comment) were cleaned up and shipped in Ionic 6.5.7 earlier this week. I'm not able to reproduce the issue anymore. (It appears these references were more significant than I originally noted)

[joewoodhouse]

@liamdebeasi yea still seems to happen for me, if I go to that page and run the script above

const run = () => { document.querySelector('ion-button').click(); setTimeout(() => { document.querySelector('ion-modal').dismiss(); setTimeout(run,500)}, 500) }; run()

and watch the performance tab, the nodes and listeners still increase indefinitely

[liamdebeasi]

Is that with the ionic.bundle.css file included?

[joewoodhouse]

Yea even if I remove the for the ionic css

[liamdebeasi]

I was able to avoid the Chrome memory leak bug in: #26911

I'll take a look at any remaining retainers once that fix ships.

[liamdebeasi]

A workaround for the Chrome memory leak bug shipped out today. I gave the repo another test using your code in #26833 (comment) and things seem better:

Can you take another look?

[joewoodhouse]

Hi @liamdebeasi yea just ran my test again locally and it seems to be sorted now! I see the same graphs as you and the number of nodes and listeners always returns back to the same number after a GC. Nice work!

[liamdebeasi]

Great! Sounds like the issue is resolved, so I am going to close the thread. If you see the issue appear again feel free to reply, and I can re-open this.

[ionitron-bot]

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.