Skip to content

Agent's active custom element constructor map can have null registries, causing problems #11256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
AtkinsSJ opened this issue Apr 24, 2025 · 3 comments · Fixed by #11261
Closed

Agent's active custom element constructor map can have null registries, causing problems #11256

AtkinsSJ opened this issue Apr 24, 2025 · 3 comments · Fixed by #11261
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)

Comments

@AtkinsSJ
Copy link
Contributor

What is the issue with the HTML Standard?

With this test case based on https://wpt.live/custom-elements/registries/CustomElementRegistry-upgrade.html :

<!DOCTYPE html>
<some-host id="host">
    <template shadowrootmode="closed" shadowrootcustomelementregistry>
        <a-b></a-b>
    </template>
</some-host>
<script>
customElements.define('a-b', class GlobalABElement extends HTMLElement { });
</script>

Running this with my changes in Ladybird:

  1. "Upgrade an element" step 8 puts a constructor in the map with a null registry.
  2. "HTML element constructors" step 3 says:

    If the surrounding agent's active custom element constructor map[NewTarget] exists:
    And as far as I can tell, "exists" just means it has an entry, and so it's true even if that entry is null.

  3. registry gets assigned to that, so it's null too.
  4. Step 5 of that algorithm then makes use of registry, and explodes.

Changing "HTML element constructors" step 3 to also check for non-null fixes the issue.

I'm not sure if we even should have null registries in that map. In step 1 above, is the element supposed to have a null registry? As far as I can tell I'm assigning the element's registry where I'm supposed to, so I think it's correct. But I have been staring at this for too long so I might be missing something.

cc: @annevk
@AtkinsSJ
Copy link
Contributor Author

Actually this might be related to a different issue I was having: The previous version of https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-element-definition returned null if there was no browsing context, but the new algorithm doesn't. I added that check back in because it was causing problems, but maybe returning null from there is causing the problem here.

@annevk annevk added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Apr 25, 2025
@annevk
Copy link
Member

annevk commented Apr 25, 2025

Nice find, this one took me a while, but I'm pretty sure the problem is that "upgrade particular elements within a document" needs to also accept a CustomElementRegistry and only continue for a particular element if that element's registry is the registry that invoked the algorithm.

(In WebKit this is implemented in enqueueUpgradeInShadowIncludingTreeOrder, which is called from CustomElementRegistry::addElementDefinition, which is called from JSCustomElementRegistry::define, which corresponds to the JS method under discussion.)

annevk added a commit that referenced this issue Apr 25, 2025
@annevk
Fix upgrade particular elements within a document again
607dcc2 
When a CustomElementRegistry define() call starts upgrading elements, we need to make sure to only include those associated with the current registry.

This has test coverage through WPT custom-elements/registries/CustomElementRegistry-upgrade.html.

Fixes #11256.
@annevk annevk mentioned this issue Apr 25, 2025
Merged
@AtkinsSJ
Copy link
Contributor Author

Thanks! That definitely sounds like it would cause problems! It'll be interesting to see if this solves any of my other issues.

annevk added a commit that referenced this issue Apr 28, 2025
@annevk
Fix upgrade particular elements within a document again
5f9f41b 
When a CustomElementRegistry define() call starts upgrading elements, we need to make sure to only include those associated with the current registry.

This has test coverage through WPT custom-elements/registries/CustomElementRegistry-upgrade.html.

Fixes #11256.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix upgrade particular elements within a document again whatwg/html
2 participants
@AtkinsSJ @annevk