Potential Memory Leak?

[edbaafi]

Hi @calebdwilliams!

Thanks for the important polyfill! Rather than a confirmed bug report, this is more of a question about a potential memory leak in the current implementation.

I have only done a cursory review of the codebase so please correct me if I'm wrong, but it seems the following is true:

1. a Location object holds a strong reference to a Document or ShadowRoot
2. $locations in ConstructedStyleSheet.ts is a WeakMap where keys are instances of ConstructedStyleSheet and values are arrays of Location instances
3. addAdopterLocation pushes a Location object (including its strong reference to a Document or ShadowRoot) to the Location array that is mapped in $locations (such mapping from the key of the specific ConstructedStyleSheet adopted by the same Document or ShadowRoot strongly referred to by the Location object being pushed to the array)
4. addAdopterLocation is called when a stylesheet is initially adopted (or when any 'adopted' style node is removed)
5. removeAdopterLocation is the only place that the Location objects (and their strong reference to a Document or ShadowRoot) are removed from the array that is mapped in $locations
6. removeAdopterLocation is called when a ConstructedStyleSheet is removed from a Document or ShadowRoot's adoptedStylesheets but is not called in any other case

Based on the above, it seems that in the following scenario we would have a memory leak:

1. Custom element instance 1 is constructed and a ConstructedStyleSheet is constructed and adopted by Custom element instance 1's shadowRoot
2. Custom Element instances 2 - 1,000,000 are constructed and added and then later removed from the DOM, where each of custom element 2 - 1,000,000's constructors cause their shadow roots to adopt the same ConstructedStyleSheet from step 1

As a WeapMap's values are strongly held until the key is referred to only by the WeakMap itself, if Custom element instance 1 is still strongly held in the DOM after instances 2 - 1,000,000 are no longer held in the DOM or referenced by anything but this polyfill, it would seem that this polyfill would cause instances 2 - 1,000,000's not to be garbage collected. This is because each of instances 2 - 1,000,000's shadow root will still be referenced by the Location objects in the array that is the value of the $locations entry with the key of the ConstructedStyleSheet still adopted by custom element instance 1's shadow root. And because shadowRoot refers to the element it is attached to with the ShadowRoot.host property, the element itself will not be garbage collected.

If this is the case, this would seem to be a serious issue as adopting a StyleSheet when a custom element is constructed is a major use case for adopted stylesheets and constructable stylesheets so that all instances of a custom element constructor can share a single set of styles. At the very least a warning not to use this polyfill with short lived custom elements may be in order.

Please forgive me if I've missed something or if any of this is unclear. Also, unfortunately it doesn't seem we can force garbage collection so a test case where memory is not being garbage collected doesn't seem to be proof of a memory leak so we seem to be forced to report potential issues with analysis like the above: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management

[edbaafi]

Update: I ended up rolling my own polyfill for my use case so didn't revisit this. But I just re-read this issue and remembered that I later learned about and used the Edge browser's heap snapshots and its forced garbage collection feature to debug similar issues in other codebases. Just wanted to point that out in case someone who relies on this polyfill wants to replicate the above steps and determine if this is in fact an issue.