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.