bug: Potential Memory/Resource Leak

[joewoodhouse]

Prerequisites

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

Stencil Version

2.18.0

Current Behavior

I am investigating some behaviour I'm seeing in my application (an Ionic application using StencilJS as the framework/runtime) where memory and DOM nodes appear to be leaking. This is occurring when modals are being opened and then dismissed.

I have created a minimal reproduction which you can find here: https://rococo-creponne-086676.netlify.app/

Code here: https://github.com/joewoodhouse/stencil-modal-memory-investigation

The reproduction site basically offers two buttons - one that demonstrates the resource leak, and one that demonstrates non-leak behaviour. Both buttons open a modal via the modalController.create method, then immediately dismiss it, and repeat. The component that is created in the modal (app-home) takes one prop, which alters the content it displays. When the "Run Broken" button is pressed, the app-home component renders an <ion-list/> in it's element. When "Run Working" is pressed, the app-home component uses a <div/> instead of an <ion-list/>. The use of div vs ion-list is the only difference (from a user perspective) between the two scenarios.

Once you have pressed "Run Broken" you can see the resource leak via DevTools. Using the Performance Monitor you can see the number of DOM nodes ever increasing, and any attempt to manually trigger a garbage collection does not bring them down. Where as when you choose "Run Working" then the number of DOM nodes will remain stable and triggering a manual GC will immediately flush it back to starting levels.

Taking a Heap Snapshot also shows the leak, where if you inspect the snapshot you will see 100s of Detached elements (ion-modal) instances in the heap.

Tested in Chrome 105.0

I have tried and failed to recreate this within the Ionic source code - my original hunch was that is was some leak in the modal implementation, or perhaps even the ion-list element. But I could not recreate it there which made me believe that it must be more down to the Stencil runtime. Whenever I'm exploring the heap snapshots I come to a dead end at $lazyInstance$ which I think is part of the Stencil runtime, but I am not an expert and couldn't get further.

This is a real issue for us that our users are experiencing - I hope that my code and reproductions are useful and if you need any more info please let me know.

Expected Behavior

Resources (heap memory and DOM nodes) should not leak when opening and closing modals.

Steps to Reproduce

Use the reproduction site (either directly here https://rococo-creponne-086676.netlify.app/ or the code is here https://github.com/joewoodhouse/stencil-modal-memory-investigation

Press "Run Broken"

Monitor DOM nodes and Heap memory

Note: The modal is only visible at a desktop viewport size, I couldn't work out the Ionic reason why

Code Reproduction URL

https://github.com/joewoodhouse/stencil-modal-memory-investigation https://rococo-creponne-086676.netlify.app/

Additional Information

Using latest Ionic as well (6.2.7)

[joewoodhouse]

I've now refined my reproduction, though I'm not closer to being able to explain what's going on. I've pushed my changes to my repo and Netlify link.

The core seem to be:

• The app main page contains an <ion-list> element
• A modal is presented
• The modal's component renders a custom component called <my-list>
• The <my-list/>'s render() function returns <Host class="list-md"/>

With the above set of conditions, the leak occurs. If the class list of the <my-list/> component does not contain list-md then the leak does not occur. Equally, if the <ion-list> on the main page is removed, the leak also does not occur.

So the leak appears to be due to the interaction between the <ion-list> and the <my-list> elements, it seems through their CSS. This is where my understanding breaks down unfortunately.

My next step will be to try and remove the modal element completely from the reproduction to narrow things down further.

[tanner-reits]

@joewoodhouse Thanks for reporting this issue and providing a replication site/repo/steps! Reproduction cases are super helpful for us as we try to pinpoint where some of these obscure issues lie.

That being said, this one could be a doozy. I was able to verify the issue with nodes being infinitely created on your reproduction url. Looks like some garbage collection process is getting hindered. I'll get this one labeled appropriately so it gets pulled into our backlog for work in a future sprint! Thanks for your patience and we'll post any updates here!

[vaibhavshn]

I've been debugging this issue.

Only place i could find which still references a node which has been unmounted is the hostRefs weakmap. Although the $hostElement$ and the $lazyInstance are set as key in this weakmap, there are still references to this key in the value object.

And as per MDN:

A WeakMap is a collection of key/value pairs whose keys must be objects, with values of any arbitrary JavaScript type, and which does not create strong references to its keys.

This might be preventing the nodes from being garbage collected.

In the following image, my-test has been removed from DOM, but the node still exists in the value and key.

code where this is set: https://github.com/ionic-team/stencil/blob/66ce0d330b296ad8ac1ef7ebb5732816af5a6244/src/client/client-host-ref.ts#L32

also, even state values, reference to elements are also not cleaned up because of this.

Solution would be to remove the references to the key from the values, and come up with another way to handle it.

[vaibhavshn]

We've seen this memory leak affect our application when we've unmounted a lot of elements, especially things such as tooltips, chat messages are used extensively throughout the app.

[joewoodhouse]

@vaibhavshn Nice work, let me know if you need any help. @tanner-reits has there been any internal investigation on this one? Still causing us problems

[alicewriteswrongs]

@vaibhavshn thanks for digging into this! It looks like we've got a circular reference between the key and the value within the WeakMap which would I think prevent the key from ever being garbage collected, since the reference to the key held by the value (under $hostElement$) is going to be a strong reference :( so even if it's not referenced anywhere else (e.g. it has been removed from the DOM) I think the garbage collector will continue to skip on by and leave it hanging around since it sees that strong reference.

Thank you again for digging into this and finding some key clues! Such things are tricky, and your work will definitely help address the issue. So thanks again! 🔎🧐👍💯

I think I will have time fairly soon to look more into what we can do to address the issue. At minimum, we need to not have a reference to the key in the WeakMap from the value, but I'll need to explore a little bit to see how exactly we can get rid of this. Possibly we can change the reference in the value object to be a WeakRef...

Thanks again!

[vaibhavshn]

Sure @alicewriteswrongs, do let me know of any timeline you have on the fix. And do let me know if you need any help from me too.

[raymondboswel]

I have another example of a memory leak, here: https://github.com/raymondboswel/stencil-redux-example. The non leak version uses RxJS for state management and the leaky version uses stencil/store, which is the only difference between the leaky and non-leaky versions. Should I create another issue (perhaps on stencil/store?) or is it possibly related to this leak?

[IndyVC]

Hi all, we noticed something similar: Using stencil for our components, we noticed a lot of detached elements (which are all created in stencil, used in a react application with the stencil react wrapper). We got performance issues so started looking around with chrome dev tools, apparantly we stored a shitload of elements that are not drawn on the screen anymore.

If this could be of any help:

@raymondboswel we have components that do not import anything from stencil/store, so I'assuming its not stencil/store related.

[alicewriteswrongs]

@IndyVC thanks for more info - always a help in tracking things like this down! At present I know there is definitely some memory leakage happening as a result of how Stencil's virtual DOM implementation holds on to references to DOM nodes. In a lot of situations we create objects holding metadata related to how and where a component has been rendered to the DOM, and often these have references to DOM nodes (but they aren't cleaned up well by the Stencil runtime).

I did some work replacing those normal references with WeakRefs but I wasn't able to completely address the memory issue by doing so. At present we're working on getting Stencil v3 out the door, and once we do that should free up some time for the team to look into issues like this one.

I have not yet investigated the memory leak which happens when using stencil-store, so I can't say right now whether or not it's related - thank you for filing an issue on the repo and providing a reproduction case though!

[joewoodhouse]

@alicewriteswrongs any update on this issue? This is basically making our production app unusable at the moment as the references are leaking so much that the app eventually crashes (we're using this for a hybrid mobile app with Ionic and vanilla JS). Any suggestions of workarounds or any help I can give let me know.

[gabides]

Hi @alicewriteswrongs any news on addressing this issue? We are having memory leak issues in our app due to this, and it is causing some crashes in our app

[ghaiat]

hey, any update on this ? it s pretty big issue

[alicewriteswrongs]

Sorry to report that there's no news on this front - rest assured that I will share updates when I have them!

[ghaiat]

@alicewriteswrongs does this affect all stencil components? only the ones with @State? or something else?

is there any workaround?

our application is crashing a lot because of this and we need to find a solution for our clients asap ....

[denyo]

We're also waiting for this to be fixed. We want to use WeakMaps with a component's host element as a key that we don't have to clear manually once the component disconnects. However, with the current behavior the maps just keep growing since the references are never garbage collected.

[joewoodhouse]

@ghaiat from my investigation this impacts all components

@alicewriteswrongs appreciate you keeping us in the loop. I would say, to me this seems like a really massive issue, and it impacts my work building and maintaining Ionic apps on an almost daily basis. And from the above seems like others do too. Since I reported this there have been several releases of Stencil (including a major release) that in my opinion contain a number of "minor" features compared to this. I'd be really keen to hear that this issue is being taken seriously and is on the roadmap for resolution soon.

[ghaiat]

@joewoodhouse i agree 100%

@alicewriteswrongs can you explain how an issue as big as is, causing perf/crash issues for anyone using stencil has not been moving for 9 months, this is insane

[alicewriteswrongs]

hey y'all, let's keep it civil, I just work here 🙂 @ghaiat the short answer, if you're curious, is that the people who wrote Stencil originally no longer work on it so bugs like this require a significant amount of time for us to root-cause and fix. There's no conspiracy to keep memory leaks around, just a small team with limited resources and a series of priorities.

I am looking into this now actually (as in spending time on it today) and I will definitely, 100%, I promise let you all know when I have something to share. At present I can't really say more than that.

[capc0]

sorry to ask, but it has been a while. Any known workarounds for this issue? Can we provide more data or help otherwise?

[capc0]

a possible solution without breaking the current WeakMap logic could be to have a method unregisterHost which removes the element from the hostsRef WeakMap. https://github.com/ionic-team/stencil/blob/66ce0d330b296ad8ac1ef7ebb5732816af5a6244/src/client/client-host-ref.ts#L6 However, there must be some core callback where that function would be called. My guess we be somewhere around here in the disconnectedCallback: https://github.com/ionic-team/stencil/blob/66ce0d330b296ad8ac1ef7ebb5732816af5a6244/src/runtime/bootstrap-lazy.ts#L128

[conceptree]

Hello @alicewriteswrongs ,

This is a big one and feels like it should be a top priority. Is there any perspective of a delivery date for this fix?

Thanks in advance.

[pm0u]

Any updates here? Running into some issues that we think could be caused by this

[rwaskiewicz]

Hey folks,

I don't have an update on this particular issue at this time.

Please do me a favor and add 👍's to the issue summary to upvote it - that's the best way for the team to track issues that are affecting projects. GitHub doesn't give us an easy way to track other types of comments, which makes them more likely to not be properly counted when we prioritize issues.

Thanks!

[adbetin]

Any updates here? I'm facing the same performance issue related to DOM detached elements that are referenced by a WeakMap. Any suggested date or workaround?

[aeharding]

Any updates here? I'm facing the same performance issue related to DOM detached elements that are referenced by a WeakMap. Any suggested date or workaround?

Vote on the issue please don't spam.

[denyo]

Any updates here? I'm facing the same performance issue related to DOM detached elements that are referenced by a WeakMap. Any suggested date or workaround?

Use a map and take care of the steps manually.

const observedNodesMap: Map<HTMLElement, () => void> = new Map();

export const observeChildren = <T extends HTMLElement, K = keyof T>(
  node: T,
  callback: () => void,
  attributes?: (Lowercase<K extends string ? K : string> | keyof AriaAttributes)[]
): void => {
  // node might not be defined in connectedCallback
  if (node) {
    observedNodesMap.set(node, callback);
    // init observer or whatever you want to do
  }
};

export const unobserveChildren = <T extends HTMLElement>(node: T): void => {
  observedNodesMap.delete(node);
};

And then in your components

public connectedCallback(): void {
  observeChildren(this.host, () => { ... });
}

public disconnectedCallback(): void {
  unobserveChildren(this.host);
}

This could be made reusable and less verbose with a custom decorator or reactive controller.

[vaibhavshn]

Any updates here? I'm facing the same performance issue related to DOM detached elements that are referenced by a WeakMap. Any suggested date or workaround?

Use a map and take care of the steps manually.

const observedNodesMap: Map<HTMLElement, () => void> = new Map();



export const observeChildren = <T extends HTMLElement, K = keyof T>(

  node: T,

  callback: () => void,

  attributes?: (Lowercase<K extends string ? K : string> | keyof AriaAttributes)[]

): void => {

  // node might not be defined in connectedCallback

  if (node) {

    observedNodesMap.set(node, callback);

    // init observer or whatever you want to do

  }

};



export const unobserveChildren = <T extends HTMLElement>(node: T): void => {

  observedNodesMap.delete(node);

};

And then in your components

public connectedCallback(): void {

  observeChildren(this.host, () => { ... });

}



public disconnectedCallback(): void {

  unobserveChildren(this.host);

}

This could be made reusable and less verbose with a custom decorator or reactive controller.

I am not sure I understand how this would solve or work around the bug here. The underlying weakmap is still being used to store the objects that is causing the bug right?

[joewoodhouse]

Sorry to be that guy, but any updates on this issue?

It's fast approaching 2 years (!!) since I raised this issue. This is a real memory leak, which causes mine and probably everyone else's production apps to crash/restart.

We're currently facing a serious incident with a customer which basically boils down to the app crashing due regularly due to this memory leak. If I had an option to move off of Stencil/Ionic at the moment I would, because of this, which makes me sad to say as I've been an Ionic dev since day 1.

Any update would be much appreciated!

[christian-bromann]

@joewoodhouse unfortunately we don't have an update at the moment. We investigated the issue without success. We were also facing competing priorities which makes it difficult to give this issue the necessary attention it deserves. We would appreciated any support in the investigation of this problem. Thank you for understanding!