Disallow recursive custom element constructions

With this CL, recursive custom element constructions are no
longer allowed. I.e. this will now only run the constructor once:
  class extends HTMLElement {
    constructor() {
      super();
      customElements.upgrade(this);
    }
  }

Previously, the code and spec had a bug which caused the above
code snippet to infinitely recurse. In [1] the spec has changed,
to set the custom element state to "failed" before the constructor
is called. With this change in place, recursive calls will
early-out at step #2 (of [2]), and avoid the recursion.

[1] https://github.com/whatwg/html/pull/5126
[2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades

Bug: 966472
Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727841}

third_party/blink/renderer/core/dom/element.h

@@ -621,8 +621,11 @@ class CORE_EXPORT Element : public ContainerNode, public Animatable {
   }
 
   bool IsDefined() const {
-    return !(static_cast<int>(GetCustomElementState()) &
-             static_cast<int>(CustomElementState::kNotDefinedFlag));
+    // An element whose custom element state is "uncustomized" or "custom"
+    // is said to be defined.
+    // https://dom.spec.whatwg.org/#concept-element-defined
+    return GetCustomElementState() == CustomElementState::kUncustomized ||
+           GetCustomElementState() == CustomElementState::kCustom;
   }
   bool IsUpgradedV0CustomElement() {
     return GetV0CustomElementState() == kV0Upgraded;

third_party/blink/renderer/core/dom/node.cc

@@ -3148,7 +3148,8 @@ void Node::SetCustomElementState(CustomElementState new_state) {
       break;
 
     case CustomElementState::kCustom:
-      DCHECK_EQ(CustomElementState::kUndefined, old_state);
+      DCHECK(old_state == CustomElementState::kUndefined ||
+             old_state == CustomElementState::kFailed);
       break;
 
     case CustomElementState::kFailed:

third_party/blink/renderer/core/dom/node.h

@@ -102,8 +102,6 @@ enum class CustomElementState : uint32_t {
   kCustom = 1 << kNodeCustomElementShift,
   kUndefined = 2 << kNodeCustomElementShift,
   kFailed = 3 << kNodeCustomElementShift,
-
-  kNotDefinedFlag = 2 << kNodeCustomElementShift,
 };
 
 enum class SlotChangeType {

third_party/blink/renderer/core/html/custom/custom_element_definition.cc

@@ -112,7 +112,7 @@ HTMLElement* CustomElementDefinition::CreateElementForConstructor(
         document);
   }
   // TODO(davaajav): write this as one call to setCustomElementState instead of
-  // two
+  // two.
   element->SetCustomElementState(CustomElementState::kUndefined);
   element->SetCustomElementDefinition(this);
   return element;
@@ -190,21 +190,39 @@ CustomElementDefinition::ConstructionStackScope::~ConstructionStackScope() {
 
 // https://html.spec.whatwg.org/C/#concept-upgrade-an-element
 void CustomElementDefinition::Upgrade(Element& element) {
-  DCHECK_EQ(element.GetCustomElementState(), CustomElementState::kUndefined);
+  // 4.13.5.1 If element is custom, then return.
+  // 4.13.5.2 If element's custom element state is "failed", then return.
+  if (element.GetCustomElementState() == CustomElementState::kCustom ||
+      element.GetCustomElementState() == CustomElementState::kFailed) {
+    return;
+  }
+
+  // 4.13.5.3. Set element's custom element state to "failed".
+  element.SetCustomElementState(CustomElementState::kFailed);
 
+  // 4.13.5.4: For each attribute in element's attribute list, in order, enqueue
+  // a custom element callback reaction with element, callback name
+  // "attributeChangedCallback", and an argument list containing attribute's
+  // local name, null, attribute's value, and attribute's namespace.
   if (!observed_attributes_.IsEmpty())
     EnqueueAttributeChangedCallbackForAllAttributes(element);
 
+  // 4.13.5.5: If element is connected, then enqueue a custom element callback
+  // reaction with element, callback name "connectedCallback", and an empty
+  // argument list.
   if (element.isConnected() && HasConnectedCallback())
     EnqueueConnectedCallback(element);
 
   bool succeeded = false;
   {
+    // 4.13.5.6: Add element to the end of definition's construction stack.
     ConstructionStackScope construction_stack_scope(*this, element);
+    // 4.13.5.8: Run the constructor, catching exceptions.
     succeeded = RunConstructor(element);
   }
   if (!succeeded) {
-    element.SetCustomElementState(CustomElementState::kFailed);
+    // 4.13.5.?: If the above steps threw an exception, then element's custom
+    // element state will remain "failed".
     CustomElementReactionStack::Current().ClearQueue(element);
     return;
   }

third_party/blink/renderer/core/html/custom/element_internals.cc

@@ -328,8 +328,13 @@ void ElementInternals::SetElementArrayAttribute(
 bool ElementInternals::IsTargetFormAssociated() const {
   if (Target().IsFormAssociatedCustomElement())
     return true;
-  if (Target().GetCustomElementState() != CustomElementState::kUndefined)
+  // Custom element could be in the process of upgrading here, during which
+  // it will have state kFailed according to:
+  // https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades
+  if (Target().GetCustomElementState() != CustomElementState::kUndefined &&
+      Target().GetCustomElementState() != CustomElementState::kFailed) {
     return false;
+  }
   // An element is in "undefined" state in its constructor JavaScript code.
   // ElementInternals needs to handle elements to be form-associated same as
   // form-associated custom elements because web authors want to call

third_party/blink/web_tests/external/wpt/custom-elements/pseudo-class-defined.html

@@ -90,4 +90,22 @@ function test_defined(expected, element, description) {
     assert_equals(style.color, expected ? defined : not_defined, 'getComputedStyle');
   }, `${description} should ${expected ? 'be' : 'not be'} :defined`);
 }
+
+test(function () {
+    var log = [];
+    var instance = document.createElement('my-custom-element-2');
+    document.body.appendChild(instance);
+    customElements.define('my-custom-element-2',class extends HTMLElement {
+        constructor() {
+            super();
+            log.push([this, 'begin']);
+            assert_false(this.matches(":defined"), "During construction, this should not match :defined");
+            log.push([this, 'end']);
+        }
+    });
+    assert_equals(log.length, 2);
+    assert_array_equals(log[0], [instance, 'begin']);
+    assert_array_equals(log[1], [instance, 'end']);
+}, 'this.matches(:defined) should not match during an upgrade');
+
 </script>

third_party/blink/web_tests/external/wpt/custom-elements/upgrading.html

@@ -12,6 +12,9 @@
 <script src="resources/custom-elements-helpers.js"></script>
 </head>
 <body>
+<infinite-cloning-element-1></infinite-cloning-element-1>
+<infinite-cloning-element-2 id="a"></infinite-cloning-element-2>
+<infinite-cloning-element-2 id="b"></infinite-cloning-element-2>
 <div id="log"></div>
 <script>
 setup({allow_uncaught_exception:true});
@@ -203,6 +206,53 @@ test(() => {
 }, 'If definition\'s disable shadow is true and element\'s shadow root is ' +
     'non-null, then throw a "NotSupportedError" DOMException.');
 
+test(() => {
+    var log = [];
+
+    customElements.define('infinite-cloning-element-1',class extends HTMLElement {
+        constructor() {
+            super();
+            log.push([this, 'begin']);
+            // Potential infinite recursion:
+            customElements.upgrade(this);
+            log.push([this, 'end']);
+        }
+    });
+
+    assert_equals(log.length, 2);
+    const instance = document.querySelector("infinite-cloning-element-1");
+    assert_array_equals(log[0], [instance, 'begin']);
+    assert_array_equals(log[1], [instance, 'end']);
+}, 'Infinite constructor recursion with upgrade(this) should not be possible');
+
+test(() => {
+    var log = [];
+
+    customElements.define('infinite-cloning-element-2',class extends HTMLElement {
+        constructor() {
+            super();
+            log.push([this, 'begin']);
+            const b = document.querySelector("#b");
+            b.remove();
+            // While this constructor is running for "a", "b" is still
+            // undefined, and so inserting it into the document will enqueue a
+            // second upgrade reaction for "b" in addition to the one enqueued
+            // by defining x-foo.
+            document.body.appendChild(b);
+            log.push([this, 'end']);
+        }
+    });
+
+    assert_equals(log.length, 4);
+    const instanceA = document.querySelector("#a");
+    const instanceB = document.querySelector("#b");
+    assert_array_equals(log[0], [instanceA, 'begin']);
+    assert_array_equals(log[1], [instanceB, 'begin']);
+    assert_array_equals(log[2], [instanceB, 'end']);
+    assert_array_equals(log[3], [instanceA, 'end']);
+}, 'Infinite constructor recursion with appendChild should not be possible');
+
+
 </script>
 </body>
 </html>