Skip to content

HTML parser should not insert nodes the DOM would reject - #69748

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
annevk:eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document
Aug 19, 2026
Merged

HTML parser should not insert nodes the DOM would reject#69748
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
annevk:eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document

Conversation

@annevk

@annevk annevk commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

e8b1d6b

HTML parser should not insert nodes the DOM would reject
https://bugs.webkit.org/show_bug.cgi?id=319780
rdar://183256491

Reviewed by Ryosuke Niwa.

The parser inserts nodes with low-level operations that skip pre-insertion validity
checks, so script that moves a node on the stack of open elements during parsing
could make us build a tree the DOM does not allow.

Use Document::canAcceptChild() when inserting into a Document, which drops Text
node children, a second element child, and a second or misordered DocumentType
child on the floor. This is reachable by foster parenting into a table that script
has made another Document's document element, and by document.open() leaving
children in the Document before the DOCTYPE token or the "html" start tag.

The adoption agency algorithm checked for cycles with Node::contains(), which only
considers the node's own tree, so it missed a parent script had moved into a shadow
root (crashing) or into a template element's content (silently creating an invalid
tree).

Move the nextChild check out of executeInsertAlreadyParsedChildTask() and into
insert(), where nextChild is used. Script can run before a queued insertion takes
place: in the case from bug 169222, parserRemoveChild() destroys an iframe
element's frame and its unload handler removes nextChild from the parent, which is
covered by fast/parser/scriptexec-during-parserInsertBefore.html. Nothing about
that is specific to the adoption agency, though no input is known to reach the
check through a plain insertion.

Because the template element branch runs first and clears nextChild, a stale
nextChild no longer prevents an insertion into a template element's contents. That
is what appropriate place for inserting a node calls for, as it redirects into the
contents after the last child, discarding the reference node.

Spec changes: whatwg/html#12708
              whatwg/html#12709

Tests: imported/w3c/web-platform-tests/html/syntax/parsing/foster-parenting-into-document-with-element-child.html
       imported/w3c/web-platform-tests/html/syntax/parsing/insert-into-nonempty-document.html
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_url.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_write.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_write_single.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-document-target.html
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-shadow-cycle.html
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-template-cycle.html

Canonical link: https://commits.webkit.org/319453@main

72bdea2

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ❌ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 🧪 win-tests loading 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 api-mac-debug
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk3-gcc
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-wk2-stress ✅ 🧪 gtk-wk2
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-intel-wk2 ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🛠 mac-safer-cpp ✅ 🛠 playstation
✅ 🛠 tv ✅ 🧪 mac-site-isolation
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@annevk annevk self-assigned this Jul 19, 2026
@annevk annevk added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Jul 19, 2026
@webkit-early-warning-system

This comment was marked as outdated.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 19, 2026
@annevk annevk removed the merging-blocked Applied to prevent a change from being merged label Aug 18, 2026
@annevk annevk changed the title HTML parser should drop nodes that cannot be inserted into a document HTML parser should not insert nodes the DOM would reject Aug 18, 2026
@annevk
annevk force-pushed the eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document branch from 521994e to 4ab005f Compare August 18, 2026 15:52
@webkit-early-warning-system

This comment was marked as outdated.

@annevk
annevk marked this pull request as ready for review August 18, 2026 15:55
@annevk
annevk requested review from cdumez and rniwa as code owners August 18, 2026 15:55
task.parent = templateElement->fragmentForInsertion();
task.nextChild = nullptr;
} else {
if (task.nextChild && task.nextChild->parentNode() != task.parent) [[unlikely]]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to use containsIncludingHostElements here as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nextChild needs to be a direct child. And insert() only deals with freshly created elements, if that's what you were worried about. (It has ASSERT(!task.child->parentNode());)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 18, 2026
@webkit-ews-buildbot

This comment was marked as resolved.

@webkit-ews-buildbot

This comment was marked as resolved.

@annevk annevk removed the merging-blocked Applied to prevent a change from being merged label Aug 19, 2026
@annevk
annevk force-pushed the eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document branch from 4ab005f to 72bdea2 Compare August 19, 2026 06:31
@annevk annevk added the merge-queue Applied to send a pull request to merge-queue label Aug 19, 2026
https://bugs.webkit.org/show_bug.cgi?id=319780
rdar://183256491

Reviewed by Ryosuke Niwa.

The parser inserts nodes with low-level operations that skip pre-insertion validity
checks, so script that moves a node on the stack of open elements during parsing
could make us build a tree the DOM does not allow.

Use Document::canAcceptChild() when inserting into a Document, which drops Text
node children, a second element child, and a second or misordered DocumentType
child on the floor. This is reachable by foster parenting into a table that script
has made another Document's document element, and by document.open() leaving
children in the Document before the DOCTYPE token or the "html" start tag.

The adoption agency algorithm checked for cycles with Node::contains(), which only
considers the node's own tree, so it missed a parent script had moved into a shadow
root (crashing) or into a template element's content (silently creating an invalid
tree).

Move the nextChild check out of executeInsertAlreadyParsedChildTask() and into
insert(), where nextChild is used. Script can run before a queued insertion takes
place: in the case from bug 169222, parserRemoveChild() destroys an iframe
element's frame and its unload handler removes nextChild from the parent, which is
covered by fast/parser/scriptexec-during-parserInsertBefore.html. Nothing about
that is specific to the adoption agency, though no input is known to reach the
check through a plain insertion.

Because the template element branch runs first and clears nextChild, a stale
nextChild no longer prevents an insertion into a template element's contents. That
is what appropriate place for inserting a node calls for, as it redirects into the
contents after the last child, discarding the reference node.

Spec changes: whatwg/html#12708
              whatwg/html#12709

Tests: imported/w3c/web-platform-tests/html/syntax/parsing/foster-parenting-into-document-with-element-child.html
       imported/w3c/web-platform-tests/html/syntax/parsing/insert-into-nonempty-document.html
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_url.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_write.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/html5lib_write_single.html?file=scripted_foster01
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-document-target.html
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-shadow-cycle.html
       imported/w3c/web-platform-tests/html/syntax/parsing/adoption-agency-reparenting-template-cycle.html

Canonical link: https://commits.webkit.org/319453@main
@webkit-commit-queue
webkit-commit-queue force-pushed the eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document branch from 72bdea2 to e8b1d6b Compare August 19, 2026 13:04
@webkit-commit-queue

Copy link
Copy Markdown
Collaborator

Committed 319453@main (e8b1d6b): https://commits.webkit.org/319453@main

Reviewed commits have been landed. Closing PR #69748 and removing active labels.

@webkit-commit-queue
webkit-commit-queue merged commit e8b1d6b into WebKit:main Aug 19, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 19, 2026
@annevk
annevk deleted the eng/HTML-parser-should-drop-nodes-that-cannot-be-inserted-into-a-document branch August 19, 2026 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DOM For bugs specific to XML/HTML DOM elements (including parsing).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants