fix(browser): warm only the font sources the renderer will consider - #748
Open
ntdatt812 wants to merge 1 commit into
Open
fix(browser): warm only the font sources the renderer will consider#748ntdatt812 wants to merge 1 commit into
ntdatt812 wants to merge 1 commit into
Conversation
prepare_screenshot_resources collected its work with css_resource_urls, which returns every url() in the stylesheet text. A @font-face src list is a priority order, not a set, so the common IE8 form produced six downloads for one font. The renderer already resolves that list correctly. font_face_declaration in obscura-render ends in .last(), so a rule with several src descriptors uses the final one, and font_source_may_be_supported drops .eot and .svg after stripping the query and fragment. The warmup went through neither, so it fetched bytes the renderer had already ruled out. css_font_face_rule applies both rules and reports the block as consumed, the same shape as the existing @import skip in the same scanner. On the reported fixture that is six fetches down to three. The url recording moves into push_css_url so the generic scan and the font path cannot disagree about quoting, fragments, data: or var(). An unterminated block returns None and is left to the generic scanner, matching css_import_rule_len: warming too much is better than dropping the rest of the stylesheet. Fixes h4ckf0r0day#667.
Author
|
Obstacle course, for the CONTRIBUTING pre-PR item a unit test cannot cover. Companion repo at the ref your CI pins (
No delta. The one failure is |
3 tasks
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.
Fixes #667.
What changed
prepare_screenshot_resourcescollected its work withcss_resource_urls, which returns everyurl(in the stylesheet text. A@font-facesrclist is a priority order, not a set of resources, so the IE8 form in the report produced six downloads for one font.The useful part of this is that the engine already resolves that list correctly. Only the warmup did not.
obscura-renderbuilds its font rules as:and those two encode exactly the two rules the warmup was missing:
font_face_declaration, whichfont_face_urlsreadssrcthrough, ends in.last(). A rule carrying severalsrcdescriptors uses the final one. That is the cascade, and it is what thesrc: url(.eot); src: url(...)idiom relies on.font_face_uses_the_last_duplicate_src_descriptoralready pins it.font_source_may_be_supporteddrops.eotand.svgafter stripping the query and fragment, soprobe.eot?#iefixandprobe.svg#probego too.So on the reported fixture the renderer considers three candidates and the warmup fetched six, including the two the renderer had already ruled out.
css_font_face_ruleapplies both rules and reports the block as consumed. That is the same shape as the@importskip a few lines above it in the same scanner, which exists for the same reason: the construct belongs to a different fetch path, so scanning it generically issues the wrong requests.The
url(...)recording moved intopush_css_url, shared by the generic scan and the font path, so the two cannot drift apart on quoting, fragments,data:or an unresolvedvar().An unterminated block returns
Noneand falls back to the generic scanner, matchingcss_import_rule_len: warming too much is better than dropping the rest of the stylesheet.Why three and not one, which is what Chrome fetches
Chrome fetches one because it stops at the first candidate it can use. Here
fetch_and_decode_fontwalks the list and tries the next when a decode fails, which is what makesfont_face_uses_the_first_decodable_sourcepass. Warming only the first would leave that fallback fetching cold on exactly the pages that need it, so this PR narrows the warmup to what the renderer will consider, not to what it will end up using. Going to one is a change to the decode strategy rather than to the warmup, and belongs in its own change if you want it.This also does not touch #662, the other defect in the same function: collecting from the stylesheet text rather than from the render tree. That one is larger and independent, and this does not block it.
Validation
Four tests next to the existing
css_resource_discovery_ignores_strings_comments_data_and_fragments, which still passes unchanged.The first is the reported fixture. Red before the fix, and the red is the report's own number:
The other three cover
local()anddata:sources, a}inside a string in the block, and an unterminated block. Two of them pass onmainas well, because the generic scanner already ignoreslocal(anddata:; they are here to pin the block-skip boundary, which is new code and would silently eat the rule after the block if it were wrong.Full four-configuration sequence from CONTRIBUTING, in a
rust:latestcontainer so the result matches CI rather than my Windows host:cargo build --release -p obscura-cli --bins --features rendercargo nextest run --release --features render --no-fail-fastcargo build --release -p obscura-cli --bins --no-default-featurescargo nextest run --release -p obscura --no-fail-fastcargo nextest run --release --workspace --exclude obscura --exclude obscura-render --no-default-features --no-fail-fastcargo check --release -p obscura-render --no-default-featuresI have not run
cargo fmtover the tree, per CONTRIBUTING.cargo fmt --check -p obscura-browserreports 33 pre-existing diffs, atpage.rslines 1178 through 6936 and incontext.rsandpdf.rs; this PR touches 530-781 and 4392-4474, so none of them are mine.Rendering
No output change, and no fixture is needed: this removes requests for bytes the renderer never decoded. The three sources it does warm are the three
paint.rswould have selected from the same rule, so the font that ends up used is the same one. The render-feature suite is unchanged at 1510 passing, including the@font-faceselection tests inobscura-renderthat own this behaviour.Performance
This is a reduction. On the reported fixture the screenshot warmup issues three requests where it issued six, and the two
.eotfetches it drops are the ones most likely to be large and useless. Nothing is added to a hot path: the new scan runs once per stylesheet, over a block the previous code was already walking character by character, and it walks it once rather than leaving everyurl(to the generic loop.The saving is bounded by how many sources a page's
@font-facerules declare, so a page using a singlewoff2sees no change at all.Checklist