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: elements with HostListeners are leaked if element is not attached #4067

Closed
3 tasks done
joewoodhouse opened this issue Feb 17, 2023 · 6 comments · Fixed by #5844 or ionic-team/ionic-framework#29666
Closed
3 tasks done
Assignees
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@joewoodhouse
Copy link
Contributor

joewoodhouse commented Feb 17, 2023

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

import { Component, h, Listen } from '@stencil/core';

@Component({
  tag: 'cmp-a'
})
export class CmpA {

  @Listen('testEvent', { target: 'window' }) testListener(ev: Event) {
    // Does nothing
  }

  render() {
    return <Host/>;
  }
}

Elsewhere in your code, create an instance of this element, but don't attach it to the DOM

const elm = document.createElement('cmp-a')

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 by registerHost which is called by the constructor). However, they are detached in the disconnectedCallback 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).

@ionitron-bot ionitron-bot bot added the triage label Feb 17, 2023
joewoodhouse added a commit to joewoodhouse/stencil that referenced this issue Feb 17, 2023
@joewoodhouse
Copy link
Contributor Author

I've created a unit test that higlights the bug on the following (draft) PR
#4068

@alicewriteswrongs
Copy link
Contributor

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!

Screenshot 2023-03-01 at 2 35 27 PM

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!

@alicewriteswrongs alicewriteswrongs added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label Mar 1, 2023
@ionitron-bot ionitron-bot bot removed the triage label Mar 1, 2023
@johnjenkins
Copy link
Contributor

Great bug report!

The most obvious fix would be to move the attachment of the host listeners into the connectedCallback of the element.… One other thing to note is that the @listen function itself is never called, as the Stencil runtime proxies the events.

My 2 pennies- I can’t think of many use cases where the proxy component needs to listen before being connected?

@sammalloy
Copy link

Hi @alicewriteswrongs, is there any update on this issue?

@alicewriteswrongs
Copy link
Contributor

@sammalloy unfortunately nothing to report atm, when there's a potential fix I will be sure to ping this issue 🙂

@tanner-reits
Copy link
Contributor

A fix for this was included in today's v4.19.0 release!

github-merge-queue bot pushed a commit to ionic-team/ionic-framework that referenced this issue Jun 26, 2024
### Release Notes

<details>
<summary>ionic-team/stencil (@&#8203;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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
6 participants