Phase 8 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md, designed in
REVIEWS/adr/0003-pdf-incremental-save.md.
The review treats Phase 8 as ten independent projects, each needing its own
design doc and merge criteria. Incremental save is taken first because it is
the only one whose prerequisites are already met, it is listed as a
prerequisite by two others (annotation editing and full AcroForm support),
and it closes a real credibility gap: DocumentFormEditor has been able to
edit form fields since Phase 3, and there was no way to save the result.
A grep for a public save API across all four crates returned nothing.
Design decision: append a revision, never rewrite. The original bytes are
copied verbatim and changed objects are appended with a new xref chained
through /Prev. A full rewrite would be easier and wrong: it would silently
discard everything this parser does not yet model (structure trees, optional
content, embedded files), and it would invalidate any signature, foreclosing
a feature listed later in the same phase. ADR 0003 records this in full.
Two latent bugs surfaced while building it, both pre-existing:
- find_xref_start searched with windows(10) for the 9-byte keyword
"startxref", so it never matched. Every parse silently fell through to a
forward scan for the first "xref" in the file. On a single-revision
document that happens to be correct; on an incrementally saved one it is
the *oldest* revision, so a saved edit read back as its pre-edit value.
This had no visible effect before because nothing produced multi-revision
files.
- XRefTable::parse read one section and ignored /Prev entirely, so a
multi-revision document lost every object the earlier revisions defined.
It now walks the chain newest-first, keeping the first definition of each
object, with a visited set against /Prev loops and bounds checks on the
offsets, which come from the file and cannot be trusted. A bad link ends
the chain instead of indexing out of bounds.
The xref unit fixture claimed startxref 408 in a 191-byte file and only ever
passed because of the windows(10) defect; it is corrected rather than
adjusted to keep passing.
Implementation:
- pdf-cos/src/incremental.rs: IncrementalUpdate builds one revision.
Recomputes stream /Length so a caller cannot write an inconsistent one,
emits xref subsections for contiguous runs, sizes /Size over the whole
chain, and is byte-reproducible for a given set of edits.
- pdf-document/src/save.rs: turns dirty AcroForm fields into a revision,
writing the new /V and a regenerated appearance stream referenced from
/AP, keyed by state name for checkboxes and radios.
Refusals rather than partial saves: an encrypted document returns
SaveError::Encrypted, because writing plaintext objects into it would
corrupt the file; a source with no startxref or no /Root is refused; and a
save with no pending edits returns the input unchanged rather than growing
the file and churning its timestamp.
Tests: 10 acceptance tests in tests/save_roundtrip.rs covering the ADR merge
criteria. The central one reparses from the written bytes rather than
reusing in-memory state, so it tests the file rather than the writer against
itself. Also asserts three chained revisions still reparse with the newest
value winning, that pages and annotations survive a save, and that saving
never panics on the malformed corpus.
Known limitations, recorded in the ADR rather than glossed: cross-reference
streams and object streams are not written, and superseded objects are not
compacted.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (307 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (351 tests, 6 ignored)
Both rustfmt and clippy -D warnings clean.
Fix security vulnerability in recursive-descent JSON parser:
- Add MAX_JSON_DEPTH constant (128 levels)
- Add depth field to JsonParser struct
- Check depth limit in parse_value() before recursion
- Increment depth in parse_object() and parse_array()
- Decrement depth when returning from parse_object() and parse_array()
- Return error when nesting exceeds MAX_JSON_DEPTH
This prevents stack overflow attacks using deeply nested JSON structures.
Fixes: BUG-012 (Security Vulnerability in JSON Parsing)
Fix potential null pointer dereference in style evaluation functions:
- Add empty check in evaluate_color() before accessing stops.last()
- Replace unwrap() with safe last() check in evaluate_color()
- Replace unwrap() with safe last() check in evaluate_width()
- Return default values when stops is empty
This prevents panics when evaluating styles with empty stop arrays.
Fixes: BUG-009 (Null Pointer Dereference in Style Application)
Fix potential buffer overflow in MVT parser by adding overflow checks:
- Add bounds check in read_pb_len_slice() to prevent integer overflow
- Add bounds check in skip_pb_field() for wire type 2
- Check if length is unreasonably large (> bytes.len()) before adding to pos
- Prevents integer overflow when pos + len wraps around
This prevents buffer overflow vulnerabilities when parsing malformed
MVT tiles with extremely large length values.
Fixes: BUG-007 (Buffer Overflow in MVT Parser)
Fix use-after-free in geometry rendering by deferring eviction until
after rendering is complete:
- Add pending_eviction field to TileCache
- Add set_pending_eviction() method to schedule eviction
- Modify tick() to perform pending eviction at start of next frame
- Rename evict() to evict_internal() for deferred execution
- Update view.rs to call set_pending_eviction() instead of evict()
This prevents use-after-free by ensuring that Geometry objects are not
freed while the renderer is still using them. Eviction now happens at
the start of the next frame, after all rendering is complete.
Fixes: BUG-005 (Use-After-Free in Geometry Rendering)
Fix potential integer overflow when calculating tile coordinates:
- Prevent overflow when z >= 31 (would overflow i32 when cast)
- Clamp zoom level to max 30 before casting to i32
- Apply fix to all 4 locations where powi() is used with tile coordinates:
- geometry.rs: local_tile_to_lon_lat()
- mvt_parser.rs: local_tile_to_lon_lat()
- tessellation.rs: lonlat_to_tile_coords()
This prevents undefined behavior when processing tiles at very high zoom levels.
Fixes: BUG-004 (Integer Overflow in Tile Coordinate Calculation)
Enhance HTTP error handling to include detailed context for debugging:
- Add tile coordinates (z, x, y) to error messages
- Add generation number to error messages
- Log unknown request_id errors
- Improve error messages for missing response body
- Improve error messages for missing thread pool
This provides better debugging information when HTTP requests fail,
making it easier to diagnose network issues and tile loading problems.
Fixes: BUG-003 (Missing Error Handling in HTTP Requests)
Fix memory leak in cache eviction by explicitly freeing GPU resources
(Geometry objects) when tiles are evicted from the cache.
Changes:
- Add cx: &mut Cx parameter to evict() method
- Free fill_geometry and stroke_geometry before removing tiles
- Update all callers to pass cx parameter
- Update test code to create default Cx for tests
This prevents GPU memory leaks when tiles are evicted from the cache
during pan/zoom operations.
Fixes: BUG-002 (Memory Leak in Cache Eviction)
Three items were carried forward as known gaps rather than quietly dropped.
This addresses all three; two are closed outright and one is bounded by an
environment limit that is now documented rather than implied.
1. Fuzzing had never actually run (Phase 6 step 6.3).
The five cargo-fuzz targets were only compile-checked, so "zero panics on
arbitrary input" was an aspiration. They have now been run under nightly
libFuzzer:
parse_object 1,970,750 runs
parse_xref 2,471,345 runs
decode_stream 1,120,019 runs
parse_content_stream 2,655,663 runs
parse_document 2,381,367 runs
About 10.6 million executions in total, no crashes and no new findings. That
is a real result rather than a green checkmark: the three crashes the corpus
found in Phase 6 were the ones worth finding, and the fuzzer confirms the
fixes hold under adversarial input.
2. Combo dropdown overlay (Phase 4 step 4.3).
A combo box that cannot be opened is a text field with extra steps, so the
open list is real state, not a rendering detail. Clicking a combo box opens
its options; the dropdown takes a click before any field underneath it,
matching the draw order; choosing a row sets the value through
DocumentFormEditor; clicking elsewhere dismisses it without changing the
value. render_open_combo() returns placement data so the drawing code stays
trivial and the geometry is testable without a renderer.
3. Makepad event delivery.
Upstream added a makepad_test framework, so this is now testable in
principle. Adds a test host binary and six UI tests that drive the widget
through the Studio protocol: a real click on the fixture link must surface
OpenUri on the host, typing must reach the field, and a click on empty space
must emit nothing so the positive assertions are not vacuous.
They are #[ignore] by default because the Studio hub cannot start an app in
this sandbox: the harness launches with --stdin-loop, which Makepad refuses
without a Studio websocket, and the build exits 101 before startup.
Upstream own spreadsheet-ui and map UI suites fail identically here with the
same error, so this is the environment rather than this code. The tests are
checked in and compiled by cargo test so they cannot rot, CI runs them where
a hub exists, and the module documents how to run them by hand.
Getting there also fixed a real defect in the test host: it copied
ui.main_view.render() from the spreadsheet app startup hook, but a plain
View has no render method, so the app errored at startup.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (270 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (321 tests, 6 ignored)
cargo +nightly fuzz run <target> -- -max_total_time=60 (5 targets)
Both rustfmt and clippy -D warnings clean.
Phase 7 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md, taken only now
because the review is explicit that it comes after correctness is proven.
The caching and threading live in pdf-graphics rather than the Makepad crate
because none of it needs a GPU. That is what lets the staleness, eviction and
cancellation rules be tested without a window; the widget keeps only the
parts that genuinely need a Cx.
Step 7.1, off-thread parsing (new pdf-graphics/src/worker.rs):
- RenderWorker interprets content streams on a background thread and returns
results tagged with the Generation they were requested for.
- PendingPages tracks in-flight pages so the widget can draw a placeholder
and never queues the same page twice.
- Drop joins the thread rather than detaching it: a detached thread writing
into a dropped channel is the kind of shutdown race that surfaces as a
flaky test months later.
- A content stream that fails to parse yields an empty page, so one broken
page cannot take down the document.
Step 7.2, texture and memory management (new pdf-graphics/src/cache.rs):
- PageCache is an LRU keyed by page index with a byte budget, not an entry
count: one image-heavy page can outweigh fifty text pages, so counting
entries would evict the wrong things.
- retain_around() releases pages that scrolled out of view, keeping a margin
so a small scroll does not immediately re-render.
- Decoding produces DecodedImage bytes off-thread; GPU upload stays on the
UI thread.
- A page larger than the whole budget is still stored, since refusing it
would mean re-rendering it every frame.
Step 7.3, render command cache:
- CachedPage holds the interpreted Vec<RenderCommand>, so replaying a page
skips re-parsing its content stream.
- invalidate_appearance() marks a form edit dirty without discarding the
commands, because a form edit changes what is drawn over the page, not the
page content stream.
Widget wiring: PdfPageWidget carries a generation, refuses PageContent from a
superseded document, and draws a placeholder while a page is still rendering.
Exit criterion (new pdf-document/tests/phase7_exit_criterion.rs, 9 tests)
against a new 60-page corpus fixture. Measured here: first page 3ms against
the 200ms budget, and a warm cache read 389x faster than re-parsing (1us vs
389us). The budget is deliberately loose because a shared CI runner is
unpredictable and a flaky performance test gets muted, and a muted test is
worse than none; it still catches the order-of-magnitude regression the
review is guarding against. Memory is asserted bounded across 30 document
switches, and every page of an abandoned document is refused.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (270 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (307 tests)
Both rustfmt and clippy -D warnings clean.
Phase 6 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md: "Real PDFs. Real
regressions. No test theater."
Step 6.1, corpus. 27 fixtures across basic, fonts, forms, annotations,
images, edge and malformed, in the layout the review specifies. They are
produced by tests/corpus/generate.py rather than committed as opaque blobs,
because a corpus you cannot read is a corpus you cannot trust; CI regenerates
them and fails if they differ. Hand-rolled rather than library-produced,
since fixtures for a parser must contain constructs a library refuses to
emit.
Step 6.2, corpus tests (pdf-document/tests/corpus.rs, 27 tests). Text,
vectors, Flate, multipage, CID fonts, every form field type, inherited field
keys, link actions, hidden annotations, XObjects, inline images with
embedded EI bytes, rotation, crop boxes, nested CTMs and content arrays.
Step 6.3, robustness (pdf-document/tests/robustness.rs, 3 tests) plus five
cargo-fuzz targets. cargo-fuzz needs nightly and libFuzzer so it cannot gate
a stable CI run; the harness covers the same ground deterministically by
mutating the real corpus with a fixed-seed PRNG, so a failure is reproducible
from the seed rather than only from a saved artefact. The fuzz targets remain
the deeper coverage-guided search and run on a schedule.
Three crashes on untrusted input, all found by this work and all previously
reachable from a malformed file:
- collect_pages_ref recursed forever on a /Kids cycle. Stack overflow aborts
the process; it cannot be caught. Now tracks visited nodes and bounds depth.
- PdfDocument::resolve and the COS lexer recursed once per nesting level, so
a file of 5000 open brackets overflowed the stack. Both are now bounded.
- decode_85_group multiplied an accumulator that a malformed group can
overflow, and subtracted below zero on a digit outside the valid range.
Both panic in a debug build. Now saturating.
Step 6.4, CI (.forgejo/workflows/pdf.yml). An engine job that runs the
corpus and robustness suites under rustfmt and clippy -D warnings; a separate
makepad-integration job so a missing system library is not reported as a PDF
regression; and a scheduled fuzz job. The engine job also enforces the two
architectural rules mechanically rather than in prose: no Makepad dependency
or import in the engine crates, and no process or URL launching anywhere in
them.
Also fixes .gitignore: the blanket *.pdf rule silently excluded all 27
fixtures, which would have left CI unable to run them on a fresh clone.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (233 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (268 tests)
Both rustfmt and clippy -D warnings clean; all five fuzz targets compile.
Add comprehensive visual regression testing using Makepad's test framework:
Makepad Test App:
- Create makepad_test_app with 13 test scenarios
- Test empty map, single/multiple tiles, zoom levels 10-16
- Test dark theme, POIs, labels, roads, water, buildings
- Support both interactive and headless modes
- Include UPDATE_GOLDEN environment variable for updating references
Test Infrastructure:
- Add MAKEPAD_TESTING_GUIDE.md with comprehensive documentation
- Create visual_regression.rs with 13 test functions
- Add screenshot comparison with 1% threshold
- Save diff images on failure for debugging
- Support golden image directory structure
Test Scenarios:
1. empty_map - No tiles loaded
2. single_tile_amsterdam - Single tile render
3. multiple_tiles_grid - 3x3 tile grid
4. zoom_level_10 - Low zoom overview
5. zoom_level_12 - Medium zoom
6. zoom_level_14 - Standard city zoom
7. zoom_level_16 - High zoom with details
8. dark_theme - Dark theme rendering
9. pois_visible - Point of interest icons
10. labels_visible - Text labels
11. roads_render - Road geometry
12. water_features - Water/canal rendering
13. buildings_render - Building footprints
This completes the visual testing component of Phase 6.
Phase 5 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md.
Step 5.1, real metrics (new pdf-graphics/src/sfnt.rs):
- Reads head, hhea, OS/2 and hmtx from embedded TrueType/OpenType programs,
which the review asks for by name. sTypoAscender/Descender are preferred
over hhea when non-zero, since subset fonts often zero them; sCapHeight
and sxHeight are only read from OS/2 version 2 and later, because reading
them from a v1 table returns whatever bytes follow it.
- Every read is bounds-checked. A table pointing outside the file, a
truncated directory or a zero unitsPerEm yields None rather than a panic
or a divide by zero. These are untrusted embedded programs.
- resolve_font() now reads FontFile/FontFile2/FontFile3 and falls back to
the descriptors declared Ascent/Descent only when no program is embedded.
- renderer.rs looks up a per-font ascent instead of leaving ascent_em
permanently None, which had left the fallback ratio always in effect.
Step 5.2, accumulated advances (new pdf-graphics/src/advance.rs):
- measure_advance() implements PDF 32000-1 9.4.4 properly:
(w0/1000 * Tfs + Tc + Tw) * Th/100, with Tw restricted to single-byte
code 32. Applying Tw to a two-byte code whose low byte is 32 is a classic
composite-font bug and is now covered by a test.
- glyph_advances() gives per-glyph widths so a caret or a partial selection
rectangle no longer assumes even spacing.
- GlyphAdvanceCache is keyed by font name and size as the review specifies,
and carries a generation so it rebuilds when page data changes rather
than every frame. Redefining a font drops its stale measurements.
Step 5.3, composite fonts:
- The old build_cid_widths truncated every CID to u8, so any glyph above
255 silently took the default width. It is replaced by CidWidths keyed by
the real CID, with the CMap resolved from a predefined name or an
embedded stream.
- ResolvedFont::decode_text returns None for a composite font with no
ToUnicode map instead of guessing Latin-1 and producing plausible
nonsense.
Tests: new tests/phase5_exit_criterion.rs asserts the exit criterion.
Selection across a proportional and a monospaced run verifies the second
run starts at the first ones true end; per-glyph advances sum to the run
advance and H measures wider than l, which an even-spacing estimate would
not. Copy returns correct Unicode through ToUnicode including a non-ASCII
scalar, and through WinAnsi for simple fonts. Search finds both hits across
two lines with rectangles inside their own runs and the right vertical
order. Cache reuse is asserted through hit and miss counts.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (203 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (238 tests)
Both rustfmt and clippy -D warnings clean.
Integration Tests:
- Add tile_decode_integration.rs with 17 test cases
- Test simple and complex geometry decoding
- Test security limit enforcement
- Test error handling for malformed input
- Test various OSM feature types
- Test different zoom levels
- Test Unicode tag handling
Benchmarks:
- Add tile_decode_bench.rs with 6 benchmark suites
- Benchmark simple JSON decoding
- Benchmark scaling with feature count (10-1000 features)
- Benchmark MVT parsing performance
- Benchmark geometry tessellation
- Benchmark POI extraction (100 POIs)
- Benchmark label extraction (50 labels)
Fuzz Testing:
- Add fuzz testing infrastructure with cargo-fuzz
- Create 3 fuzz targets: MVT parser, Overpass parser, full pipeline
- Test parsers with random input to find crashes and edge cases
Test Runner:
- Add tools/run_map_tests.sh for easy test execution
- Support for unit, integration, bench, fuzz, and coverage tests
- CI-friendly test execution
Documentation:
- Add PHASE6_TESTING_VALIDATION.md with comprehensive guide
- Document test categories, running tests, and best practices
- Include CI workflow examples and future improvements
This completes Phase 6 of the code quality improvements.
Phase 4 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md. The previous
commit added the interaction logic but left it an orphan: nothing routed
Makepad events into it, which is the exact defect the review names when it
says PdfPageView declares is_interactive() -> false and an empty
handle_event.
New pdf-makepad/src/page_view.rs, PdfPageWidget:
- is_interactive() returns true and handle_event routes real events.
- Step 4.1 ordering: form field events, then annotation hit testing, then
text selection. The order is enforced inside InteractionState::click so
it cannot drift between the widget and the tests.
- Step 4.2: link underlines and form widget borders are drawn, and the
cursor changes over links and text fields.
- Step 4.3: field values draw inside their widget rects, a focused field is
highlighted and shows its uncommitted buffer.
- Step 4.4: navigation is emitted as PdfPageAction to the host. The widget
never opens a URL (rule 5).
- Key focus loss commits the pending edit, so an edit is never silently
lost, while Escape still abandons it.
- Only keys the editor handles are consumed; everything else falls through
to the host instead of being swallowed.
The widget keeps no decision logic of its own: it syncs the viewport from
its on-screen rect and delegates. That keeps the part that needs a live Cx
as small as possible, because it is the part that cannot be unit tested.
Also adds Selection to interaction.rs so drag-to-select and copy read
through the Phase 5 PageText API rather than re-deriving positions.
Tests: new tests/phase4_exit_criterion.rs drives the exit criterion against
the real two-page AcroForm fixture, not synthetic dictionaries. Click a link
gives OpenUri; click a field, type, commit, and the value changes, the
appearance regenerates with the new text and the draw list shows it. It also
asserts an abandoned edit changes nothing, the checkbox uses the /On state
declared in the file, and form fields route before annotations.
Validation: TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh
202 tests pass; rustfmt and clippy -D warnings clean.
Phases 4 and 5 of REVIEWS/DART_PDF_VS_MAKEPAD_PDF_GAP_ANALYSIS.md.
pdf-makepad had never compiled. It was missing eight PdfDevice methods, had
a non-exhaustive RenderCommand match, called a function through the wrong
path and held a borrow conflict. Everything previously claimed about the
Makepad rendering path was therefore unverified. It now builds and is tested
under a new pdf-ui target; the makepad build needs system GUI libraries, so
that target checks for them with pkg-config and names the missing packages
rather than failing in the linker.
Phase 4 (new pdf-makepad/src/interaction.rs):
- Viewport maps between PDF space (y up) and screen space (y down) in one
place, so hit testing and rendering cannot disagree. Inverted /Rect
corners are normalised.
- Click routing follows the review order: form fields first, then annotation
hit testing. A field overlapping a link takes the click.
- Clicking a link emits PdfAction::OpenUri to the host. The viewer never
opens a URL itself (rule 5).
- Text editing is buffered: typing changes a working copy and only Enter,
Tab or blur commits it, so Escape abandons an edit with the document
untouched. Caret arithmetic is in characters, not bytes, so a non-ASCII
value cannot panic.
- Checkboxes toggle on click using the state the widget declares.
- Hover reporting for cursor feedback; read-only fields are not targets.
- render_fields() returns placement data so a focused field shows its
uncommitted buffer while drawing stays trivial.
Phase 5:
- New pdf-graphics/src/cid.rs: composite font support. Codespace ranges give
variable-width code decoding (the previous CMap was u8 to char, so every
two-byte CID font decoded as garbage), plus cidchar/cidrange, ToUnicode
bfchar/bfrange including array destinations and multi-character values
such as ligatures, /W and /DW CID widths, and Identity-H/V. An unknown
predefined CMap returns None instead of silently substituting Identity;
unmapped codes decode to U+FFFD instead of vanishing. Width ranges are
bounded so a malformed /W cannot exhaust memory.
- New pdf-graphics/src/text.rs: the PageText extraction API. Segments carry
origin, advance and font size; find() returns matches with rectangles;
selection spans runs and works in either drag direction. Runs sharing a
baseline stay on one line even with mixed font sizes.
- renderer.rs: the two unexplained `fs * 0.75` constants are replaced by a
named DEFAULT_EM_TO_CAP_RATIO used only as a fallback, with the real
ascent preferred when the descriptor supplies one. Ts (text rise) was
stored but never applied, so superscripts drew on the baseline.
Also fixes clippy across pdf-makepad, which had never been linted.
Validation:
TEST_TARGET=pdf ./tools/test-rust-clean.sh (167 tests)
TEST_TARGET=pdf-ui ./tools/test-rust-clean.sh (191 tests)
Both rustfmt and clippy -D warnings clean.
