diff --git a/FIDELITY.md b/FIDELITY.md index 4aa0721..1d20521 100644 --- a/FIDELITY.md +++ b/FIDELITY.md @@ -18,6 +18,16 @@ The committed PNGs under `testdata/renders/` back every claim here. Reproduce them with the commands at the bottom. The measured-vs-Chrome numbers live in [`bench/REPORT.md`](bench/REPORT.md). +## 2026-10-07 (round 165) — a 500-page bench sweep found a real renderer crash: `transform:rotate()` on a box with an overflowing descendant wrote past its own offscreen buffer (engine#263, patch) + +Building a 500-page corpus (268 distinct sites, at most four pages each, round 164's own planned expansion) and running the bench against it crashed on page 21: `https://www.smashingmagazine.com/2018/10/mobile-app-retention-rate/`, with `panic: runtime error: index out of range [6339531] with length 6336512` inside `blendPixel`, reached via `blitImage` → `paintItem` → `paintRotated`. Reproduced directly with `cmd/render` against the live URL, no bench harness involved — a real, user-facing crash, not a test artefact. + +- **Root cause**: `paintRotated` (round 145) sized its offscreen buffer (`tmp`) using only the rotated box's own border box (`rectOf(box)`), trusting it to bound everything `paintBoxContent`'s recursion could write into that buffer. But a descendant can paint outside its ancestor's own `Y+H` — a shrink-wrapped float, a negative margin, a marker, an outset box-shadow — exactly the case `subtreeExtent` (the filter/opacity/mask-image group path's own ink-bounds helper, already in this file) exists to catch. `paintRotated` never used it. `blitImage`'s own per-pixel write (`blendPixel` → `dst.PixOffset` → `dst.Pix[i]`) has no bounds check beyond the ancestor clip rectangle, which is NOT scoped down to `tmp`'s own smaller height — so a descendant placed between the box's own bottom and the real canvas's bottom silently wrote past `tmp`'s allocation and crashed the whole render. +- **Fixed**: `tmp`'s height now comes from `subtreeExtent(box).Intersect(dst.Rect).Max.Y` instead of `bx.Max.Y` alone. `subtreeExtent`'s own base rectangle already covers the box's full border box unconditionally, so the result is provably never shorter than before (verified by reasoning, not just testing: `bx ⊆ dst.Rect` and `bx ⊆ subtreeExtent(box)`, so their intersection already contains `bx`) — no separate floor/clamp needed, and the coverage gate confirmed that by flagging the clamp branch I initially wrote as dead code, which was deleted rather than tested around (round 143/144's own established practice). The final rotated image's own disclosed scope (content outside the box's own border box is not captured into what gets rotated) is unchanged — only the OFFSCREEN BUFFER'S allocation grows to stop the crash; what ends up in the visible output does not. +- **Test**: `TestRotateDoesNotCrashWhenAChildOverflowsTheBoxBottom`, a hand-built box tree with a rotated box whose child's own border box extends below the rotated box's own bottom, painting an opaque inline image (routed through the exact `paintItem → blitImage → blendPixel` path the real crash trace showed — a transparent test image would have silently skipped the vulnerable write via `blendPixel`'s own zero-coverage short-circuit, which is what made my first two attempts at this test pass even against the unfixed code; caught by deliberately stash-verifying against the OLD code and seeing it ALSO pass, the same "a passing test proves nothing until it fails the right way" discipline this session has used before). Stash-verified: reverting `paint.go` alone reproduces the real panic, with the identical `paintBox → paintBoxContent → blendPixel` stack. +- **Coverage**: `paint` initially dropped to 99.9% on the clamp branch described above; fixed by deleting the unreachable code, restoring the 100% floor on the first attempt after that correction — not by adding a test for a branch that cannot execute. +- **Corpus**: `bench/urls-100.txt` (100 pages) and `bench/urls-500.txt` (500 pages, 268 sites, ≤4/site) are new, built from each site's own sitemap and checked for a live HTTP 200 before inclusion, extending the original ten-page `urls.txt`. This entry is the crash fix the first 500-page run surfaced on page 21, before a single full run completed; the corpora's own measured numbers (once a full run completes without crashing) are a separate, later entry. + ## 2026-10-04 (round 155) — each Wikimedia 429 costs about 2s of wall time per article render (its Retry-After is 1s, plus queueing); pacing requests removes the 429s but makes renders slower (docs only, no behaviour change) Round 154 showed the 429s cost no fidelity. This round measures what they cost in time. Ten renders of the same saved en.wikipedia.org article: runs with no 429s take about 0.85s; runs with three take 2.9–3.1s; a run with five takes 4.0s. So each rate-limited response adds about 2s to the page, far more than the font-loading gain in round 151. Every 429 carried `Retry-After: 1`, which `images.go`'s retry honours, and the rest is queueing behind the other in-flight requests. diff --git a/paint/paint.go b/paint/paint.go index f2b9505..dedd99c 100644 --- a/paint/paint.go +++ b/paint/paint.go @@ -1660,7 +1660,23 @@ func paintRotated(dst *image.RGBA, box *layout.Box, f *Fonts, imgs map[*dom.Node if bx.Empty() { return } - tmp := image.NewRGBA(image.Rect(0, 0, dst.Rect.Dx(), bx.Max.Y)) + // tmp must be tall enough for everything paintBoxContent's recursion can + // actually write to, not just box's own border box: a shrink-wrapped + // float, a negative margin, a marker or a box-shadow can place painted + // content below box.Y+box.H (subtreeExtent's own doc comment, already + // relied on by the filter/opacity/mask-image group path below) even + // though none of that reaches the FINAL rotated image (src, two lines + // down, still crops to bx alone — the disclosed border-box-only scope + // above is unchanged). Confirmed live: smashingmagazine.com crashed + // blendPixel with an out-of-range index because tmp was undersized by a + // few rows — a real out-of-bounds WRITE, not merely a cosmetic clip. + // subtreeExtent's own base rect already covers box's full border box + // (shadowExpandedRect starts from box.X/Y/W/H unconditionally), so its + // extent already contains bx without unioning it in, and the result is + // never shorter than bx (bx ⊆ dst.Rect and bx ⊆ the extent, so their + // intersection is too) — no floor to clamp against. + tmpH := subtreeExtent(box).Intersect(dst.Rect).Max.Y + tmp := image.NewRGBA(image.Rect(0, 0, dst.Rect.Dx(), tmpH)) tpp := painter.NewPixelPainter(tmp.Pix, tmp.Rect.Dx(), tmp.Rect.Dy()) paintBoxContent(tmp, tpp, box, f, imgs, bgImgs, clip) src := image.NewRGBA(image.Rect(0, 0, bx.Dx(), bx.Dy())) diff --git a/paint/rotate_test.go b/paint/rotate_test.go index 639842a..1778146 100644 --- a/paint/rotate_test.go +++ b/paint/rotate_test.go @@ -4,6 +4,8 @@ package paint import ( + "image" + "image/color" "testing" "github.com/go-webengine/engine/css" @@ -152,3 +154,54 @@ func TestRotateNegativeAngle(t *testing.T) { t.Errorf("bottom of -90°-rotated box = %+v, want red (was the LEFT half before a counter-clockwise 90°)", bottom) } } + +// TestRotateDoesNotCrashWhenAChildOverflowsTheBoxBottom covers a real crash +// (round 165, found by a 500-page corpus sweep on smashingmagazine.com): +// paintRotated's own offscreen buffer was sized only to the rotated box's +// OWN border box (rectOf), trusting it to bound everything paintBoxContent's +// recursion could write — but a shrink-wrapped float, a negative margin, a +// marker or a box-shadow can place a descendant's painted pixels below the +// box's own Y+H (subtreeExtent's own doc comment, already relied on by the +// filter/opacity/mask-image group path). The buffer was then too short, and +// blendPixel wrote past its own Pix slice — an out-of-bounds WRITE, not a +// mere clipping inaccuracy, and it crashed the whole render. +func TestRotateDoesNotCrashWhenAChildOverflowsTheBoxBottom(t *testing.T) { + dst := white(100, 80) + imgNode := &dom.Node{Type: dom.Element, Tag: "img"} + src := image.NewRGBA(image.Rect(0, 0, 4, 4)) + // Opaque: a zero-value (fully transparent) source would let blitImage's + // own zero-coverage short-circuit in blendPixel skip the actual pixel + // write entirely, silently NOT exercising the out-of-bounds write this + // test exists to catch. + for py := 0; py < 4; py++ { + for px := 0; px < 4; px++ { + src.SetRGBA(px, py, color.RGBA{R: 0, G: 180, B: 0, A: 255}) + } + } + root := &layout.Box{ + Node: &dom.Node{Type: dom.Element, Tag: "div"}, Style: &css.Style{}, + X: 0, Y: 0, W: 100, H: 80, + Children: []*layout.Box{{ + Node: &dom.Node{Type: dom.Element, Tag: "div"}, Style: &css.Style{RotateDeg: 10}, + X: 0, Y: 0, W: 60, H: 40, + Children: []*layout.Box{{ + // A real child box (e.g. a shrink-wrapped float or a negative + // margin placed it here) whose own border box — Y:[35,55) — + // extends 15px below the rotated box's own bottom edge (40), + // while still within dst's own 80px canvas. Its image is + // painted via paintItem -> blitImage -> blendPixel, the exact + // real crash trace (round 165, smashingmagazine.com) — blitImage + // writes straight to dst.Pix with no bound beyond the outer + // clip, which paintRotated's old tmp (sized only to the ROTATED + // box's own bottom, not its subtree's) did not account for. + Node: &dom.Node{Type: dom.Element, Tag: "div"}, Style: &css.Style{}, + X: 10, Y: 35, W: 20, H: 20, + Lines: []*layout.LineBox{{X: 10, Y: 35, W: 20, H: 20, Items: []*layout.InlineItem{ + {Image: imgNode, X: 10, Y: 37, ImgW: 4, ImgH: 4, Width: 4, LineHeight: 4}, + }}}, + }}, + }}, + } + // The old code panicked inside blendPixel; this must simply not crash. + PaintFull(dst, root, NewFonts(), map[*dom.Node]image.Image{imgNode: src}, nil) +}