Move the check for HTMLElement and its subclass from customElements.define to construction timing #541

rniwa · 2016-08-11T02:58:53Z

Right now, CustomElementsRegistry.define is spec'ed to check whether the interface object inherits from HTMLElement interface or not.

This check is hard to implement in WebKit as is because we don't have a mechanism to check the inheritance hierarchy of interface constructor objects (we can check the inheritance hierarchy of constructed objects via branding).

It's also weird to specifically filter out HTMLElement and their sub-interfaces when we throw the same error for things like Range, Text, and Set.

My preference would be moving this check to the construction time. We can check that new.target is not HTMLElement or one of its subclasses instead.

The text was updated successfully, but these errors were encountered:

rniwa · 2016-08-11T03:03:36Z

@travisleithead @annevk @domenic

domenic · 2016-08-12T19:58:29Z

Hmm. So let me try to understand your proposed alternate fix. Consider the following:

<!DOCTYPE html>
<bad-1></bad-1> <!-- (1) -->
<script>
  customElements.define("bad-1", HTMLElement);
  document.createElement("bad-1"); // (2)
  new HTMLElement(); // (3)
</script>
<bad-1></bad-1> <!-- (4) -->

So with your proposal of checking new.target, clearly (3) will throw.

The other cases are less clear, but I think they work... at some point they all call Construct(HTMLElement), which throws:

For (1), that causes the upgrade to fail; the exception will be send to window.onerror
For (2), that causes createElement to synchronously re-throw the exception
For (4), that will cause the parser to fail, thus the parser will instead create a HTMLUnknownElement.

So I guess it works.

Can you also help me understand why this check would be easier to make at construction time, instead of at definition time? In both cases you need to compare a constructor (either C, or new.target) against the list of all constructors. In neither case can you check a constructed object, since no object has been constructed yet. It seems a bit more costly to do the check on every element creation too... That's part of why I think I came up with the currently-specced approach, of just prohibiting these "bad-1" -> HTMLElement associations from ever getting into the custom element registry in the first place.

Mr0grog · 2016-08-12T20:43:10Z

Seeing the above example, it also occurs to me that: a) any given custom element is likely to be used more than once on a page and b) its definition will often be loaded asynchronously, so when @domenic's case (1) occurs, it'll probably occur many times.

That is, if an author passes an invalid constructor to customElements.define() and it doesn’t throw until construction time, they might suddenly see 10 or 20 error events on window show up at once. That doesn’t seem so great.

rniwa · 2016-08-14T01:54:40Z

Can you also help me understand why this check would be easier to make at construction time, instead of at definition time? In both cases you need to compare a constructor (either C, or new.target) against the list of all constructors.

You don't because you can just check that you're constructing yourself during the construction. e.g. let's say you're trying to define a custom element with interface HTMLInputElement. Then in your implementation, either HTMLInputElement has a constructor or it doesn't.

HTMLInputElement has a constructor. In this case, you can simply check that new.target isn't HTMLInputElement in the constructor.
HTMLInputElement doesn't have a constructor. In this case, the attempt to Construct the element immediately fails.

So in either scenario, one doesn't have to enumerate the list of all builtin HTML*Element constructors.

rniwa · 2016-08-14T01:59:19Z

Also, I'm not sure how your check works against Proxy objects. What happens if you defined a Proxy to HTMLElement? Proxy could change its behavior anytime it wants so it seems any check we do during the definition time won't be sufficient.

kojiishi · 2016-08-15T05:44:57Z

+1 to @rniwa, turns out that the current Blink impl skips this check.

domenic · 2016-08-15T15:20:29Z

You don't because you can just check that you're constructing yourself during the construction.

Ah, great, that makes sense! OK, I can change this then...

Also, I'm not sure how your check works against Proxy objects.

I think the new.target would still be the original constructor (when the proxy delegates construction). So it would still fail. But I guess we don't need to worry about it any more.

+1 to @rniwa, turns out that the current Blink impl skips this check.

I hope someone is writing web platform tests for this!! (And the Proxy case is a good one too.) I thought I even saw some from Firefox land in WPT...

rniwa · 2016-08-15T18:43:38Z

Also, I'm not sure how your check works against Proxy objects.

I think the new.target would still be the original constructor (when the proxy delegates construction). So it would still fail. But I guess we don't need to worry about it any more.

My point was that this case is hard/impossible to catch at definition time. e.g. constructor = new Proxy(HTMLElement, {}); Then constructor !== HTMLElement yet it would still construct HTMLElement. So the only timing at which we can catch case like this is at construction time when, as you said, new.target would tell us exactly which object we're trying to construct.

Previously, this would be prevented at definition time. However, that required extra work to know all of the possible element interfaces, and was inconsistent with other failure cases. This moves the necessary check to construction time, which allows it to be simplified. Fixes WICG/webcomponents#541.

Reflects whatwg/html#1674 and WICG/webcomponents#541.

dominiccooney · 2016-08-16T07:25:58Z

I support this proposal, specifically in HTMLConstructor stepts, if new.target === the active function object then throw a TypeError. Given HTMLConstructor step 4.2, which ties together the definition's local name and the active function object's allowable local names, this prevents mayhem like minting elements with unrelated local names or effectively using built-in constructors directly to create elements.

https://bugs.webkit.org/show_bug.cgi?id=161430 Reviewed by Antti Koivisto. Source/WebCore: Per WICG/webcomponents#541, we must throw a TypeError when the HTMLElement constructor is called with new.target set to itself (i.e. new HTMLElement after registering it with a custom element). Note that we can't check this at the time of customElements.define because it could be a Proxy object. Also see: https://html.spec.whatwg.org/#html-element-constructors Tests: fast/custom-elements/CustomElementRegistry.html fast/custom-elements/HTMLElement-constructor.html * bindings/js/JSHTMLElementCustom.cpp: (WebCore::constructJSHTMLElement): Throw a TypeError when NewTarget is HTMLElement constructor itself. LayoutTests: Added test cases for defining a custom element with the HTMLElement constructor, and making sure the HTMLElement constructor throws an exception when newTarget is itself. * fast/custom-elements/CustomElementRegistry-expected.txt: * fast/custom-elements/CustomElementRegistry.html: * fast/custom-elements/HTMLElement-constructor-expected.txt: * fast/custom-elements/HTMLElement-constructor.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@205263 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Previously, this would be prevented at definition time. However, that required extra work to know all of the possible element interfaces, and was inconsistent with other failure cases. This moves the necessary check to construction time, which allows it to be simplified. Fixes WICG/webcomponents#541.

https://bugs.webkit.org/show_bug.cgi?id=161430 Reviewed by Antti Koivisto. Source/WebCore: Per WICG/webcomponents#541, we must throw a TypeError when the HTMLElement constructor is called with new.target set to itself (i.e. new HTMLElement after registering it with a custom element). Note that we can't check this at the time of customElements.define because it could be a Proxy object. Also see: https://html.spec.whatwg.org/#html-element-constructors Tests: fast/custom-elements/CustomElementRegistry.html fast/custom-elements/HTMLElement-constructor.html * bindings/js/JSHTMLElementCustom.cpp: (WebCore::constructJSHTMLElement): Throw a TypeError when NewTarget is HTMLElement constructor itself. LayoutTests: Added test cases for defining a custom element with the HTMLElement constructor, and making sure the HTMLElement constructor throws an exception when newTarget is itself. * fast/custom-elements/CustomElementRegistry-expected.txt: * fast/custom-elements/CustomElementRegistry.html: * fast/custom-elements/HTMLElement-constructor-expected.txt: * fast/custom-elements/HTMLElement-constructor.html: Canonical link: https://commits.webkit.org/179603@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205263 268f45cc-cd09-0410-ab3c-d52691b4dbfc

rniwa added v1 custom-elements labels Aug 11, 2016

domenic self-assigned this Aug 11, 2016

domenic assigned rniwa Aug 12, 2016

domenic mentioned this issue Aug 15, 2016

Move the check on registering an element interface as a custom element whatwg/html#1674

Merged

domenic unassigned rniwa Aug 15, 2016

domenic added a commit to web-platform-tests/wpt that referenced this issue Aug 15, 2016

Remove no-longer valid custom element definition test

a64b7d6

Reflects whatwg/html#1674 and WICG/webcomponents#541.

domenic mentioned this issue Aug 15, 2016

Remove no-longer valid custom element definition test web-platform-tests/wpt#3467

Merged

annevk closed this as completed in whatwg/html@9c9b080 Aug 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the check for HTMLElement and its subclass from customElements.define to construction timing #541

Move the check for HTMLElement and its subclass from customElements.define to construction timing #541

rniwa commented Aug 11, 2016

rniwa commented Aug 11, 2016

domenic commented Aug 12, 2016

Mr0grog commented Aug 12, 2016 •

edited

Loading

rniwa commented Aug 14, 2016 •

edited

Loading

rniwa commented Aug 14, 2016

kojiishi commented Aug 15, 2016

domenic commented Aug 15, 2016

rniwa commented Aug 15, 2016 •

edited

Loading

dominiccooney commented Aug 16, 2016

Move the check for HTMLElement and its subclass from customElements.define to construction timing #541

Move the check for HTMLElement and its subclass from customElements.define to construction timing #541

Comments

rniwa commented Aug 11, 2016

rniwa commented Aug 11, 2016

domenic commented Aug 12, 2016

Mr0grog commented Aug 12, 2016 • edited Loading

rniwa commented Aug 14, 2016 • edited Loading

rniwa commented Aug 14, 2016

kojiishi commented Aug 15, 2016

domenic commented Aug 15, 2016

rniwa commented Aug 15, 2016 • edited Loading

dominiccooney commented Aug 16, 2016

Mr0grog commented Aug 12, 2016 •

edited

Loading

rniwa commented Aug 14, 2016 •

edited

Loading

rniwa commented Aug 15, 2016 •

edited

Loading