Boost: insert deferred scripts only at the document's real closing body tag - #51032
Draft
LiamSarsfield wants to merge 9 commits into
Draft
Boost: insert deferred scripts only at the document's real closing body tag#51032LiamSarsfield wants to merge 9 commits into
LiamSarsfield wants to merge 9 commits into
Conversation
…dy tag Defer JS collects the scripts it moves and re-inserts them in the last output buffer. append_script_tags() did that with a global str_replace() of '</body>', so every textual occurrence in that buffer received a copy of the moved scripts -- including occurrences that are not markup: a literal '</body>' inside a script's source, inside a <textarea>, inside an HTML comment, or inside a quoted attribute value. The injected '</script>' then closed the surrounding script early and the remaining JavaScript rendered as visible page text. Blank out script and textarea bodies and HTML comments before searching, using same-length filler so offsets stay valid, and insert at the last remaining occurrence -- the document's real closing tag. Masking is required rather than simply taking the last raw occurrence, because hosts and plugins sometimes emit markup after </body>. The global replace predates the document.write pinning added in #49545, but pinning newly exposes it: before that change the offending script was moved out of the buffer along with its literal '</body>'. Adds regression coverage for all five contexts plus the trailing-script case.
LiamSarsfield
added
Bug
github-actions
Bot
added
[Boost Feature] Defer JS
Contributor
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
Bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 2 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
Follow-up to the previous commit, applying the findings of two independent
code reviews. The mask-then-search approach stands; these are the cases it
did not cover.
* Fail closed when the region mask fails. A PCRE failure means nothing about
the buffer has been established, so searching the unmasked buffer was the
one path that could still splice the scripts into a JavaScript string. It
now reports no position, and the scripts are appended after the buffer.
* Ignore anything emitted after </html>. Hosts and plugins append markup
there, and a literal '</body>' in it was winning the search.
* Mask <style> and <title> bodies and quoted attribute values as well. A
literal '</body>' in any of them sits in trailing markup often enough to
matter, and inserting into an attribute also terminates it early.
* Mask a leading region whose opening tag was flushed in an earlier chunk.
Output_Filter hands this class a sliding window, so a <textarea>, comment
or script can begin outside it; a closing tag with no opening tag before
it means the buffer starts inside that region.
* Recognise the spec's empty comments ('<!-->', '<!--->') and its end-bang
close ('--!>'), which the comment arm previously ran straight past.
* Return early when there is nothing to insert. Lcp registers a second
Output_Filter on the same global hook, so every request masked the whole
buffer a second time to append an empty string.
Five new tests, and the textarea, comment and attribute fixtures now put
their decoy after the document's real closing tag so the masking, rather
than the ordering, is what they test. Each mechanism above was verified
load-bearing by mutating it and confirming exactly its own test fails.
The 11 pre-existing pipeline fixtures still produce byte-identical output.
…-585) Round-2 review of the previous commit found that its two new safety mechanisms run in the wrong order. `find_body_close_position()` bounded the search at the document's first `</html>` and only then looked for a region whose opening tag had been flushed in an earlier output chunk. When such a region's contents hold `</body></html>` — the natural shape of any pasted HTML sample sitting in a `<textarea>`, a `<style>` block or a comment — the bound deleted the very closing token the second step keys on, so the region's literal `</body>` won the search. That is worse than both trunk and the previous commit on the same page: the deferred bundle lands inside the region and never executes. Resolving the region first fixes it, and the two steps are independent in that direction: the region check only ever blanks a prefix of the buffer. Also from the same reviews: * The region check now recognises `</style>` and `</title>`, so it covers the same element list as the mask it complements. * It fails closed on a PCRE error, like the mask does. `preg_match()` reports failure as `false` and "no match" as `0`; conflating them meant an unresolved buffer was searched as though it were all markup. * The `</html>` bound uses `stripos()`. Tag names are case-insensitive and the mask already is, so a single `</HTML>` used to drop the bound and hand the search to trailing output. * `find_body_close_position()` returns null when `mbstring.func_overload` is rebinding the string functions. Everything it does is byte arithmetic; the base code's `str_replace()` had none and was immune. * Docblock corrections: the appended scripts do not always run (a response cut short mid-region can leave them inert — as on trunk), the function does not copy the buffer only once, and the `</html>` bound comment named contexts the mask does in fact cover. Verification. 20 tests now cover this function; each of the eight mechanisms was mutated in turn and every mutation fails at least one test that names it. Two mutations initially "survived" — one because the runner misread a two-suite run, one because the fixture's script was never moved in the first place; both are corrected here rather than left as coverage. On 200,000 generated buffers built from known contexts the locator returns the real closing tag or nothing, never a third answer (previous commit: 1,935 wrong in 20,000). Ordinary pages are unchanged byte-for-byte against trunk at every output-chunk offset.
…OST-585) Two independent round-3 reviews found routes by which the single inserted copy still lands somewhere it does not run, in each case because the mask answers a question it cannot actually answer from one output-buffer window. This commit narrows what the locator claims to know. The changes, and what each one is for: * Elements the mask does not model — <xmp>, <plaintext>, <noembed>, <noframes>, <noscript>, <template>, <iframe> and CDATA sections — can hold a literal '</body>' between the real closing tag and '</html>', and it wins the search. That class is the one where this PR was worse than trunk: the base code's global replace also hit the real tag, so the scripts ran; here the one copy went into inert content and none of them ran. Pairing those elements in the mask is not reliable (<plaintext> has no closing tag, <template> nests), so instead the chosen offset is rejected when it falls inside one, and the scripts are appended after the buffer, where they run. * The leading-region scan resolved a region opened in a flushed chunk from the first leftover closing token of any type. A '</script>' pasted into a <textarea> is RCDATA, not the end of the region, and treating it as the end reproduced the original bug verbatim. Tokens of more than one type now fail closed. Tokens of one type still resolve it: a region cannot contain its own closing token. * The same scan blanked its prefix unconditionally, so an unpaired token in trailing output — after '</html>', where nothing bounds it — deleted the real closing tag and the bound with it and handed the search to whatever the host emitted. A prefix holding a '</body>' is now inconclusive by definition and fails closed. This also demotes the flush-spanning case the previous commit fixed from a correct insertion to an append; the corruption it fixed stays fixed, and the ambiguity is no longer resolved by guesswork. * Quoted attribute values were matched as free-floating '="…"' / '=\'…\''. That under-reached (HTML allows whitespace around '=', so `data-x = "…"` was searchable) and over-reached (an unbalanced quote in ordinary prose started a match that ran to the next quote anywhere in the buffer, taking the real closing tag with it). Both directions are fixed by replacing the two arms with one anchored opening-tag arm. * '</script foo>' and '</script/>' close a script in a browser; the mask's '</script\s*>' did not agree, so such a script's body was searched as markup. The closing-tag syntax is now shared by every arm, as is the element list, which had already drifted apart once between the mask and the scan. * The '</html>' bound was a literal, so '</html >' silently dropped it. * The phpcs suppression on the mbstring.func_overload guard named only the PHP 7 deprecation code; the PHP 8 development ruleset reports a different one, which is why that required check was red. * The locator's docblock claimed every unresolvable case returns null and that prefix-blanking never yields a wrong position. Both were false as written, and the second was the stated justification for the previous commit's ordering. Corrected rather than re-asserted. Verification, and where it is weaker than the previous commit claimed: * 211 tests, 644 assertions. New coverage for each unmodelled element driven through the real Output_Filter chunking, every legal spelling of a quoted attribute and of a raw-text closing tag, '</html >', the mixed-token and trailing-token cases, and an unbalanced quote in page text. * Eight mutations, one per mechanism, each killed by a test that names it. The previous commit's claim that this held for all of its mechanisms was false for three of them: the mask's fail-closed branch was covered only by a test whose backtrack limit tripped a sibling guard, the single-quoted attribute arm had no test, and the mbstring guard has none and still has none — it cannot be reached without a per-directory ini change. The first two are now covered, the two PCRE failure paths are isolated from each other, and the third is stated rather than claimed. * A 40,000-buffer corpus over these contexts: no wrong offsets, against 9,305 for the previous commit on the same input. Eleven ordinary pages are byte-identical to trunk at every chunk offset. * Cost: about 1.4x the previous commit on a 34 KB buffer, 0.13 ms with PCRE JIT on and 0.55 ms with it off. The quadratic behaviour on a buffer full of unterminated openers with JIT disabled is unchanged in shape and about 25% faster in absolute terms; it is not fixed here. Not addressed, deliberately: carrying the tokenizer state across Output_Filter ticks, which is what would let the scan know which region opened instead of inferring it, and a WP_HTML_Tag_Processor-based locator. Both are rewrites rather than follow-up lines, and neither belongs in a regression fix.
…roup PHP 7.2 and 7.3 report a capture group that did not take part in a match as a plain empty string rather than the [value, offset] pair the OFFSET_CAPTURE flag produces for groups that did, so indexing it raised "Uninitialized string offset: 1" and the required PHP 7.2/7.3 test jobs failed. It surfaced on the CDATA fixture, where the element-name group never participates; the leading-region scan had the same latent shape dependency for a comment close. Both scans now derive what they need from the matched text, which is the same on every supported version, and the groups are non-capturing. Adds the comment-close case to the suite, which is the shape the second site would have raised on.
Without CoversClass metadata the coverage report attributes none of this class's lines to the suite that exercises them, so every line the fix adds counts against the file and the coverage check reports a drop the tests do not have. Other Boost test classes already carry the same declaration.
This reverts commit 15fc5c2. The declaration did not change this file's measured coverage (134/265 both before and after) and dropped class-output-filter.php from 12/13 to 0/13, because scoping the class's coverage to Render_Blocking_JS excludes the Output_Filter lines the same tests drive. The Brain Monkey suite contributes no coverage to this file either way, so the metadata only costs.
…ving up Round-5 review findings. Every change here is verified against a corpus of trailing-output shapes (a decoy between the document's own closing tag and </html>), which is the slot all of them share: before after nested <template> WRONG -> CORRECT <plaintext> APPEND -> CORRECT </iframe> with a \x0b WRONG -> CORRECT closed <iframe> APPEND -> CORRECT closed <template> APPEND -> CORRECT closed CDATA APPEND -> CORRECT <iframe-widget> APPEND -> CORRECT The unmodelled-element backstop searched for a closing tag once per opener and took the first one it found. That both cost a buffer scan per opener and got the two elements its own docblock names wrong: <template> nests, and <plaintext> never closes. It is now one ordered pass that collects the spans with the tokenizer's own rules — a raw-text element swallows tags until its own close, <template> is tracked by depth, and anything still open at the end of the buffer runs to the end of it, which is what makes <plaintext> fall out for free rather than needing a case of its own. Rejecting a candidate no longer ends the search. Nothing about one occurrence says anything about the one before it, and on trailing output holding a decoy the next candidate left is the document's own. A retry can only move the answer towards the start of the buffer and every candidate it reaches is checked the same way, so it cannot turn an append into a wrong offset. On the second fuzz corpus this recovers placement on 3,009 of 20,000 buffers (6,620 -> 9,629 correct) with the wrong-offset count still zero. Two paths scaled quadratically and both were reachable from author-controlled content, a post title among it: 4,000 unmatched <title> openers, 32 KB 335 ms -> 0.39 ms 16,000 nested <template> pairs, 352 KB 2.2 s -> 17 ms The first is capped rather than fixed: each lazy raw-text arm of the mask costs a scan to the end of the buffer at every unmatched opener, so past a small budget the buffer is not searched at all. The second is gone, being the same per-opener loop the region walk replaces. Also: - PCRE counts a vertical tab as whitespace and HTML does not, so the shared closing-tag grammar spells the five HTML whitespace bytes out. Reading '</iframe\x0b>' as a closing tag ended a region the browser is still inside. - A word boundary succeeds before a hyphen, and a custom element's name must contain one, so '<iframe-widget>' and '<script-x>' were read as the elements they are named after. Now guarded with a negative lookahead. - The leading-region scan returns an offset instead of a rewritten buffer. Same answers, one less full-buffer copy, and the '</html>' bound now starts its search at that offset rather than relying on the region having been blanked. - The docblock and the changelog claimed the appended scripts still run. That is false after an unterminated <plaintext>, whose tokenizer state has no exit before end of file. Both now say appending leaves the markup alone, which is the property that actually holds. - Merged the two mask-failure tests that exited through the same branch, keeping the one that isolates it and the assertion the other one carried. Fifteen mutations, one per mechanism, each killed by a test that names it. 228 tests / 687 assertions. Identical results on PHP 7.2, 7.3, 7.4, 8.0, 8.3, 8.4 and 8.5. 44/44 pipeline fixtures byte-identical to both trunk and the previous commit. Both fuzz corpora report zero wrong offsets. Five wrong-offset classes remain, all in the same trailing slot and all sharing one root: the mask enumerates the token shapes that are not markup, and that list cannot be completed. Closing the class needs a tokenizer, which is a design change rather than another arm.
…hat bound the work Round-5 review found three defects and one of them is a corruption this PR introduced. All three come down to counting or pairing tokens without the rule that makes the count mean anything. 1. unmasked_regions() popped the <template> depth on any closing tag, so a stray </iframe>, </xmp>, </noembed>, </noframes> or </noscript> ended the region early and offered a </body> in template content as an insertion point. Scripts written there do not run and the template is corrupted. Only a </template> pops the depth now. 2. The region list was collected first and rescanned per candidate, which is quadratic in candidates x regions: 16,000 <template> regions in 480 KB cost 2,119 ms. The candidates are collected in the same ordered pass as the regions that disqualify them, so the search is linear and the same input costs 25 ms. unmasked_regions() and offset_in_regions() are replaced by body_close_position(), which is also smaller than the two of them. 3. unclosed_raw_text_openers() subtracted one unordered total from another, so a closing tag of another element, one arriving before any opener, and one spelled </titlex> each cancelled an opener that the mask leaves unpaired. The budget then never fired on the buffers it exists for: 245 KB of KSES-clean post content shaped that way cost 445 ms and reported a count of zero. Pairing per element, in document order, against the mask's own closing tag grammar reports 7,000 and costs 3 ms. The same round showed the PR losing to trunk on a class it inherits rather than introduces. When Output_Filter has flushed the opening tag of an <iframe>, <xmp>, <noembed>, <noframes>, <noscript> or <template>, the window has no way to know the region is open, and a literal </body></html> in its contents reads as the end of the document. Trunk's str_replace left a second, runnable copy at the real closing tag; this locator moves the only copy into content a browser never runs, so the page's deferred JavaScript does not execute at all. leading_region_end() already resolves this for the elements the mask can pair, by reading a leftover closing tag as evidence that the region opened before the buffer. It now reads the other elements' closing tags the same way. They are not masked, so both of their tags survive and a leftover one has to be identified by pairing rather than by survival — pairing them as the totals did would fail closed on ordinary markup and cost 51% of the placements on the adversarial corpus. The '</html>' bound is passed to the scan rather than cut out of the buffer for the same reason: a '</html>' inside such a region would otherwise take the closing tag that is the only evidence of it. Also: the region walk no longer carries the claim that a retry cannot turn an append into a wrong offset, which round 3 and round 4 both flagged and which was false; the loop it described is gone. A comment naming a helper deleted last round now names its replacement. The changelog says that a cached page keeps serving the broken output until the cache is purged. Verified: 256 tests / 757 assertions and 101 / 495 green. 14 mutations, one per mechanism, all killed by a test that names it. Byte-identical output to trunk and to the previous commit on 44 ordinary pages x chunk offsets. Both fuzz corpora unchanged at 12,518 and 9,629 correct placements, 0 wrong offsets. Identical results on PHP 7.2, 7.3, 7.4, 8.0, 8.3, 8.4 and 8.5. phpcs and PHPCompatibility clean, phan 0 issues. 34 KB buffer 0.143 ms. Honest accounting: the file is one code line larger than last round, not smaller. The merged walk removed about 45 lines and the two pairing fixes added about the same back. Known and unfixed: five wrong-offset classes, all needing a decoy between the document's own </body> and </html> — unterminated raw text, an end tag with a quoted attribute, a bogus declaration, a bogus processing instruction, and a quote in an unquoted attribute value. A region whose opening tag was flushed and whose closing tag never arrives is still unresolvable from the window.
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
1 participant
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.
Fixes #
Proposed changes
Defer Non-Essential JavaScript collects the scripts it moves and re-inserts them in the last output buffer.
append_script_tags()did that with a globalstr_replace()of</body>, so every textual occurrence in that buffer received a copy of the moved scripts — including occurrences that are not markup:</body>inside a script's source (e.g. an HTML string adocument.write()call later emits)<textarea>(RCDATA)The injected
</script>then closed the surrounding script early, so the rest of that script's JavaScript rendered as visible page text and the script's real work never ran.</body>only.<script>,<textarea>,<style>and<title>bodies, HTML comments and opening tags — quoted attribute values included — are blanked out with same-length filler before the search (so byte offsets stay valid for the original buffer), then the last remaining occurrence is used and the tags are inserted withsubstr_replace().</body>, and a literal</body>in such trailing output would otherwise still be chosen. For the same reason the search stops at the document's first</html>.Output_Filterhands this class a sliding window, so a region can open in a chunk that was already flushed. A closing token left over after the mask means exactly that. Which region opened cannot be answered from the window alone, so that inference is now limited to what the token types actually establish: one type resolves it (a region cannot contain its own closing token), more than one does not, and a prefix that holds a</body>of its own is inconclusive either way.<plaintext>has no closing tag,<template>nests — so their spans are collected in one ordered pass with the tokenizer's own rules (a raw-text element swallows tags until its own close,<template>is tracked by depth, anything still open when the buffer ends runs to the end of it) and a chosen offset falling inside<xmp>,<plaintext>,<noembed>,<noframes>,<noscript>,<template>,<iframe>or a CDATA section is discarded.mbstring.func_overloadrebinding the string functions, a buffer holding more unclosed raw-text openers than the mask will scan, no closing tag in the buffer — all return null, and the scripts are appended after the buffer rather than any existing markup being rewritten.<title>openers in 32 KB took 335 ms before the cap and 0.39 ms after. The second such path — one closing-tag search per unmodelled opener — is gone with the loop the region walk replaces, taking 16,000 nested<template>pairs in 352 KB from 2.2 s to 17 ms.data-jetpack-boost="ignore"-marked script, trailing markup both before and after</html>, a region opened before the buffer starts, every legal spelling of a quoted attribute value and of a raw-text closing tag,</HTML>and</html >, an unbalanced quote in page text, both PCRE-failure fallbacks in isolation from each other, every unmodelled element driven end-to-end through the realOutput_Filterchunking rather than a hand-made window, nested<template>, a tag inside a raw-text element, an unclosed inert element, a vertical tab in a closing tag, custom elements named after modelled ones, and a buffer past the opener budget.Notes for review:
document.writepinning added in Boost: keep position-dependent inline scripts (document.write) in place when Defer JS is enabled #49545. Pinning newly exposes it without any author opt-in — before that change the offending script was moved out of the buffer along with its literal</body>.append_script_tags()is hooked tojetpack_boost_output_filtering_last_buffer, so it normally only sees the final joint buffer (the last two output chunks, ~8 KB). Corruption therefore required the offending literal to land in that tail, which is why the reported site saw it on only two pages.</BODY>and body-less fragments — those still fall through to append-at-end exactly as before.</body>and</html>holds a decoy. The mask enumerates the token shapes that are not markup, and that list cannot be completed by adding arms: an unterminated raw-text opener, an end tag carrying a quoted attribute, a bogus declaration (<!bogus … >), a bogus comment (<? … ?>) and a quote in an unquoted attribute value are all still searched as markup. Separately, an unescaped</body>in a plain text node is an end tag to the HTML parser too, so it is correctly not masked. And the region scan still infers rather than knows, because its input is a tail of a byte stream. Closing the class needs a tokenizer — either carrying state acrossOutput_Filterticks or aWP_HTML_Tag_Processor-based locator — which is a design change rather than a follow-up line. Appending is not an executable fallback in every case either: after an unterminated<plaintext>the tokenizer state has no exit before end of file, so the appended scripts are text. The changelog therefore claims only that appending leaves existing markup alone.Code coverage requirementcheck is red and I could not move it. The report attributes no coverage at all to this file from theunitsuite — the one that holds every test in this PR — so each added line counts as uncovered no matter how many tests exercise it:find_body_close_position()reads 4/27 lines covered while 40 tests drive it, and the annotated report marks lines as unexecuted that cannot be. Declaring#[CoversClass]on the test class changed this file's number not at all and droppedclass-output-filter.phpfrom 12/13 to 0/13, so that was reverted. Getting a real number here means moving these tests to thewith-wordpresssuite, which is a test-harness change rather than part of this fix.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Automated:
jp test php plugins/boost— 228 tests, 687 assertions. 58 tests cover the insertion point; the 11 pre-existing pipeline fixtures produce byte-identical output before and after, on both the single-chunk and split-chunk paths. Results are identical on PHP 7.2, 7.3, 7.4, 8.0, 8.3, 8.4 and 8.5.jp phan plugins/boost— 0 issues.Manual:
?cache-bust=1) to force a cache miss.popupHtmlandmovable sibling.Before this change: the
popupHtmlstring has a<script>tag injected inside it, themovable siblingscript appears twice, and part of the JavaScript is visible as page text.After this change: the
popupHtmlstring is byte-for-byte what you entered,movable siblingappears exactly once immediately before the real</body>, and no JavaScript is visible on the page.<textarea>, in an HTML comment, and in adata-attribute — each should also come through untouched.<textarea>holding a pasted HTML sample (one containing</body></html>) more than 8 KB above the end of the page. The sample must come through verbatim; the moved scripts are appended after the page rather than before</body>on this shape, and must still run.<iframe>or<template>after the page's</body>with a literal</body>inside it. The element must come through verbatim and the moved scripts must sit at the page's own</body>, not after the page.Note for anyone verifying against a previously-affected site: corrupted HTML may already be stored in Boost's Page Cache (and in the host's cache), so purge caches before re-testing, and keep any existing Defer JS URL exclusions in place until this ships.