TL;DR: The function __ensureTemplatized in elements/dom-repeat.js
caused the templatizeInstanceClass to retain a reference to the
HTMLElement (i.e. the DomRepeat object) that triggered creation of the
templatizeInstanceClass (see templatize.js:566).
The function ensureTemplatized in elements/dom-repeat.js contained
an arrow function definition for the MutationObserver and the
function for forwardHostProp in it; the arrow function definition
forces context allocation of this so it can accesses it later. This
introduces false sharing with forwardHostProp: that function refers
to the same context, and transitively also to the this of
`ensureTemplatized, although it neither references nor accesses it. Note thatforwardHostPropwill get it's ownthis` variable on each
invocation). This results in the HTMLElement that originally created
the __ctor to be retained (i.e. not cleaned up by the GC). The
retainment path can be seen in the following heap snapshot:
https://imgur.com/a/javUpRx
In the screenshot, the tab on the top shows the detached HTML element
(the HTMLElement that created the ctor, which I suspect is really
a DomRepeat object inheriting from HTMLElement) and the bottom shows
the retainment chain. The retainment chain is to be read as follows:
Line 1: this is a property of a context. Line 2: that context is
the context of the function forwardHostProp. Line 3: That function
is stored in a property called forwardHostProp of an Object. Line 4:
that Object is stored in a property `templatizeOptionsin an object with constructorTemplateLegacy. Line 5: ThatTemplateLegacyobject is theprotoof an object constructed with constructorklass. Line 6: That object, in turn, is theprotoof an object constructed withklass$jscomp$1. Line 7: that object is a property called__templatizeInstance` on am HTMLElement object. Line 8: that
HTMLElement is an internal property of a ShadowRoot object, and so on.
This CL fixes this problem by moving the arrow function into its own
method __waitForTemplate. This causes the context of
__waitForTemplate to get a context-allocated this, but prevents
the context of __ensureTemplatized from having a context-allocated
this. Unfortunately, it is hard to verify this fix, because
inspecting the function object for forwardHostProp shows all these
context collapsed into 'Local'. Basically, the only way to verify a
fix for an issue like this atm. is to get a reproducible way to
produce a heap snapshot that exhibits the problem, and then verifying
that the problem is not present anymore after applying the fix.
In my repro (navigating between to pages of Gerrit code review tool)
this got rid of about 37 detached HTMLElements.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
TL;DR: The function
__ensureTemplatized
in elements/dom-repeat.js caused thetemplatizeInstanceClass
to retain a reference to the HTMLElement (i.e. the DomRepeat object) that triggered creation of the templatizeInstanceClass (see templatize.js:566).The function ensureTemplatized in elements/dom-repeat.js contained an arrow function definition for the
MutationObserver
and the function forforwardHostProp
in it; the arrow function definition forces context allocation ofthis
so it can accesses it later. This introduces false sharing withforwardHostProp
: that function refers to the same context, and transitively also to thethis
of `ensureTemplatized, although it neither references nor accesses it. Note that
forwardHostPropwill get it's own
this` variable on each invocation). This results in the HTMLElement that originally created the __ctor to be retained (i.e. not cleaned up by the GC). The retainment path can be seen in the following heap snapshot: https://imgur.com/a/javUpRxIn the screenshot, the tab on the top shows the detached HTML element (the
HTMLElement
that created the ctor, which I suspect is really aDomRepeat
object inheriting fromHTMLElement
) and the bottom shows the retainment chain. The retainment chain is to be read as follows: Line 1:this
is a property of a context. Line 2: that context is the context of the functionforwardHostProp
. Line 3: That function is stored in a property calledforwardHostProp
of an Object. Line 4: that Object is stored in a property `templatizeOptionsin an object with constructor
TemplateLegacy. Line 5: That
TemplateLegacyobject is the
protoof an object constructed with constructor
klass. Line 6: That object, in turn, is the
protoof an object constructed with
klass$jscomp$1. Line 7: that object is a property called
__templatizeInstance` on am HTMLElement object. Line 8: that HTMLElement is an internal property of a ShadowRoot object, and so on.This CL fixes this problem by moving the arrow function into its own method
__waitForTemplate
. This causes the context of__waitForTemplate
to get a context-allocatedthis
, but prevents the context of__ensureTemplatized
from having a context-allocatedthis
. Unfortunately, it is hard to verify this fix, because inspecting the function object forforwardHostProp
shows all these context collapsed into 'Local'. Basically, the only way to verify a fix for an issue like this atm. is to get a reproducible way to produce a heap snapshot that exhibits the problem, and then verifying that the problem is not present anymore after applying the fix. In my repro (navigating between to pages of Gerrit code review tool) this got rid of about 37 detached HTMLElements.Reference Issue