ionic-team / stencil

A toolchain for building scalable, enterprise-ready component systems on top of TypeScript and Web Component standards. Stencil components can be distributed natively to React, Angular, Vue, and traditional web developers from a single, framework-agnostic codebase.
https://stenciljs.com
Other
12.56k stars 786 forks source link

bug: Deleting Element with @State causes memory leak when used in componentDidLoad lifecycle hook #3158

Open tarqwara opened 2 years ago

tarqwara commented 2 years ago

Prerequisites

Stencil Version

2.11.0

Current Behavior

When a @State variable is changed in the componentDidLoad lifecycle hook and that component is detached (removed) from the DOM, it remains in the Javascript memory, causing a memory leak.

Expected Behavior

The component ShadowRoot, nodes, event listeners etc. are not kept in the memory when the component is detached.

Steps to Reproduce

  1. Add a @State variable
  2. Change the value of the state variable in the componentDidLoad() hook
  3. Remove the element from the DOM
  4. Run a profile in the Chrome Dev Tools Memory tab (click the Collect garbage button beforehand for cleaner result)
  5. You should see detached HTMLElement, ShadowRoot, StyleElement etc.

image

Code Reproduction URL

https://github.com/tarqwara/stencil-state-memory-leak

Additional Information

No response

rwaskiewicz commented 2 years ago

Thanks for this! I suspect that because the element is being removed outside of Stencil, it doesn't know that it's been removed, which is leading to the references there in the screenshot. I'm going to mark the issue to get ingested for us to take a closer look at.

danielalvenstrand commented 2 years ago

Hi, have you had the time to take a deeper look into this issue? This bug persists and is causing us some problems still.

vaibhavshn commented 1 year ago

Have added details about possible memory leak (DOM nodes not being cleaned up) in this issue: https://github.com/ionic-team/stencil/issues/3607#issuecomment-1331622222

alicewriteswrongs commented 1 year ago

@tarqwara hello there! I know it's been quite a while since you opened this issue, but I wanted to ask if you were seeing this error outside of running with the dev server?

I'm investigating #3607 today and was looking at this issue as possibly related. After upgrading your reproduction case to @stencil/core@3.4.0 I was able to reproduce the issue if I used the Stencil dev server (stencil build --dev --watch --serve) but if I instead run a full build and then serve the results, doing something like:

npx http-serve www

I no longer see the issue (meaning, after I 'detach' the element I don't see any DetachedHTMLElement objects in a memory snapshot). This makes me suspect that something in the Stencil dev server code is holding on to a reference to those elements.

If you're still up for it it would be great if you could confirm under what circumstances you've run into the error!

vaibhavshn commented 1 year ago

@alicewriteswrongs looks like it is reproducible in the build output still.

I have a simple example running over at https://stencil-scratchpad.vercel.app. You can try toggling the component and check the Memory Snapshot.

alicewriteswrongs commented 1 year ago

Hey thanks for providing that repro! I did notice that the relevant objects are being logged to the console, which I believe will prevent them from getting garbage collected (see this article for an explanation of that). Would it be possible for you to test if it's reproducible without the logging?

vaibhavshn commented 1 year ago

Hmm, looks like you are right. It happens only in the dev server and not in the final build output.

So does it mean that the issue https://github.com/ionic-team/stencil/issues/3607 also happens only in dev server.

Also I am trying to log the hostRefs value without it being logged with my sample but looks like both dev server and production builds work fine, without the memory leak in the simple sample.

alicewriteswrongs commented 1 year ago

I think #3607 is a separate issue I think, although unfortunately I still haven't had time to fully dig into it, so I don't think it is dev server specific. It's a complicated one! Garbage collection is supposed to make our lives easy, not harder!