fix(moq-video): end X11 window capture when the window is destroyed - #3383
Conversation
X clients reuse freed window ids, so revalidating the target with get_geometry alone cannot tell the selected window from a replacement that inherited its id. Select StructureNotify at open and drain the queue each frame so DestroyNotify ends the stream either way. Completes the x11-window-identity quest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d090f56f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // before this reply is already queued here. A destroyed window whose id | ||
| // nobody took makes the request above fail instead, which is equally | ||
| // terminal. | ||
| if self.destroyed()? { |
There was a problem hiding this comment.
If the owner destroys the target and reuses its XID after this get_geometry reply but before GetImage is processed, this drain can return empty, while get_image().reply() later queues the DestroyNotify and successfully captures the replacement. The code then publishes one frame from a window the user never selected before noticing the event on the next iteration. Drain again after a successful image reply and cover this ordering in the regression test, since the current predicate-only test cannot catch it. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L148-L152
Useful? React with 👍 / 👎.
1 participant
Unverified. Written on a Mac, and
capture/x11.rsiscfg(target_os = "linux"), so nothing local compiled or ran the changed file. The check that matters is in What a reviewer with an X session should check below.Summary
capture/x11.rsidentified its target by XID alone and revalidated it each frame withget_geometry. An XID is not an identity: X clients allocate window ids out of their own range and do reuse freed ones, so a window destroyed and replaced by the same client can inherit the id. If the replacement's even-clamped dimensions match,get_geometrykeeps succeeding and capture keeps publishing, now against a window the user never selected. The exposure is narrow (destroy, reuse, and a size match all inside one frame interval) but the failure mode is publishing content nobody chose, the same class as the layered-window leak fixed during feat(video): complete native screen capture #3244 review.StructureNotifyon the target at open, and drain the event queue before each frame; aDestroyNotifyfor the captured id ends the stream regardless of what happens to the id afterwards. The subscription goes in before theget_window_attributes/get_geometryround trips, so everything read back describes a window already being watched. Event masks are per-client, so the window's owner is unaffected, and this is a read-only interest rather than theSetPropWmarker the Windows backend writes into a foreign window.get_geometry, not before: the server writes events and replies down one ordered stream, so aDestroyNotifygenerated before that reply is already queued locally by the time the reply lands. A destroyed window whose id nobody reused makesget_geometryfail instead, which was already terminal. Draining every frame is also what keeps x11rb's event queue from growing for the life of the stream.ConfigureNotifyrides the same subscription and can later replace the per-frameget_geometryround trip. That is out of scope here;/quest/m2/x11-capture-shm.mdowns it, and its plan is updated to say the subscription now exists.What a reviewer with an X session should check
window destroyed. A small Xlib program that creates, destroys, and recreates a window in a tight loop is enough to hit it.resized from ..., cursor blending still works, and display (non-window) capture is untouched.Test plan
just fix,just check,just teston macOS, all clean. All three skip the changed file, sincecapture/x11.rsis Linux-only.x11.rswas additionally run throughrustfmtdirectly, because macOSjust fixdoes not reachcfg-gated modules.moq-video --features capturetox86_64-unknown-linux-gnuwas attempted (rustup's Linux std, zig as the C/C++ cross compiler). The whole Rust dependency graph and openh264's C++ built for Linux; it stops atv4l2-sys-mit's bindgen, which needs a real cross libc header set. So the changed file itself was not compiled for Linux.change_window_attributeswithChangeWindowAttributesAux::new().event_mask(EventMask::STRUCTURE_NOTIFY),VoidCookie::check,Connection::poll_for_event,Event::DestroyNotify) was lifted verbatim into a throwaway crate and built and run against x11rb 0.14 on the host, so the calls type-check and the match behaves.enqueue_packetto confirm the drain cannot swallow an error a live cookie is waiting for: errors matching a sent request go topending_replies, and only unclaimed ones reach the event queue.a_destroyed_window_ends_capture_even_when_its_id_lives_on. It is a genuine but partial regression test: it pins the "is this event about my window" decision, not the round trip to a real server, which is the part that needs an X session.Public API changes
None. The subscription,
Capture::destroyed, anddestroysare all private to the module. (moq-videois0.0.xregardless.)Cross-package sync
No rows apply: no wire format, catalog, FFI, or CLI surface changed.
Quest
Completes and deletes
/quest/m0/x11-window-identity.md, drops its entry from/quest/m0/README.md, and refreshes the sentence in/quest/m2/x11-capture-shm.mdthat assumed no event subscription existed yet.🤖 Generated with Claude Code
(Written by Claude Opus 5)