From b5e77ee31ab15bb54ee03e2bf430d8104342b9f6 Mon Sep 17 00:00:00 2001 From: Mason Freed Date: Tue, 15 Sep 2020 01:51:39 +0000 Subject: [PATCH] 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] https://github.com/w3c/webcomponents/issues/871#issuecomment-683971426 [2] https://github.com/whatwg/html/pull/5909 Bug: 1042130 Change-Id: Iacf97a49133b5f7f44710e5c0287f01cfebe4c44 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2392975 Reviewed-by: Alexei Svitkine Reviewed-by: Kouhei Ueno Commit-Queue: Mason Freed Auto-Submit: Mason Freed Cr-Commit-Position: refs/heads/master@{#806830} GitOrigin-RevId: 8f4d32caa68b363efdb4cd27821f3d893ff6f464 --- .../mojom/web_feature/web_feature.mojom | 1 + .../v8/script_custom_element_definition.cc | 3 + blink/renderer/core/dom/node.cc | 7 +- blink/renderer/core/dom/node.h | 25 +++---- .../html/custom/custom_element_definition.cc | 8 +-- .../core/html/custom/element_internals.cc | 5 +- blink/renderer/core/html/html_element.cc | 19 +++++ .../html/parser/html_construction_site.cc | 4 +- ...lement-internals-shadowroot.tentative.html | 69 +++++++++++++++++++ 9 files changed, 121 insertions(+), 20 deletions(-) diff --git a/blink/public/mojom/web_feature/web_feature.mojom b/blink/public/mojom/web_feature/web_feature.mojom index 50241098116a..6388c4b4ed10 100644 --- 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 index 763fdc7e82af..82d310fda306 100644 --- 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 index 91b64e8222f7..cef59fe04df7 100644 --- 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 index c5679a7eb046..261c41a49e2b 100644 --- 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 index dbc617f51b18..6c647a44f307 100644 --- 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 index 70f3934c88e4..9f6f846d02a6 100644 --- 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 index 28f1d9a46f24..500eb4e659c6 100644 --- 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 index b7644d32558a..870c7f517624 100644 --- 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/blink/web_tests/external/wpt/shadow-dom/declarative/element-internals-shadowroot.tentative.html b/blink/web_tests/external/wpt/shadow-dom/declarative/element-internals-shadowroot.tentative.html index 0f01cc41acf1..8469a6c1f1ff 100644 --- a/blink/web_tests/external/wpt/shadow-dom/declarative/element-internals-shadowroot.tentative.html +++ b/blink/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'); + + +