Custom elements disconnected from a document should not be upgraded

[rniwa]

The current specification says, in section 6, that:

Each registry has an associated upgrade candidates map of all instances of unresolved elements, mapping a custom element type to a sorted element queue. It is is initially empty.

Whenever an unresolved element is created, it must be added to the respective sorted element queue in upgrade candidates map.

This results in the following code to keep accumulating the list of unresolved elements in the heap:

for (var i = 0; i < 1000000; i++)
    document.createElement('my-element');

I don't think this is an acceptable behavior. I don't think any custom element created prior to its definition getting loaded and not inserted into a document should be upgraded. In particular, any element that's not even referenced by JS and not in any node tree shouldn't be kept alive just for the sole purpose of being upgraded.

[rniwa]

@annevk @travisleithead @dglazkov @domenic

[rniwa]

This also results in rather unexpected behavior of unresolved custom elements inside template content and document without a browsing context getting upgraded. THAT should at least be avoided.

[annevk]

This also results in rather unexpected behavior of unresolved custom elements inside template content and document without a browsing context getting upgraded. THAT should at least be avoided.

That is a different issue and is about what documents the registry works for.

As for this issue, it seems reasonable that the upgrade candidates map holds weak references to the elements. That is, if they are only referenced from there, they should just be collected and not instantiated.

[rniwa]

As for this issue, it seems reasonable that the upgrade candidates map holds weak references to the elements. That is, if they are only referenced from there, they should just be collected and not instantiated.

I initially considered that approach but that could expose GC behavior. For example, WebKit currently implements stack-scanning conservative generational GC. It would mean that if the stack happens to contain a value that matches the node's memory address or if the major GC (non-eden collection) doesn't happen in time before the end of nano-task occurs, we may end up instantiating custom elements that would otherwise be dead in other browsers.

[annevk]

Can't we figure out by the time we invoke the callback whether anyone but "us" holds a reference to the object? That seems like the kind of deterministic check we'd need here.

[rniwa]

We can't because JSC/WebKit uses stack-scanning conservative GC. It's literally impossible for GC to deterministically tell us whether a given node is definitely alive or not.

Note that this is not a problem unique to WebKit. Blink's new memory collector, Oilpan, uses conservative stack-scanning GC as well so they won't be able to implement such a deterministic check unless they modify their GC either.

[annevk]

So let me try to understand the proposal:

1. Nodes inserted into the document that matches their registry, at the time of upgrade, will be upgraded.
2. Will nodes inserted past the time of upgrade, but created before the time of upgrade, still be upgraded, but only upon being inserted? I think if we don't do this we create race conditions.

It seems a little easier to just upgrade all of them and assume developers won't allocate a million of custom elements they don't intend to use.

[rniwa]

Yeah, that's the proposal. The problem is that the current spec says that we need to treat any element with - as a custom element regardless of whether the author has shown the intent to use custom elements or not. As such, any existing content that creates a lot of temporary elements with - in its name and never call defineElement would leak all those elements indefinitely.

[annevk]

Perhaps once the "load" event has fired we should clear the "upgrade candidates map" or some such? And require all elements to be defined from that point on? I wonder what @esprehn @domenic et al think.

[domenic]

I would really prefer not to introduce this kind of inconsistency by requiring document-insertion for upgrade. Developers do complicated and interesting things with elements before inserting them into documents, as part of libraries and such. They shouldn't need to do some kind of insert-then-remove dance to get their elements to have the correct prototype and behavior.

I would prefer to keep the list of unresolved elements as the spec currently has them. It could be made a weak set in the spec, and implementers can either implement that immediately or will wait until they see problems in practice caused by lots of unused un-upgraded elements. I know Blink has implemented such things in the past with just weak references without a problem (we do that for the unhandled promises weak set).

[dglazkov]

Generally agreeing with @domenic here. I understand the concern though. It's just tree-connected concept seems like the wrong tool for the job.

[rniwa]

It could be made a weak set in the spec, and implementers can either implement that immediately or will wait until they see problems in practice caused by lots of unused un-upgraded elements.

As I mentioned in #419 (comment), that kind of optimization will be observably non-interoperable between browsers.

[domenic]

@rniwa you must be misunderstanding me. I am referring to holding a weak reference in an unobservable way, which is certainly possible in Blink with Oilpan. It might not be possible with your GC, which is fine; it just means you cannot implement this optimization.

[rniwa]

@domenic : Oilpan is a stack-scanning conservative GC so it can retain objects that are not actually retained. If you implemented this optimization using Oilpan, it would be observable.

Also, it can't possibly an optimization because it's observable whether a given custom element is upgraded or not. Just because JS no longer has a reference to it, it doesn't mean whether it got later upgraded or not is not. You can create an element, never store the reference, and you can observe that your element got upgraded later. You can't just not upgrade the element just because JS doesn't keep a reference to it.

The proposal @annevk made was about making this weak map mandatory so that any element that loses JS reference and detached from the tree would not be upgraded at the end of current nano-task. I'm pointing out that it's impossible to implement such semantics in WebKit (and Blink if your design doc is still up-to-date).

[domenic]

@rniwa I think I see now. The case in question is when there are no JS consumers holding a reference to the element at all, only the browser holding a single weak reference to it. Let element be such an element. Then:

• If the GC has run and collected the weak reference, nothing will happen when you register x-foo.
• If the GC has not run, and the weak reference still holds a pointer to element, registering x-foo will observably call the XFoo constructor on element to upgrade it.

And then the claim is that our GCs have no way to detect how many non-weak references there are to a given element. (Which seems reasonable, as otherwise they would probably be refcounting systems instead of GCs.)

This does seem problematic.

[rniwa]

Another problem is eden collection versus major collections in (multi) generational GC. Depending on what heuristics are used, you may decide that some elements should not be collected in minor GC even if there was no JS reference to it.

In theory, we should be able to always trigger precise full GC which can deterministically tell us how many weak and strong references we have, but I don't think that's an acceptable implementation requirement for many browser vendors.

[esprehn]

You just need to spec a deterministic time for when the upgrade maps are cleared. Today in blink the references are weak so you could indeed observe the GC by waiting and then doing registerElement at various times. In practice no one has ever complained or noticed.

ex. How useful is upgrade after the load event as fired? Maybe we should turn that system off at that point?

[domenic]

You just need to spec a deterministic time for when the upgrade maps are cleared.

I'm not sure this helps without a way to tell who is referencing the elements at this time, which seems equivalent to triggering a precise full GC at these times.

ex. How useful is upgrade after the load event as fired? Maybe we should turn that system off at that point?

That seems like it would break lazy-loading of sections of pages along with their element definitions.

[esprehn]

You just need to spec a deterministic time for when the upgrade maps are cleared.

I'm not sure this helps without a way to tell who is referencing the elements at this time, which seems equivalent to triggering a precise full GC at these times.

You don't tie to who is holding references. You spec something like "after the load event, clear the upgrade candidate map for all elements". Then you spec some way for authors to keep things alive in the map for longer, for example with waitUntil().

ex. How useful is upgrade after the load event as fired? Maybe we should turn that system off at that point?

That seems like it would break lazy-loading of sections of pages along with their element definitions.

It means you might need to expose something like a waitUntil() promise on the registry. For example document.customElementRegistry.get("x-foo").waitUntil(promise) to deal with your lazy load. You're explicitly making it live in the upgrade map longer.

[domenic]

Hmm. That doesn't seem much better than just not doing upgrades for disconnected nodes, plus maybe adding some kind of document.upgradeElement(x). Authors have to opt-in to making their stuff work either way, instead of being able to just treat all elements the same no matter when/where they are created.

[rniwa]

If being connected to a document was too restrictive, we could add a new flag on element to indicate whether an element is upgradable, and make all elements created by the parser to have that flag turned on by default. e.g. document.createElement('my-element', {upgrade: true}). We could also add a helper function on template element e.g. template.instantiate(), which does template.content.cloneNode(true) and then sets that flag on every single cloned node.

Allowing all disconnected nodes to be upgraded indefinitely in the future without any author intent is not acceptable for us.

[esprehn]

You mean document.importNode(template.contents, true) ? You never want cloneNode, the template document doesn't have a registry.

[rniwa]

It doesn't matter which one. We can define instantiate to do whatever semantics we'd like define it to be since there is exactly one register per global object per issue #369.

[domenic]

After thinking about this some more I am a fan of the following parts of @rniwa's plan:

• add a new upgradeable flag to create an element, which if set and the element is an undefined potentially-custom element, will add it to the upgrade candidates map.
• createElement(NS) gets a new option that lets authors control this flag. upgrade is easier to spell than upgradeable even if it's slightly less accurate, so let's go with upgrade.
• the parser always sets this flag when parsing documents

Unresolved questions:

• Does the parser set the flag when parsing fragments?
• Do we set the flag when cloning nodes?

I think "yes" is probably OK for both of these questions? The undefined potentially-custom elements inside the parsed fragment or cloned tree will have other things referencing them, almost certainly.

We then have the issue of how to provide more ergonomic sugar for after-the-fact upgrading. @rniwa proposed a template-based helper but I think we should explore this space in a bit more detail before settling on that.

I think the lowest-level possible primitive is document.upgradeElement(el) which will perform an upgrade if possible (e.g. if the element was created without the upgradeable flag set), and will add it to the upgrade candidates map otherwise. (Assuming it has a valid custom element name as its local name, and is in the HTML namespace.) Maybe it returns a value to signify what happened. I'm not sure whether this primitive is useful enough to expose, but in extensible web style I'd lean toward exposing it.

A higher-level API might look something like Node.prototype.upgradeDescendants() which will find all children that are potentially upgradeable and perform the document.upgradeElement(el) algorithm on them. This seems a bit more useful for working with document fragments or other nodes that are disconnected but not necessarily part of a template.

Finally if we think that it'll be common to use disconnected <template> elements that are created before appropriate element definitions are loaded, we could have a template.instantiateIn(document) or similar that essentially does a combination of document.importNode(template.contents, true) + result.upgradeDescendants(). I think how useful this is and even how it works is pretty dependent on the two "unresolved questions" above.

---

This is a pretty big issue, so thanks @rniwa for catching it. It's frustrating we can't just do things automatically, and this will definitely hurt the ability for upgrades to "just work" without the developer knowing about them, but it's important to stay deterministic and not expose GC semantics.

[treshugart]

@domenic re:

Upgrades become an in-document concept only.

I take this to mean that if you've loaded your definition and then call document.createElement() that the new element will not be upgraded. Is this correct?

[annevk]

@treshugart in that case the new element would simply be the custom element.

[annevk]

@domenic is there any reason for the upgrade() API to not just take a root? Is this a performance critical operation that cannot traverse descendants in all scenarios?

[andyearnshaw]

This is less developer-hostile than it appears.

This is true. In the future, I'll be able to recommend using import statements to get around this, but our lazy-loaded elements all dispatch an upgraded event already so that developers know when the API is available.

[domenic]

@domenic is there any reason for the upgrade() API to not just take a root? Is this a performance critical operation that cannot traverse descendants in all scenarios?

Two reasons, neither super-compelling:

1. IMO the { deep } option helps us expose the lower-level primitive for people who want to do something fancy, while also making it pretty easy to do the potentially-more-common case. Since this is for rare use cases anyway, allowing more flexibility makes sense to me.
2. It was easier for me to name :). .upgrade(el) upgrading inclusive descendants is weird. Maybe it's OK to have a long name like .upgradeInclusiveDescendants(el) since this is a rarely-used operation, but at that point you're not saving any characters over .upgrade(el, { deep: true }).

[annevk]

If we have { deep }, do we also have { superDeep }, including shadow roots? I wonder what cloneNode() does for elements with shadow roots.

[annevk]

In Chrome it does not clone the shadow roots.

[domenic]

That seems kind of bad, hmm.

[annevk]

Sorry, I thought about it, and it's good. Shadow roots should be created when the element is created, as they are part of the behavior of the element. Since cloning is defined in terms of creation, it should all work out for properly allocated shadow roots.

[domenic]

Just to clarify. Given these two cases:

const el = document.createElement("div");
el.attachShadow();
const clone = el.cloneNode(true);

and

document.customElements.define("x-tag", class extends HTMLElement {
  constructor() {
    super();
    this.attachShadow();
  }
});

const el = document.createElement("x-tag");
const clone = el.cloneNode(true);

the first will have no shadow root in clone, but the second will. And you think that is OK and expected?

I guess I can't think of any better alternative. It seems a bit weird that cloning a tree does not clone the entire tree. And it seems to break the desire to orthogonalize custom elements and shadow DOM. But oh well.

[annevk]

Yeah, otherwise you get weird code paths. Cloning clones the shadow tree, unless the newly created element already has one?

[domenic]

That could work, but is probably a bit too magic.

Maybe @dglazkov or @hayatoito or someone can give us background on why Blink/the current spec decided not to clone shadow trees, and make us feel better about that decision?

[rniwa]

Since the most common use case of shadow DOM is in conjunction with custom elements, it should work best in that use case.

Now, I don't think we can check whether the newly created element has shadow DOM or not because we're going to upgrade those elements right before returning to author scripts. If anything, it needs to be keyed off of element names (i.e. whether the cloned node's name has - in it or not).

[dglazkov]

This will take a while to page in, but I distinctly remember https://bugs.webkit.org/show_bug.cgi?id=61997 as being one of the first places we've discovered that cloning shadow trees along with the tree is fraught with peril.

[domenic]

I've taken care of the majority of this in recent commits. However I haven't added a customElements.upgrade API yet, and have noticed some other issues with upgrades which I'll open a separate issue for.

[domenic]

Since nobody besides me seems enthusiastic about customElements.upgrade(), we can leave it as a v2 feature. I'll close this now. I've added an example illustrating the edge-cases around upgrades and in-document-ness to https://w3c.github.io/webcomponents/spec/custom/#custom-elements-upgrades-examples.