fix(js): document.write feeds one input stream and inserts at the insertion point - #670
Open
aech wants to merge 2 commits into
Open
fix(js): document.write feeds one input stream and inserts at the insertion point#670aech wants to merge 2 commits into
aech wants to merge 2 commits into
Conversation
appendChild and removeChild report a childList mutation, and appendChild loads a written stylesheet. insertBefore and replaceChild did neither, so an observer on a parent saw a node appear only when it went to the end, and a <link> placed anywhere else never loaded. before(), after() and replaceWith() all route through insertBefore, so they were affected too. The new test walks those plus replaceChild on connected elements, which is the check AGENTS.md asks for after touching a mutation method. Argument order was correct everywhere, only the reporting was missing.
document.write() inserts into the document's input stream. There is one stream and one insertion point per document, and the tokenizer carries its state across calls, so a construct may be split anywhere, even in the middle of a tag name. Obscura parsed every call as a standalone fragment. Anything spanning two calls was lost and stayed in the body as text. The SAP UI5 cachebuster writes exactly that way, one call for the tag, one per attribute, then '>' and the closing tag, so none of its bootstrap parts were ever requested. Obscura parses a document in one pass, so there is no live tokenizer to join. This keeps one parser per document instead, which is what gives the calls a shared tokenizer state. It parses into its own tree and mirrors that into the document as it grows. Only <script> and <template> wait until they are complete, a script because inserting it runs it, a template because its children live in a separate contents document that a child walk never reaches. Which nodes are finished comes from TreeBuilder::trace_handles, not from TreeSink::pop. pop looks like the signal, and its doc says so, but of the three paths that pop the stack of open elements only one reports to the sink; an end tag usually takes pop_until, which does not. Written nodes also went to the end of the body rather than to the insertion point, so a script in the <head> inserted behind everything the parser had already seen. bootstrap.js already tracks the running script in __currentScriptNid, and the point advances with every node placed, across calls too. The host creates the nodes without attaching them and returns where each belongs. bootstrap.js attaches them with appendChild and insertBefore so the insertion steps run. The mirroring walk starts at the last child and stops at the first node already handed over, because the parser only appends. Reading the whole child list per call made the cost grow with the stream: 852 ms against 370 ms over 5000 calls. Interleaved against d9ef40b, same release build, fifteen runs per revision, 5000 operations per case, medians: insertBefore 168 ms -> 168 ms 0.0% replaceChild 299 ms -> 316 ms +5.7% insertBefore w/ observer 163 ms -> 182 ms +11.7% document.write 553 ms -> 557 ms +0.7% The observer case is the only one that adds work. It is the alignment with appendChild, which always reported: on this branch insertBefore with an active observer costs 165 ms, appendChild 160 ms. document.open() drops the stream, since a fresh parse begins.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Fixes #669.
document.write()inserts into the document's input stream. There is one input stream and one insertion point per document. The tokenizer carries its state across calls, so a construct may be split at any point. https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#dom-document-writeobscura parsed each call as a standalone fragment. Anything spanning two calls was lost and remained as text in the body. Details and measurement in the issue.
The second commit brings three parts:
The input stream. obscura parses a document in one pass, so there is no running tokenizer that
writecould attach to. Instead, one parser now lives per document. The shared tokenizer state comes from it. It parses into its own tree, which is mirrored into the document as it grows. Only<script>and<template>wait until they are complete: a script because inserting it executes it, a template because its children live in their own contents document.document.open()discards the input stream.The insertion point. Written nodes went to the end of
body. The standard requires the position of the writing script, otherwise the execution order breaks for every script that is not already at the end.bootstrap.jsalready tracks the running script in__currentScriptNid. The point advances with every inserted node, including across calls.The insertion steps. The host creates the nodes and only reports where they belong. Insertion happens in
bootstrap.jswithappendChildandinsertBefore, because these calls also report the mutation, register window named access, and load a written stylesheet. Reaching into the tree from the host skipped all three.Two independent defects surfaced along the way, both reachable without
document.write.insertBeforeandreplaceChildreported no mutation and did not load a written stylesheet,appendChildandremoveChilddo both. So it is a gap, not intent. Measured on the unmodified state:appendChildreportsSPAN,insertBeforereports nothing.I only found
replaceChildthrough the test thatAGENTS.mdrequires for touching mutation methods:before(),after(), andreplaceWith()all go throughinsertBefore. The test checks them together withreplaceChildon connected elements. The argument order was correct everywhere, the reporting was missing.Dedicated tests:
insert_before_reports_to_mutation_observersandchild_node_methods_place_nodes_and_report_once. If you would rather have the two separately, I will move them into their own PR.Validation
On the Obstacle Course: 32 of 33, and the same 32 on
d9ef40b. The failure isobserver-intersection, reproduced three times per state. The IntersectionObserver fires once instead of firing again after each append, the page reaches 10 of 50 cards. This is independent of this branch. If you get 33/33, let me know, then it is my environment.New tests, all of which failed before:
document_write_joins_an_element_split_across_callsdocument_write_joins_a_tag_name_split_across_callsdocument_write_runs_a_script_split_across_callsdocument_write_inserts_at_the_writing_scripts_positiondocument_write_keeps_call_order_at_the_insertion_pointdocument_write_shows_an_element_that_is_never_closeddocument_write_grows_an_open_element_across_callsdocument_write_reports_to_mutation_observersdocument_write_registers_window_named_accessinsert_before_reports_to_mutation_observerschild_node_methods_place_nodes_and_report_oncebefore/after/replaceWith/replaceChildon connected elements, as AGENTS.md requiresThe five existing
document.writetests stay green, among themcontextual_fragment_and_document_write_keep_executable_script_policy, which guarantees script execution from a single call.Additionally measured against Chromium 151 and against the unmodified state over CDP, same path for all three: open element, growth across calls, MutationObserver, stylesheet, window named access, and
document.close()agree again.Rendering
Not applicable. The change touches neither layout nor paint nor any output surface.
Performance
Measured interleaved against
d9ef40b, identical release build, fifteen runs per revision, 5000 operations per case, time in the page withperformance.now(). Interleaved means alternating old and new, so that machine drift does not leak into the difference. Medians, followed by the range across all runs:insertBeforereplaceChildinsertBeforewith active MutationObserverdocument.writeThe observer case is the only one that adds real work. That is intentional: a notification is now created there that was missing before. The overhead is the alignment with
appendChild, not a new load. On this branch,insertBeforewith an active observer costs 165 ms,appendChildwith an active observer 160 ms, so the same.The ranges overlap throughout. An earlier run on a quieter machine gave -4.1%, -2.8%, +0.9% and -6.6% for the same four cases. Anyone who wants to reproduce the numbers can find the probe in the issue.
The first version was four times slower. The mirroring read the entire child list of the parser tree on every call, so the cost per call grew with the length of the input stream. Across 5000 calls that was 852 ms versus 4130 ms. The walk now starts at the last child and stops at the first node already handed over, because the parser only appends at the end. Its own commit
perf(js): the write stream only walks what is new.The active MutationObserver costs nothing measurable, even though a report now occurs there that was missing before.
Checklist
What this branch does not touch
Parser-blocking scripts. An external
<script src>inserted viadocument.writemust be loaded and executed before the next element gets its turn. In obscura it runs afterwards, because there is no running parser that could be stopped. That touches the loading architecture, no longerdocument.write. The issue describes it with measurements.The practical price is there as well:
me.sap.comstill hits its browser guard even with these commits, although the four bootstrap parts now arrive and sit in the right place.The silent discard.
if (!body) returndiscards a write without a report as long as there is no<body>yet.