Skip to content

[Text] TextCollapsingProperties corrections - #21404

Closed
Gillibald wants to merge 21 commits into
AvaloniaUI:masterfrom
Gillibald:fixes/textTrimmingModes
Closed

[Text] TextCollapsingProperties corrections#21404
Gillibald wants to merge 21 commits into
AvaloniaUI:masterfrom
Gillibald:fixes/textTrimmingModes

Conversation

@Gillibald

Copy link
Copy Markdown
Contributor

What does the pull request do?

Audits and fixes BiDi correctness across every TextCollapsingProperties
implementation — TextTrailingCharacterEllipsis,
TextTrailingWordEllipsis, TextLeadingPrefixCharacterEllipsis,
TextPathSegmentEllipsis — and the helpers they share
(LogicalTextRunEnumerator, ShapedTextRun.TryMeasureCharacters /
TryMeasureCharactersBackwards,
TextPathSegmentEllipsis.MeasureSegmentWidth). Adds a BiDi-focused test
class with 24 tests pinning correct behavior, documents the
logical-order Collapse contract on the abstract API, and includes a
perf rewrite of MeasureSegmentWidth that drops it from O(N·S + S·glyphs)
to O(N + S·(log N + log clusters)).

Trimming should always operate in logical order. The consumer
(TextLineImpl.CollapseFinalizeLine) re-runs the BiDi reorderer on
the result, so pre-applying visual order would be reordered a second
time and produce garbled output on RTL or mixed-bidi lines. Several
implementations either iterated visual-order runs while splitting at
logical offsets, or built width queries on visual-order glyph walks that
produced wrong counts for RTL.

What is the current behavior?

Nine concrete bugs, each verified by a failing test before the fix:

  • B1LogicalTextRunEnumerator returned _textRuns[0] on every
    MoveNext when _indexedTextRuns was null; all iterations yielded
    the same first run.
  • B2TextLeadingPrefixCharacterEllipsis walked textLine.TextRuns
    (post-bidi visual order) but called SplitTextRuns at logical
    character offsets. RTL and mixed-bidi prefixes came from the wrong
    characters.
  • B3TextLeadingPrefixCharacterEllipsis constructor guarded the
    readonly field _prefixLength (always 0) instead of the parameter
    prefixLength, so negative values were silently accepted.
  • B4TextLeadingPrefixCharacterEllipsis.Collapse hardcoded
    FlowDirection.LeftToRight for TextFormatter.CreateSymbol, ignoring
    the FlowDirection property — the ellipsis symbol's bidi level was
    wrong on RTL paragraphs.
  • B5TextLeadingPrefixCharacterEllipsis suffix loop appended runs
    in reverse-logical order; layout was wrong whenever the suffix spanned
    more than one run.
  • B6TextLeadingPrefixCharacterEllipsis width-budget tracking
    compared a cumulative currentWidth against the decreasing
    availableWidth, tripping overflow on multi-run lines well before the
    budget was exhausted. Per-run measuredLength was used as a global
    character index when splitting, and availableSuffixWidth
    over-subtracted whenever prefixLength capped the prefix partway
    through a fully-fitting run.
  • B7TextPathSegmentEllipsis fallback called
    ShapedTextRun.Split(0) whenever the whole post-prefix run fit in the
    remaining budget; Split(0) throws ArgumentOutOfRangeException.
  • B8TextPathSegmentEllipsis.MeasureSegmentWidth assumed
    GlyphCluster values increase monotonically with glyph index. That
    holds for LTR but RTL buffers store glyphs in visual order with
    cluster values decreasing, so RTL segments always measured as
    ~0 width. The middle-collapse algorithm rejected every window and
    fell back to "trim from start", dropping the logically-first segments
    of every RTL path.
  • B9ShapedTextRun.TryMeasureCharacters and
    TryMeasureCharactersBackwards walked the buffer in visual order and
    returned counts that callers used as logical leading/trailing prefix
    lengths. For RTL runs the cumulative width was being measured against
    the wrong set of characters — the count happened to come out right
    only for uniform-width fonts.

What is the updated/expected behavior with this PR?

Every collapse implementation now operates in logical order, leans on
the shared infrastructure (LogicalTextRunEnumerator, the cluster cache
on ShapedBuffer), and returns runs that the consumer can safely
re-bidi via FinalizeLine. RTL trailing ellipsis preserves the
logical-leading prefix; leading-prefix ellipsis preserves both the
logical-leading prefix and a width-correct logical-tail suffix;
path-segment ellipsis performs middle-collapse on RTL paths and no
longer crashes on small trailing runs.

New test class
tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCollapsingBidiTests.cs
(24 tests) pins:

  • LTR sanity that mirrors the existing Should_Collapse_Line baseline.
  • Width-edge short-circuits: no-collapse returns the same line instance;
    symbol-wider-than-width returns an empty collapsed line with
    HasCollapsed = true.
  • Pure RTL and mixed LTR/RTL paragraphs against each ellipsis class.
  • Multi-run lines via FixedRunsTextSource.
  • LogicalTextRunEnumerator iterated directly on a TextLineImpl whose
    _indexedTextRuns is null (covers B1).
  • TextLeadingPrefixCharacterEllipsis constructor validation and
    FlowDirection actually used for the ellipsis symbol (covers B3/B4).
  • Middle-collapse on both LTR and RTL paths (covers B7/B8).
  • Contract probes for TryMeasureCharacters and
    TryMeasureCharactersBackwards at varied budgets, cross-checked
    against the cluster cache (covers B9).

How was the solution implemented (if it's not obvious)?

Phased; each phase landed as its own commit so the diff can be stepped
through one bug at a time. All commits self-contained — each is CI-green
on its own.

  1. Fix LogicalTextRunEnumerator — one-line fix
    (_textRuns[_index] instead of _textRuns[0]) plus class-level
    XML-doc describing the logical-vs-visual ordering contract.
  2. Fix TextLeadingPrefixCharacterEllipsis on bidi and multi-run
    lines
    — rewrites Collapse to materialize logical-order runs
    once, use per-run width checks, track charsBeforeCurrentRun for
    global split positions, collect the suffix into a temporary list
    and drain it in logical order, and re-derive availableSuffixWidth
    from the actual prefix run widths.
  3. Fix TextPathSegmentEllipsis Split(0) in fallback path
    guards the split: if splitAt > 0 split as before; if length > 0
    use the whole run; otherwise drop it. Same shape as the
    SplitTextRuns length-zero fix landed earlier.
  4. Add TextCollapsingProperties BiDi characterization tests
    new test file; assertions target invariants
    (logical prefix preserved, ellipsis present, no crashes) rather than
    exact glyph output so they survive font changes.
  5. Document the logical-order Collapse contract — XML docs on
    TextCollapsingProperties.Collapse (implementations must return runs
    in logical order; consumer re-bidis via FinalizeLine) and
    LogicalTextRunEnumerator (use this instead of TextLine.TextRuns
    from inside Collapse implementations).
  6. Fix MeasureSegmentWidth on RTL buffers — picks baseCluster
    based on buffer.IsLeftToRight (smallest cluster either way) and
    adapts the early-exit checks so the cluster-window scan works in
    both directions.
  7. Speed up MeasureSegmentWidth — pre-computes runStartChars[]
    once per Collapse so the helper can binary-search the first
    overlapping run, and delegates per-shaped-run width measurement to a
    new ShapedBuffer.GetCharRangeWidth(startChar, endChar) method that
    uses the existing cluster-width prefix cache via binary search.
    Per-call shape: O(N + buffer.Length) → O(log N + log clusters).
    Per-Collapse: O(N·S + S·glyphs) → O(N + S·(log N + log clusters)).
    The direction-aware glyph walk added by step 6 disappears — the
    cluster cache is built in logical order for both directions.
  8. Fix TryMeasureCharacters / TryMeasureCharactersBackwards for
    RTL runs
    — both methods now delegate to two new ShapedBuffer
    helpers (FindLeadingCharCountWithinWidth,
    FindTrailingCharCountWithinWidth) that binary-search the cluster
    cache. Because the cache is in logical order for both bidi
    directions, the returned counts are the correct logical leading /
    trailing char count regardless of buffer direction. Each call is
    O(log clusters), cluster-atomic, and surrogate-pair counting is
    implicit in the cluster cache's startChars[] deltas.

Checklist

Breaking changes

None. Every behavior change is a bug fix:

  • LTR collapse output is unchanged — verified by the existing
    Should_Collapse_Line LTR baseline still passing alongside the new
    LTR characterization tests.
  • RTL collapse output now trims at the correct logical position with
    the actual rendered width fitting the budget. For uniform-width text
    this is visually indistinguishable from the previous output; for
    variable-width text (Arabic letter forms, ligatures, surrogate pairs)
    the trim point now lands where it should.

No public API additions or signature changes.

Obsoletions / Deprecations

None.

Fixed issues

Depends on: #21400

Gillibald added 17 commits May 20, 2026 13:06
Restructure Skia GlyphRunImpl ctor
Add more profiling with TextRunCache
The `else if (_textRuns != null)` branch — taken when the line hasn't
been finalized (or isn't a TextLineImpl) so `_indexedTextRuns` is null —
returned `_textRuns[0]` on every call instead of `_textRuns[_index]`, so
every `MoveNext` yielded the same first run.

Also adds a class-level XML-doc explaining the logical-vs-visual
ordering contract and when each backing list is used.
The Collapse implementation had several issues that broke it on
anything beyond a single LTR run:

- Iterated `textLine.TextRuns` (post-bidi visual order) but split at
  logical character offsets, so RTL and mixed-bidi lines selected the
  wrong characters for the prefix.
- The constructor's bounds check guarded the readonly field
  `_prefixLength` (always 0) instead of the parameter `prefixLength`,
  so negative values were silently accepted.
- `CreateSymbol` was called with a hardcoded `FlowDirection.LeftToRight`,
  ignoring the `FlowDirection` property — the ellipsis symbol always
  picked up the wrong bidi level on RTL paragraphs.
- The suffix loop appended runs in reverse-logical order to
  `collapsedRuns`, producing wrong layout when there was more than one
  post-prefix run.
- Width-budget tracking compared a cumulative `currentWidth` against
  the decreasing `availableWidth`, tripping overflow far too early on
  multi-run lines.
- Per-run `measuredLength` was used as a global character index when
  computing the split position; for an overflow in run K with
  `measuredLength = M`, the split position should be
  `Σ lengths of runs 0..K-1 + M`, not just `M`.
- `availableSuffixWidth` was derived from the loop's leftover
  `availableWidth`, which over-subtracted whenever `prefixLength` capped
  the prefix partway through a fully-fitting run.

The function now materializes the runs in logical order via
`LogicalTextRunEnumerator`, validates the parameter, honours
`FlowDirection`, collects suffix splits into a temporary list and drains
them in logical order, uses per-run width comparisons, tracks
`charsBeforeCurrentRun` for global split positions, and re-derives the
suffix budget from the actual prefix run widths. The consumer
(`TextLineImpl.Collapse` → `FinalizeLine`) re-runs the BiDi reorderer on
the result, so returning logical order is the correct contract.
…fits

In the fallback "trim from start at segment boundaries" branch,
`shapedRun.Split(splitAt)` was called with `splitAt == 0` whenever
`TryMeasureCharactersBackwards` returned a length equal to the whole
run length — i.e. when the entire post-prefix run fit in the remaining
budget. `ShapedTextRun.Split(0)` throws `ArgumentOutOfRangeException`.

Surfaced by the new RTL and mixed-bidi path tests, which fall into the
fallback branch because the middle-collapse algorithm currently can't
measure RTL segment widths correctly (separate issue worth a follow-up).

Guards the call: split as before when `splitAt > 0`; use the whole run
as `trimmedRun` when `length > 0` (entire run fits, no split needed);
otherwise leave `trimmedRun` null. Same shape as the SplitTextRuns
length-zero fix landed earlier.
New test class with 18 tests pinning the BiDi behavior of the four
`TextCollapsingProperties` implementations
(`TextTrailingCharacterEllipsis`, `TextTrailingWordEllipsis`,
`TextLeadingPrefixCharacterEllipsis`, `TextPathSegmentEllipsis`).

Coverage:

- LTR sanity (matches the existing `Should_Collapse_Line` baseline).
- Width edge cases: no-collapse short-circuit returns the same line
  instance unchanged; symbol-wider-than-width returns an empty collapsed
  line with `HasCollapsed = true`.
- Pure RTL and mixed LTR/RTL paragraphs against each ellipsis class.
- Multi-run lines via `FixedRunsTextSource`.
- `LogicalTextRunEnumerator` iterated directly on a TextLineImpl whose
  `_indexedTextRuns` is null (covers the path the recent B1 fix made
  correct).
- `TextLeadingPrefixCharacterEllipsis` constructor validation
  (negative prefixLength throws) and FlowDirection actually used for
  the ellipsis symbol's shaping.
- RTL and mixed-bidi paths for `TextPathSegmentEllipsis`.

Tests assert content-preservation invariants (logical prefix preserved,
ellipsis present, no crashes) rather than exact glyph output so they
survive font changes.
Adds a `<remarks>` block to the abstract `Collapse` member explaining
that implementations must return runs in logical order — the consumer
(`TextLineImpl.Collapse` → `FinalizeLine`) re-runs the BiDi reorderer,
so pre-applying visual order would be reordered a second time and
produce garbled output on RTL or mixed-bidi lines.

Points readers at `LogicalTextRunEnumerator` rather than
`TextLine.TextRuns` (which is post-bidi visual order) for iteration, and
at `CreateCollapsedRuns` for the standard "prefix + symbol" shape.
`MeasureSegmentWidth` assumed `GlyphCluster` values increase monotonically
with glyph index, comparing `g.GlyphCluster - buffer[0].GlyphCluster`
against the desired char range. That holds for LTR `ShapedBuffer`s but
not for RTL: RTL buffers store glyphs in visual order (the leftmost
visual glyph at index 0), so cluster values DECREASE with index and
`buffer[0].GlyphCluster` is actually the LARGEST cluster of the run.

The effect on RTL runs: `clusterLocal` was 0 for the first iteration and
negative for every subsequent glyph, so at most a single glyph's advance
ever ended up in `width`. Any non-trivial RTL segment measured as
essentially zero width, which made the middle-collapse algorithm in
`Collapse` reject every candidate window (no candidate's "savings"
brought the line under budget). RTL paths consequently always fell
through to the "trim from start" fallback, which drops the
logically-first segments.

The fix picks `baseCluster` based on the buffer's direction (`buffer[0]`
for LTR, `buffer[Length-1]` for RTL — the smallest cluster either way)
so `clusterLocal` is a true logical offset, and adapts the skip / break
checks so the early-exit optimization still works in both directions.

New regression tests:

- `Ltr_PathSegmentEllipsis_Middle_Collapse_Preserves_First_And_Last_Segments`
  — pins the LTR baseline (would catch any future regression of the
  ascending path).
- `Rtl_PathSegmentEllipsis_Middle_Collapse_Preserves_First_And_Last_Segments`
  — was failing before this fix because the result was just `…اخر.txt`
  (fallback output); now keeps both `اول` and `اخر.txt`.
`MeasureSegmentWidth` was called once per segment boundary in the
segmentation loop of `TextPathSegmentEllipsis.Collapse`. Each call:

- Re-walked every run from index 0 looking for the first overlap
  (O(N) per call across S segments → O(N·S) total).
- For each shaped overlap, iterated every glyph in the buffer
  (O(buffer.Length) per call), summing advances for glyphs whose
  cluster fell in the range.

For a path with K shaped runs, S segments, and ~T total chars, that
came out at roughly O(S · (N + T/N)) of glyph-iteration work per
`Collapse`, with the inner loop also carrying the B8 direction-aware
ascending/descending branch.

This change cuts both factors:

- Pre-compute `runStartChars[]` once at the start of `Collapse`
  (cumulative char offsets per run). `MeasureSegmentWidth` then
  binary-searches it to find the first overlapping run — O(log N) per
  call instead of O(N), independent of how deep the segment sits.
- Delegate per-shaped-run width measurement to a new
  `ShapedBuffer.GetCharRangeWidth(startChar, endChar)` helper. It
  binary-searches the existing cluster-width prefix cache
  (already built by earlier optimization work) and returns the diff
  between the two cluster boundaries. O(log clusters) per call instead
  of O(buffer.Length), and since the cluster cache is built in logical
  order for both LTR and RTL buffers, the direction-aware glyph walk
  introduced by B8 disappears.

Net asymptotic shape: O(N + S · (log N + log clusters)) instead of
O(N·S + S · glyphs). All 20 `TextCollapsingBidiTests` pass, 358 Skia
tests pass, 22,171 Base tests pass — including the LTR / RTL middle-
collapse tests that exercise both directions of the cluster cache.
Both methods walked the ShapedBuffer in visual order — for LTR runs the
visual order equals the logical order, so the returned count (computed
via cluster deltas) is the LOGICAL leading/trailing char count that
callers depend on. For RTL runs the visual order is the LOGICAL reverse
order, so the cumulative width was being measured for the wrong set of
characters:

- `TryMeasureCharacters` (forward) walked i = 0..bufferLength, which for
  RTL processes glyphs from the logical TAIL forward. The width-budget
  check fired against logical-trailing widths but the caller used the
  returned count as a logical-leading prefix length. When per-character
  widths vary by position (Arabic letter forms, ligatures, surrogate
  pairs), the trim landed at the wrong place — the kept prefix could
  exceed the budget or fall well short of it.
- `TryMeasureCharactersBackwards` had the symmetric issue: walking
  i = bufferLength-1..0 means walking logical-FORWARD for RTL, so
  callers expecting "logical-trailing N chars fit in budget" got
  "logical-leading N chars" instead.

The count came out right for uniform-width text by accident (any N
consecutive chars of equal width measure equally), which is why the
existing single-run RTL tests passed. A direct contract test at varied
budget points on Arabic confirmed the bug: at budget 9.15 the backwards
variant returned `length=2` for `"السلام عليكم ورحمة"` whose actual
logical-trailing-2-chars width is 12.77 — 38% over budget.

The fix moves both width queries to the cluster-width prefix cache on
`ShapedBuffer`. Two new helpers:

  internal int FindLeadingCharCountWithinWidth(double availableWidth)
  internal int FindTrailingCharCountWithinWidth(double availableWidth,
                                                out double consumedWidth)

Both binary-search the cluster cache, which is built in logical order
for both directions, so they return the correct logical-leading or
logical-trailing char count regardless of bidi level. Each call is
O(log clusters) instead of O(consumed glyphs), and cluster-atomic
(a multi-glyph cluster either fully fits or doesn't — semantically
cleaner than the previous per-glyph break that could leave a cluster
partially counted).

`ShapedTextRun.TryMeasureCharacters` and
`ShapedTextRun.TryMeasureCharactersBackwards` collapse to thin wrappers.
Surrogate-pair / variable-codepoint counting that was done via
`Codepoint.ReadAt` is handled implicitly by the cluster cache's
`startChars[]` deltas.

Verification: 24 BiDi tests pass (two new B9 contract tests added);
all 362 Skia tests pass; all 22,171 Base tests pass.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR audits and fixes BiDi correctness across TextCollapsingProperties implementations (and shared helpers) so collapsing operates in logical order, while also refactoring several hot-path text measurement routines to use the ShapedBuffer cluster cache for improved asymptotic performance. It adds a substantial new test surface (BiDi characterization + split/wrap/unit coverage) and extends the benchmarks project with additional Unicode/text benchmarks and a profiling harness.

Changes:

  • Fix logical-vs-visual ordering issues in text collapsing (including LogicalTextRunEnumerator, leading-prefix ellipsis, and path-segment ellipsis) and document the logical-order contract on the collapsing API.
  • Introduce/extend cluster-cache-based width and “characters that fit” queries on ShapedBuffer, and re-route wrap/metrics measurement to these helpers.
  • Add extensive new unit tests/characterization tests for wrapping/collapsing/Unicode utilities, plus new benchmarks and a profiling entry point.

Reviewed changes

Copilot reviewed 29 out of 30 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextLineTests.cs Make bidi text-bounds assertion tolerant to ULP drift.
tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextFormatterWrapCharacterizationTests.cs New characterization tests to pin wrapping behavior.
tests/Avalonia.Skia.UnitTests/Media/TextFormatting/TextCollapsingBidiTests.cs New BiDi-focused collapsing characterization tests.
tests/Avalonia.Skia.UnitTests/Media/TextFormatting/SplitTextRunsTests.cs New direct tests for TextFormatterImpl.SplitTextRuns.
tests/Avalonia.Benchmarks/Text/UnicodeTrieBenchmark.cs New benchmark for UnicodeTrie.Get throughput.
tests/Avalonia.Benchmarks/Text/UnicodeBreakEnumeratorBenchmark.cs New end-to-end benchmarks for break enumerators.
tests/Avalonia.Benchmarks/Text/TextLayoutProfile.cs New single-variant profiling-oriented TextLayout benchmark.
tests/Avalonia.Benchmarks/Text/HugeTextLayout.cs Rename emoji fixture constant for reuse by profile/harness.
tests/Avalonia.Benchmarks/Text/CodepointBenchmark.cs New benchmarks for Codepoint decoding and property access.
tests/Avalonia.Benchmarks/Program.cs Add --profile-textlayout harness bootstrapping real Skia/HarfBuzz services.
tests/Avalonia.Base.UnitTests/Media/TextFormatting/UnicodeTrieTests.cs New unit tests for trie branches + builder round-trips.
tests/Avalonia.Base.UnitTests/Media/TextFormatting/UnicodeEnumeratorAllocationTests.cs New allocation-guard tests for enumerators.
tests/Avalonia.Base.UnitTests/Media/TextFormatting/UnicodeDataTests.cs New spot-check tests for UnicodeData surface.
tests/Avalonia.Base.UnitTests/Media/TextFormatting/PropertyValueAliasHelperTests.cs New tests for generated alias helper mappings.
tests/Avalonia.Base.UnitTests/Media/TextFormatting/LineBreakEnumeratorTests.cs Enable the UAX#14 conformance theory (was previously skipped).
tests/Avalonia.Base.UnitTests/Media/TextFormatting/CodepointTests.cs New unit tests for Codepoint decoding/helpers.
src/Skia/Avalonia.Skia/GlyphRunImpl.cs Fuse glyph walks to reduce per-run overhead.
src/Avalonia.Base/Media/TextPathSegmentEllipsis.cs Fix split(0) crash and segment width measurement; optimize with binary search + cache.
src/Avalonia.Base/Media/TextFormatting/Unicode/LineBreakPairTable.cs Remove unused line-break pair table file.
src/Avalonia.Base/Media/TextFormatting/Unicode/LineBreakEnumerator.cs Optimize rule evaluation/caching and reduce repeated state reads.
src/Avalonia.Base/Media/TextFormatting/TextRunCache.cs Use foreach for perf (bounds-check elision) when add-ref/disposing cached runs.
src/Avalonia.Base/Media/TextFormatting/TextLineImpl.cs Use foreach for perf in disposal/metrics walks.
src/Avalonia.Base/Media/TextFormatting/TextLeadingPrefixCharacterEllipsis.cs Rewrite collapse to operate in logical order and fix BiDi/multi-run correctness.
src/Avalonia.Base/Media/TextFormatting/TextFormatterImpl.cs Add SplitTextRuns(..., out firstLength) and refactor wrap measurement/splitting.
src/Avalonia.Base/Media/TextFormatting/TextCollapsingProperties.cs Document logical-order contract for Collapse.
src/Avalonia.Base/Media/TextFormatting/ShapedTextRun.cs Route measurement helpers through new ShapedBuffer cache-based methods.
src/Avalonia.Base/Media/TextFormatting/ShapedBuffer.cs Add cluster-cache APIs and share cache across splits; add binary-search helpers.
src/Avalonia.Base/Media/TextFormatting/LogicalTextRunEnumerator.cs Fix enumerator bug and document logical-order purpose/contract.
src/Avalonia.Base/Media/GlyphRun.cs Use ShapedBuffer.TotalGlyphAdvance when available to avoid per-glyph summation.
.gitignore Ignore /planning and BenchmarkDotNet artifacts.

Comment thread src/Avalonia.Base/Media/TextFormatting/ShapedTextRun.cs Outdated
Comment thread src/Avalonia.Base/Media/TextFormatting/TextFormatterImpl.cs
@avaloniaui-bot

Copy link
Copy Markdown

You can test this PR using the following package version. 12.1.999-cibuild0065653-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Gillibald added 4 commits May 21, 2026 09:52
…live owner

ShapedBuffer rents its glyph storage from ArrayPool<GlyphInfo>.Shared in
the public constructor, and both Split and WithBidiLevel produce child
buffers that view into that pool array via ArraySlice — but those views
had no ownership of the rented array. Calling the owner's Dispose
unconditionally returned the array to the pool, leaving the views with
dangling references. Subsequent pool consumers could overwrite the
glyph data the views were still reading from.

Two call sites in TextFormatterImpl exercise this exact pattern:

  - SplitTextRuns:284 disposes the original ShapedTextRun (and so its
    ShapedBuffer) right after handing off the split halves to the
    caller.
  - ResetTrailingWhitespaceBidiLevels:1173 disposes the original
    ShapedTextRun right after wrapping a WithBidiLevel view of its
    buffer.

In the no-cache path the original's refcount drops to zero immediately
and the bug fires deterministically; with a TextRunCache it's deferred
until the cache itself releases the run, but the lifetime invariant
is just as broken.

The fix introduces a small shared-ownership refcount on ShapedBuffer:

  - The owner constructor (the one that calls ArrayPool.Rent) sets
    _arrayOwner = this and _refCount = 1.
  - Every view constructor (used by Split's ascending/descending paths,
    Split's empty-prefix shortcut, and WithBidiLevel) takes an
    `arrayOwnerSource` parameter, inherits the source's chain-root
    owner, and Interlocked.Increments its refcount.
  - Dispose decrements the owner's refcount; only when it reaches zero
    is the rented array returned to the pool. Double-Dispose is a
    no-op the second time round.
  - GC-managed-array variants (the existing internal ctor used by
    TextFormatterImpl for synthetic empty-line runs) leave _arrayOwner
    null and Dispose stays a no-op.

New test class ShapedBufferLifetimeTests pins the contract by
reflecting on the owner's _rentedBuffer field after each
dispose step — that's deterministic regardless of whether ArrayPool
happens to re-rent the same array. Six tests cover LTR Split,
RTL Split, chained Split, WithBidiLevel views, and double-Dispose
safety on both owner and view. All four ownership-probing tests
fail on the previous code (the buggy Dispose path) and pass with
the fix.

Verification: 6/6 lifetime tests pass; full Skia (368) and Base
(22,171) suites green.
The lifetime tests previously reflected on the private `_rentedBuffer`
field to assert when the pool array survived vs. was returned. That
worked but was fragile and obscured the contract.

Adds a narrow `internal bool ShapedBuffer.IsPoolArrayRented` property
(true while the instance still holds an ArrayPool-rented GlyphInfo[],
false after the array has been returned or for non-pool buffers) and
rewrites the tests to use it. No behavior change.
@avaloniaui-bot

Copy link
Copy Markdown

You can test this PR using the following package version. 12.1.999-cibuild0065669-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Gillibald Gillibald changed the title [WIP] TextCollapsingProperties corrections [Text] TextCollapsingProperties corrections May 21, 2026
@MrJul

MrJul commented May 26, 2026

Copy link
Copy Markdown
Member

Note: I'll wait for #21400 to be merged so this can be rebased to have a clean diff.

@Gillibald Gillibald closed this May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants