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

Prevent dom-repeat's forwardHostProp from retaining HTMLElement this #5599

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prevent dom-repeat's forwardHostProp from retaining HTMLElement this
TL;DR: The function __ensureTemplatized in elements/dom-repeat.js
caused the templatizeInstanceClass to retain a reference to the
HTMLElement of 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 that `forwardHostProp` will 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/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 `__templatizeOptions` in an object
with constructor `TemplateLegacy`. Line 5: That `TemplateLegacy` object
is the `__proto__` of an object constructed with constructor `klass`.
Line 6: That object, in turn, is the `__proto__` of 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-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.
  • Loading branch information
sigurdschneider committed Oct 22, 2019
commit 922c8b03d57850909974e1f325b547997b7c9799
24 changes: 14 additions & 10 deletions lib/elements/dom-repeat.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,23 +334,27 @@ export class DomRepeat extends domRepeatBase {
}
}

__waitForTemplate() {
// Wait until childList changes and template should be there by then
let observer = new MutationObserver(() => {
if (this.querySelector('template')) {
observer.disconnect();
this.__render();
} else {
throw new Error('dom-repeat requires a <template> child');
}
});
observer.observe(this, {childList: true});
}

__ensureTemplatized() {
// Templatizing (generating the instance constructor) needs to wait
// until ready, since won't have its template content handed back to
// it until then
if (!this.__ctor) {
let template = this.template = /** @type {HTMLTemplateElement} */(this.querySelector('template'));
if (!template) {
// // Wait until childList changes and template should be there by then
let observer = new MutationObserver(() => {
if (this.querySelector('template')) {
observer.disconnect();
this.__render();
} else {
throw new Error('dom-repeat requires a <template> child');
}
});
observer.observe(this, {childList: true});
this.__waitForTemplate();
return false;
}
// Template instance props that should be excluded from forwarding
Expand Down