-
Notifications
You must be signed in to change notification settings - Fork 798
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: elements with HostListeners are leaked if element is not attached #4067
bug: elements with HostListeners are leaked if element is not attached #4067
Comments
I've created a unit test that higlights the bug on the following (draft) PR |
Hey @joewoodhouse thanks for filing this, I was just able to confirm there's an issue here with the reproduction case you supplied. Not a happy graph! Thanks also for the write-up about how exactly to reproduce this and some thoughts on what Stencil's ideal behavior should be. I think at present the team will take some time to digest this and work towards addressing it (alongside the other memory leak + vdom order-of-operations issues we have). Thanks again for filing! |
Great bug report!
My 2 pennies- I can’t think of many use cases where the proxy component needs to listen before being connected? |
Hi @alicewriteswrongs, is there any update on this issue? |
@sammalloy unfortunately nothing to report atm, when there's a potential fix I will be sure to ping this issue 🙂 |
A fix for this was included in today's v4.19.0 release! |
### Release Notes <details> <summary>ionic-team/stencil (@​stencil/core)</summary> ### [`v4.19.0`](https://togithub.com/ionic-team/stencil/blob/HEAD/CHANGELOG.md#-4190-2024-06-26) [Compare Source](https://togithub.com/ionic-team/stencil/compare/v4.18.3...v4.19.0) ### Bug Fixes * **compiler:** support rollup's external input option ([#3227](stenciljs/core#3227)) ([2c68849](stenciljs/core@2c68849)), fixes [#3226](stenciljs/core#3226) * **emit:** don't emit test files ([#5789](stenciljs/core#5789)) ([50892f1](stenciljs/core@50892f1)), fixes [#5788](stenciljs/core#5788) * **hyrdate:** support vdom annotation in nested dsd structures ([#5856](stenciljs/core#5856)) ([61bb5e3](stenciljs/core@61bb5e3)) * label attribute not toggling input ([#3474](stenciljs/core#3474)) ([13db920](stenciljs/core@13db920)), fixes [#3473](stenciljs/core#3473) * **mock-doc:** expose ShadowRoot and DocumentFragment globals ([#5827](stenciljs/core#5827)) ([98bbd7c](stenciljs/core@98bbd7c)), fixes [#3260](stenciljs/core#3260) * **runtime:** allow watchers to fire w/ no Stencil members ([#5855](stenciljs/core#5855)) ([850ad4f](stenciljs/core@850ad4f)), fixes [#5854](stenciljs/core#5854) * **runtime:** catch errors in async lifecycle methods ([#5826](stenciljs/core#5826)) ([87e5b33](stenciljs/core@87e5b33)), fixes [#5824](stenciljs/core#5824) * **runtime:** don't register listener before connected to DOM ([#5844](stenciljs/core#5844)) ([9d7021f](stenciljs/core@9d7021f)), fixes [#4067](stenciljs/core#4067) * **runtime:** properly assign style declarations ([#5838](stenciljs/core#5838)) ([5c10ebf](stenciljs/core@5c10ebf)) * **testing:** allow to re-use pages across it blocks ([#5830](stenciljs/core#5830)) ([561eab4](stenciljs/core@561eab4)), fixes [#3720](stenciljs/core#3720) * **typescript:** remove unsupported label property ([#5840](stenciljs/core#5840)) ([d26ea2b](stenciljs/core@d26ea2b)), fixes [#3473](stenciljs/core#3473) ### Features * **cli:** support generation of sass and less files ([#5857](stenciljs/core#5857)) ([1883812](stenciljs/core@1883812)), closes [#2155](stenciljs/core#2155) * **compiler:** generate export maps on build ([#5809](stenciljs/core#5809)) ([b6d2404](stenciljs/core@b6d2404)) * **complier:** support type import aliasing ([#5836](stenciljs/core#5836)) ([7ffb25d](stenciljs/core@7ffb25d)), closes [#2335](stenciljs/core#2335) * **runtime:** support declarative shadow DOM ([#5792](stenciljs/core#5792)) ([c837063](stenciljs/core@c837063)), closes [#4010](stenciljs/core#4010) * **testing:** add `toHaveLastReceivedEventDetail` event spy matcher ([#5829](stenciljs/core#5829)) ([63491de](stenciljs/core@63491de)), closes [#2488](stenciljs/core#2488) * **testing:** allow to disable network error logging via 'logFailingNetworkRequests' option ([#5839](stenciljs/core#5839)) ([dac3e33](stenciljs/core@dac3e33)), closes [#2572](stenciljs/core#2572) * **testing:** expose captureBeyondViewport in pageCompareScreenshot ([#5828](stenciljs/core#5828)) ([cf6a450](stenciljs/core@cf6a450)), closes [#3188](stenciljs/core#3188) </details>
Prerequisites
Stencil Version
3.0.1
Current Behavior
When a component is defined with a Host Listener that species a target that isn't the element itself (e.g.
Listen({ target: 'window'})
, if an instance of that component is created but not attached to the DOM then the event listener will not be removed, meaning that the component and the underlying Stencil runtime objects cannot be garbage collected. Overtime a memory/resource leak like this will cause the application to run out of resources.Expected Behavior
The event listener should not be leaked (either it should be removed, or it should never have been created), allowing the GC to release the DOM element and the associated Stencil runtime objects, and therefore not leaking memory.
System Info
No response
Steps to Reproduce
Create a simple component with a host listener targetting the window object
Elsewhere in your code, create an instance of this element, but don't attach it to the DOM
If you now inspect the heap (DevTools > Memory > Heap Snapshot) you will see a "Detached HTMLElement" corresponding to the component. If you are able to go into Stencils internals, then the
hostRefs
WeakMap will also contain entries for this component that will never be released.Reproduction Repository
The reproduction repository provided illustrates the above. The root component creates a child component that has a host listener targetting root once every second. If you run the reproduction app and monitor the "Performance Monitor" tab in Chrome Dev Tools you'll see "DOM Nodes" and "JS event listeners" increase indefinitely. Forcing a manual GC will not bring the numbers down. Examining the heap you'll see an increasing number of Deetached HTMLElements. Examining one of those elements, under "Retainers" you'll see e.g.
elm in system / Context@313813
which is the context for the event listener.Code Reproduction URL
https://github.com/joewoodhouse/stencil-hostlistener-leak
Additional Information
Background
This may seem like a niche issue, however I came across this whilst investigating memory/resource leaks in Ionic to do with overlays. This is all tied to a suite of issues in the Stencil runtime and vdom that cause our application to regularly crash for our users after a moderate amount of use. I've discussed this with @alicewriteswrongs and others elsewhere in #3607
Root Cause and Potential Solutions
HostListeners are attached to the element in the constructor of the CustomElement (
addHostEventListeners
is called byregisterHost
which is called by theconstructor
). However, they are detached in thedisconnectedCallback
of the element, which is not guaranteed to be called.In the above case,
disconnectedCallback
is not called because the element is never connected. However there are other scenarios where both the constructor and connectedCallback can be called but disconnectedCallback is not called (these are other bugs in Stencil that I will raise shortly).The presence of
target
is important to this scenario, because adding event listeners to the element itself (i.e. the default when you don't provide a target) does not create the issue, because the GC will automatically disregard these when destroying the element. It's when the event listener is attached to something else that causes the problem.As CustomElement (and Javascript) have no
destroy
sort of concept (the inverse of the constructor) there is no obvious other place to move the disconnection of the host listeners to.The most obvious fix would be to move the attachment of the host listeners into the
connectedCallback
of the element. However this would obviously introduce a slight change in behaviour - host listeners would only then be "active" when an element is connected to the DOM. This may in fact be the correct/desired/intended behaviour, but I'm not sure.One other thing to note is that the
@Listen
function itself is never called, as the Stencil runtime proxies the events. So in the runtime you'll see HostRefs$queuedListeners$
array will be storing all the incoming events indefinitely (another resource/memory leak).The safest but obvious very breaking change to make would be to remove the
target
option all together, and force the user to do that manually in their user-space connected and disconnected callbacks (or elsewhere).The text was updated successfully, but these errors were encountered: