microsoft / microsoft/microsoft-ui-reactor

`PreviewCaptureServer` can serve a stale frame after `/preview` switches components

Open
#1,014 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
646
Forks
54
Avg merge
1d 3h
Merged PRs (30d)
84

Description

Found while fixing #989 (PR #1011). Pre-existing, different project, deliberately out of scope there because it needs a capture-protocol change rather than a validation fix.

## The problem

`PreviewCaptureServer` keeps the most recent captured frame in `_latestFrame` and serves it to `/frame` requests. `/preview` — which switches the server to a different component — **never clears or versions that field** (`src/Reactor.Devtools/PreviewCaptureServer.cs:445-457`, `641-651`).

So after a component switch, a `/frame` request that arrives before the new component has painted is answered with the *previous* component's frame. It is a fully-painted, content-bearing, entirely valid-looking image — of the wrong thing.

## Why it matters more now

PR #1011 hardened `ScreenshotCapture.PollForFrame` to hold out for a frame with visible content instead of accepting the first non-empty body. That closes the blank-frame hole, and it is the right fix for the failure that actually corrupted 103 assets.

But it closes that hole by asking "does this frame have content?", and a stale frame passes that question trivially. The guard is sound against the failure it was written for and blind to this one. Worth being explicit about, because the natural reading of "we now validate captured frames" is broader than what the guard actually establishes — and this is exactly the residue that gets forgotten once the loud bug is fixed.

Practical consequence: a doc screenshot could be committed showing the wrong component entirely. That is arguably *worse* than the blank-frame case, because a blank PNG is obviously wrong at a glance and to a byte-size heuristic, while a plausible screenshot of the wrong control is not — no gate in the pipeline can catch it, and neither can a reviewer who doesn't already know what the control should look like.

## Suggested fix

Version the frame rather than validating it, so freshness is established by the protocol instead of inferred from pixels:

- `/preview` clears `_latestFrame` and increments a generation counter.
- `/frame` returns the current generation alongside the image (header or JSON envelope).
- `ScreenshotCapture` records the generation it expects after issuing `/preview` and discards any frame from an earlier one.

That makes "this frame belongs to the component I asked for" a checkable fact rather than a timing assumption. The content check from #1011 stays useful and orthogonal — it catches an unpainted frame within the correct generation.

A cheaper stopgap would be for `/preview` to null `_latestFrame` so a stale frame is at least never *served*; that turns a wrong-content failure into a blank-frame failure, which the #1011 guard already handles correctly. It is strictly an improvement, but it leaves the freshness contract implicit and timing-dependent, so it should be a stepping stone rather than the destination.

## Reproduction

Not yet reproduced end-to-end — capture needs an interactive desktop, and the window is narrow (it requires a `/frame` landing between the switch and the first paint of the new component). The code path is unambiguous on inspection, though: there is no write to `_latestFrame` on the `/preview` path and no invalidation anywhere else, so nothing prevents it.

Flagging honestly: that makes this a code-reading finding, not a measured one. If someone wants to confirm it before investing in the protocol change, forcing a slow first paint in the new component and issuing `/frame` immediately after `/preview` should surface it deterministically.

Contributor guide

Open the contributing guide

Research direction

Start with PreviewCaptureServer.cs:445-457 and 641-651 to trace how /preview and /frame use _latestFrame, then inspect ScreenshotCapture.PollForFrame and the capture flow described in #1011. The change is done when frames from before a component switch cannot be accepted, while the existing visible-content check still handles unpainted frames; no end-to-end reproduction or test file is named.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
desktop, devtools
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.