Restrict attachInternals() to be run inside or after the constructor

Per the discussion at [1], the intention of this change is to prevent calls to attachInternals() prior to the constructor of the custom element having a chance to do so. The spec PR is at [2]. This change is gated behind the DeclarativeShadowDOM flag. With the flag disabled (the default), a use counter is added for checking on the web compatibility of this change. The use counter will measure the cases where attachInternals() is being called in a to-be-outlawed way. [1] WICG/webcomponents#871 (comment) [2] whatwg/html#5909 Bug: 1042130 Change-Id: Iacf97a49133b5f7f44710e5c0287f01cfebe4c44 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392975 Reviewed-by: Alexei Svitkine <[email protected]> Reviewed-by: Kouhei Ueno <[email protected]> Commit-Queue: Mason Freed <[email protected]> Auto-Submit: Mason Freed <[email protected]> Cr-Commit-Position: refs/heads/master@{#806830} GitOrigin-RevId: 8f4d32caa68b363efdb4cd27821f3d893ff6f464
mjfroman · Sep 15, 2020 · b5e77ee · b5e77ee
1 parent 3a0b744
commit b5e77ee
Show file tree

Hide file tree

Showing 9 changed files with 121 additions and 20 deletions.
diff --git a/blink/public/mojom/web_feature/web_feature.mojom b/blink/public/mojom/web_feature/web_feature.mojom
@@ -2794,6 +2794,7 @@ enum WebFeature {
   kCSSContainAllWithoutContentVisibility = 3467,
   kTimerInstallFromBeforeUnload = 3468,
   kTimerInstallFromUnload = 3469,
+  kElementAttachInternalsBeforeConstructor = 3470,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

diff --git a/blink/renderer/bindings/core/v8/script_custom_element_definition.cc b/blink/renderer/bindings/core/v8/script_custom_element_definition.cc
@@ -214,6 +214,9 @@ bool ScriptCustomElementDefinition::RunConstructor(Element& element) {
     return false;
   }
 
+  // 8.1.new: set custom element state to kPreCustomized.
+  element.SetCustomElementState(CustomElementState::kPreCustomized);
+
   Element* result = CallConstructor();
 
   // To report exception thrown from callConstructor()

diff --git a/blink/renderer/core/dom/node.cc b/blink/renderer/core/dom/node.cc
@@ -3232,12 +3232,17 @@ void Node::SetCustomElementState(CustomElementState new_state) {
 
     case CustomElementState::kCustom:
       DCHECK(old_state == CustomElementState::kUndefined ||
-             old_state == CustomElementState::kFailed);
+             old_state == CustomElementState::kFailed ||
+             old_state == CustomElementState::kPreCustomized);
       break;
 
     case CustomElementState::kFailed:
       DCHECK_NE(CustomElementState::kFailed, old_state);
       break;
+
+    case CustomElementState::kPreCustomized:
+      DCHECK_EQ(CustomElementState::kFailed, old_state);
+      break;
   }
 
   DCHECK(IsHTMLElement());

diff --git a/blink/renderer/core/dom/node.h b/blink/renderer/core/dom/node.h
@@ -101,8 +101,9 @@ enum class CustomElementState : uint32_t {
   // https://dom.spec.whatwg.org/#concept-element-custom-element-state
   kUncustomized = 0,
   kCustom = 1 << kNodeCustomElementShift,
-  kUndefined = 2 << kNodeCustomElementShift,
-  kFailed = 3 << kNodeCustomElementShift,
+  kPreCustomized = 2 << kNodeCustomElementShift,
+  kUndefined = 3 << kNodeCustomElementShift,
+  kFailed = 4 << kNodeCustomElementShift,
 };
 
 enum class SlotChangeType {
@@ -970,24 +971,24 @@ class CORE_EXPORT Node : public EventTarget {
     kChildNeedsStyleRecalcFlag = 1 << 16,
     kStyleChangeMask = 0x3 << kNodeStyleChangeShift,
 
-    kCustomElementStateMask = 0x3 << kNodeCustomElementShift,
+    kCustomElementStateMask = 0x7 << kNodeCustomElementShift,
 
-    kHasNameOrIsEditingTextFlag = 1 << 21,
-    kHasEventTargetDataFlag = 1 << 22,
+    kHasNameOrIsEditingTextFlag = 1 << 22,
+    kHasEventTargetDataFlag = 1 << 23,
 
-    kV0CustomElementFlag = 1 << 23,
-    kV0CustomElementUpgradedFlag = 1 << 24,
+    kV0CustomElementFlag = 1 << 24,
+    kV0CustomElementUpgradedFlag = 1 << 25,
 
-    kNeedsReattachLayoutTree = 1 << 25,
-    kChildNeedsReattachLayoutTree = 1 << 26,
+    kNeedsReattachLayoutTree = 1 << 26,
+    kChildNeedsReattachLayoutTree = 1 << 27,
 
-    kHasDuplicateAttributes = 1 << 27,
+    kHasDuplicateAttributes = 1 << 28,
 
-    kForceReattachLayoutTree = 1 << 28,
+    kForceReattachLayoutTree = 1 << 29,
 
     kDefaultNodeFlags = kIsFinishedParsingChildrenFlag,
 
-    // 4 bits remaining.
+    // 3 bits remaining.
   };
 
   ALWAYS_INLINE bool GetFlag(NodeFlags mask) const {

diff --git a/blink/renderer/core/html/custom/custom_element_definition.cc b/blink/renderer/core/html/custom/custom_element_definition.cc
@@ -196,10 +196,10 @@ CustomElementDefinition::ConstructionStackScope::~ConstructionStackScope() {
 
 // https://html.spec.whatwg.org/C/#concept-upgrade-an-element
 void CustomElementDefinition::Upgrade(Element& element) {
-  // 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) {
+  // 4.13.5.1 If element's custom element state is not "undefined" or
+  // "uncustomized", then return.
+  if (element.GetCustomElementState() != CustomElementState::kUndefined &&
+      element.GetCustomElementState() != CustomElementState::kUncustomized) {
     return;
   }
 

diff --git a/blink/renderer/core/html/custom/element_internals.cc b/blink/renderer/core/html/custom/element_internals.cc
@@ -329,10 +329,11 @@ bool ElementInternals::IsTargetFormAssociated() const {
   if (Target().IsFormAssociatedCustomElement())
     return true;
   // Custom element could be in the process of upgrading here, during which
-  // it will have state kFailed according to:
+  // it will have state kFailed or kPreCustomized according to:
   // https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades
   if (Target().GetCustomElementState() != CustomElementState::kUndefined &&
-      Target().GetCustomElementState() != CustomElementState::kFailed) {
+      Target().GetCustomElementState() != CustomElementState::kFailed &&
+      Target().GetCustomElementState() != CustomElementState::kPreCustomized) {
     return false;
   }
   // An element is in "undefined" state in its constructor JavaScript code.

diff --git a/blink/renderer/core/html/html_element.cc b/blink/renderer/core/html/html_element.cc
@@ -1583,6 +1583,7 @@ ElementInternals* HTMLElement::attachInternals(
         "Unable to attach ElementInternals to a customized built-in element.");
     return nullptr;
   }
+
   CustomElementRegistry* registry = CustomElement::Registry(*this);
   auto* definition =
       registry ? registry->DefinitionForName(localName()) : nullptr;
@@ -1592,6 +1593,7 @@ ElementInternals* HTMLElement::attachInternals(
         "Unable to attach ElementInternals to non-custom elements.");
     return nullptr;
   }
+
   if (definition->DisableInternals()) {
     exception_state.ThrowDOMException(
         DOMExceptionCode::kNotSupportedError,
@@ -1604,6 +1606,23 @@ ElementInternals* HTMLElement::attachInternals(
         "ElementInternals for the specified element was already attached.");
     return nullptr;
   }
+
+  // If element's custom element state is not "precustomized" or "custom",
+  // throw "NotSupportedError" DOMException.
+  if (GetCustomElementState() != CustomElementState::kCustom &&
+      GetCustomElementState() != CustomElementState::kPreCustomized) {
+    if (RuntimeEnabledFeatures::DeclarativeShadowDOMEnabled(
+            GetExecutionContext())) {
+      exception_state.ThrowDOMException(
+          DOMExceptionCode::kNotSupportedError,
+          "The attachInternals() function cannot be called prior to the "
+          "execution of the custom element constructor.");
+      return nullptr;
+    }
+    UseCounter::Count(GetDocument(),
+                      WebFeature::kElementAttachInternalsBeforeConstructor);
+  }
+
   UseCounter::Count(GetDocument(), WebFeature::kElementAttachInternals);
   SetDidAttachInternals();
   return &EnsureElementInternals();

diff --git a/blink/renderer/core/html/parser/html_construction_site.cc b/blink/renderer/core/html/parser/html_construction_site.cc
@@ -989,8 +989,10 @@ Element* HTMLConstructionSite::CreateElement(
           document, tag_name, GetCreateElementFlags(), is);
     }
     // Definition for the created element does not exist here and it cannot be
-    // custom or failed.
+    // custom, precustomized, or failed.
     DCHECK_NE(element->GetCustomElementState(), CustomElementState::kCustom);
+    DCHECK_NE(element->GetCustomElementState(),
+              CustomElementState::kPreCustomized);
     DCHECK_NE(element->GetCustomElementState(), CustomElementState::kFailed);
 
     // TODO(dominicc): Move these steps so they happen for custom

diff --git a/...web_tests/external/wpt/shadow-dom/declarative/element-internals-shadowroot.tentative.html b/...web_tests/external/wpt/shadow-dom/declarative/element-internals-shadowroot.tentative.html
@@ -41,4 +41,73 @@
   assert_true(constructed);
 }, 'ElementInternals.shadowRoot allows access to closed shadow root');
 
+test(() => {
+  let constructed = false;
+  const element = document.createElement('x-1');
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(),'attachInternals cannot be called before definition exists');
+  customElements.define('x-1', class extends HTMLElement {
+    constructor() {
+      super();
+      assert_true(!!this.attachInternals());
+      constructed = true;
+    }
+  });
+  assert_false(constructed);
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(),'attachInternals cannot be called before constructor');
+  customElements.upgrade(element);
+  assert_true(constructed);
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(),'attachInternals already called');
+}, 'ElementInternals cannot be called before constructor, upgrade case');
+
+test(() => {
+  let constructed = false;
+  const element = document.createElement('x-2');
+  customElements.define('x-2', class extends HTMLElement {
+    constructor() {
+      super();
+      // Don't attachInternals() here
+      constructed = true;
+    }
+  });
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(),'attachInternals cannot be called before constructor');
+  assert_false(constructed);
+  customElements.upgrade(element);
+  assert_true(constructed);
+  assert_true(!!element.attachInternals(),'After the constructor, ok to call from outside');
+}, 'ElementInternals *can* be called after constructor, upgrade case');
+
+test(() => {
+  let constructed = false;
+  customElements.define('x-3', class extends HTMLElement {
+    constructor() {
+      super();
+      assert_true(!!this.attachInternals());
+      constructed = true;
+    }
+  });
+  const element = document.createElement('x-3');
+  assert_true(constructed);
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(), 'attachInternals already called');
+}, 'ElementInternals cannot be called after constructor calls it, create case');
+
+test(() => {
+  let constructed = false;
+  const element = document.createElement('x-5');
+  customElements.define('x-5', class extends HTMLElement {
+    static disabledFeatures = [ 'internals' ];
+    constructor() {
+      super();
+      assert_throws_dom('NotSupportedError', () => this.attachInternals(), 'attachInternals forbidden by disabledFeatures, constructor');
+      constructed = true;
+    }
+  });
+  assert_false(constructed);
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(), 'attachInternals forbidden by disabledFeatures, pre-upgrade');
+  customElements.upgrade(element);
+  assert_true(constructed);
+  assert_throws_dom('NotSupportedError', () => element.attachInternals(), 'attachInternals forbidden by disabledFeatures, post-upgrade');
+}, 'ElementInternals disabled by disabledFeatures');
+
+
+
 </script>