forked from unom/punktfunk
fix(pf-vdisplay): serialize Mutter monitor rebuilds end-to-end, not just our D-Bus calls
Chaining two clients through a kept (keep-alive) Mutter display segfaults gnome-shell in meta_monitor_manager_rebuild (libmutter-18) and takes the whole desktop down; every later session then fails RemoteDesktop.CreateSession: ServiceUnknown until GDM restarts. A/B'd on .21: byte-identical on released 0.27.0 and the 0.28.0 RC, so it was never a regression — the trigger has been there all along. TOPOLOGY_LOCK already serialized every topology-mutating D-Bus call, but two gaps still let Mutter's REBUILDS overlap: - Teardown was fire-and-forget: StopGuard::drop set a flag and returned, and the session thread only noticed on its ≤200 ms park tick. The A2 dead-reuse path (reused kept display dead on first frame → mark_failed → re-create) therefore issued its fresh RecordVirtual with the doomed monitor's removal still pending — the fresh session could even win the lock BEFORE the old thread had woken to take it, adding a monitor while the dead one still stood. The drop now waits (bounded, 20 s) for the session thread to finish. - The lock was released while the shell was still rebuilding: Stop / RecordVirtual / ApplyMonitorsConfig all return mid-rebuild, and a temporary (APPLY_TEMPORARY) config auto-reverts asynchronously on top. Every locked mutation section now ends with settle_topology() — poll GetCurrentState until a removed connector is actually gone and the config serial holds still across two consecutive reads — before the guard drops. Bounded at 4 s and best-effort (a read error means the shell is gone; a hotplug storm must not park sessions), degrading to exactly the old behavior. Cost when Mutter is already quiet: one confirming read plus one 150 ms recheck per setup/teardown. The live_mutter_create_drop harness sheds its grace sleep — the synchronous drop IS the teardown confirmation now. Not fixed here, documented on TOPOLOGY_LOCK: the mid-stream mode-switch rebuild is create-before-drop by design (H2), so its RecordVirtual still lands while the superseded monitor exists; the settle makes Mutter quiescent at that point but cannot remove the coexistence itself.
This commit is contained in:
@@ -75,6 +75,20 @@ const CURSOR_EMBEDDED: u32 = 1;
|
||||
/// appearing at once, "the connector absent from MY pre-snapshot" can name a sibling's monitor.
|
||||
/// Each session runs on its own dedicated thread (see [`session_thread`]), so blocking on a std
|
||||
/// mutex — including across the awaits of its single-threaded setup future — is safe.
|
||||
///
|
||||
/// The lock alone is NOT enough, because Mutter's rebuilds outlive our D-Bus calls: `Stop` /
|
||||
/// `RecordVirtual` / `ApplyMonitorsConfig` return while the shell is still rebuilding (and, for a
|
||||
/// session whose config was applied `APPLY_TEMPORARY`, still auto-reverting it). Releasing the lock
|
||||
/// at that point hands the next session a NON-QUIESCENT Mutter, and its first mutation rebuilds
|
||||
/// concurrently with the leftover one — the exact `meta_monitor_manager_rebuild` SIGSEGV again,
|
||||
/// reproduced on 2026-08-08 (mid-bringup mode switch: two `RecordVirtual`s ~1 s apart) and
|
||||
/// 2026-08-13 (keep-alive reuse dead on first frame → teardown + immediate re-create; A/B'd
|
||||
/// identical on 0.27.0 and the 0.28.0 RC, so it was never a regression). So every locked mutation
|
||||
/// section ends with [`settle_topology`] — poll DisplayConfig until the change is visible and the
|
||||
/// config serial stops moving — BEFORE the guard drops. And because ordering across sessions runs
|
||||
/// through the keepalive drop, [`StopGuard`]'s `Drop` must be SYNCHRONOUS (wait for the session
|
||||
/// thread to finish its Stop + settle): a fire-and-forget flag let the A2 re-create win the lock
|
||||
/// before the doomed session's thread had even woken to take it.
|
||||
static TOPOLOGY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
|
||||
|
||||
/// The Mutter virtual-display driver. Each [`create`](VirtualDisplay::create) spins up a
|
||||
@@ -183,11 +197,18 @@ impl VirtualDisplay for MutterDisplay {
|
||||
let (setup_tx, setup_rx) = std::sync::mpsc::channel::<Result<u32, String>>();
|
||||
let stop = Arc::new(AtomicBool::new(false));
|
||||
let stop_thread = stop.clone();
|
||||
// Teardown confirmation: the sender lives exactly as long as the session thread, so the
|
||||
// guard's `Drop` can WAIT on `Disconnected` for the thread to finish its Stop + settle
|
||||
// (see TOPOLOGY_LOCK — the drop is the only happens-before edge ordering "old monitor
|
||||
// removed" against "next monitor created").
|
||||
let (done_tx, done_rx) = std::sync::mpsc::channel::<()>();
|
||||
let first_in_group = self.first_in_group;
|
||||
let hw_cursor = self.hw_cursor;
|
||||
thread::Builder::new()
|
||||
.name("punktfunk-mutter-vout".into())
|
||||
.spawn(move || {
|
||||
// Dropped when the thread returns — every exit path signals `done_rx`.
|
||||
let _done = done_tx;
|
||||
session_thread(
|
||||
setup_tx,
|
||||
stop_thread,
|
||||
@@ -204,7 +225,10 @@ impl VirtualDisplay for MutterDisplay {
|
||||
// that finishes after we gave up then parks for at most one 200 ms tick. `report_node` is
|
||||
// the primary defence (it stops the session outright); this is the belt-and-braces half,
|
||||
// and it also covers a thread that is somewhere else entirely when the timeout fires.
|
||||
let guard = StopGuard(stop);
|
||||
let guard = StopGuard {
|
||||
stop,
|
||||
done: done_rx,
|
||||
};
|
||||
|
||||
// 45 s (was 20 s): setups now queue on TOPOLOGY_LOCK, so a session behind a slow sibling
|
||||
// (whose guard spans up to a ~10 s stream wait + 6 s connector wait + the apply) must
|
||||
@@ -230,11 +254,35 @@ impl VirtualDisplay for MutterDisplay {
|
||||
|
||||
/// Dropping this ends the keepalive thread, closing the D-Bus connection — Mutter then tears
|
||||
/// the remote-desktop + screencast sessions (and the virtual monitor) down.
|
||||
struct StopGuard(Arc<AtomicBool>);
|
||||
///
|
||||
/// The drop is SYNCHRONOUS: it waits (bounded) for the session thread to confirm the teardown —
|
||||
/// Stop issued, the monitor removal settled under [`TOPOLOGY_LOCK`]. The registry drops these
|
||||
/// outside its pool lock and documents that the drop may block, and the callers that immediately
|
||||
/// re-create (the A2 dead-reuse teardown, a mode-switch retire) are exactly the ones that NEED the
|
||||
/// wait: with the old fire-and-forget flag, the fresh session's `RecordVirtual` could win
|
||||
/// `TOPOLOGY_LOCK` before this session's thread had woken (≤200 ms park tick) to take it, adding a
|
||||
/// monitor while the doomed one still stood — gnome-shell then died rebuilding the monitor manager
|
||||
/// (`meta_monitor_manager_rebuild`, 2026-08-13, byte-identical on 0.27.0 and the 0.28.0 RC).
|
||||
struct StopGuard {
|
||||
stop: Arc<AtomicBool>,
|
||||
/// Signals `Disconnected` when the session thread — which owns the paired sender — returns.
|
||||
done: std::sync::mpsc::Receiver<()>,
|
||||
}
|
||||
|
||||
impl Drop for StopGuard {
|
||||
fn drop(&mut self) {
|
||||
self.0.store(true, Ordering::Relaxed);
|
||||
self.stop.store(true, Ordering::Relaxed);
|
||||
// Generous: teardown is one ~200 ms park tick + Stop + a ≤4 s settle, but the thread may
|
||||
// first have to outwait a sibling's setup holding TOPOLOGY_LOCK (up to ~16 s of stream +
|
||||
// connector waits). Timing out is degraded-but-safe: the next mutation still queues on the
|
||||
// lock; only the wake-up ordering guarantee is lost.
|
||||
match self.done.recv_timeout(Duration::from_secs(20)) {
|
||||
Ok(()) | Err(std::sync::mpsc::RecvTimeoutError::Disconnected) => {}
|
||||
Err(std::sync::mpsc::RecvTimeoutError::Timeout) => tracing::warn!(
|
||||
"mutter: virtual-output teardown did not confirm within 20 s — proceeding; the \
|
||||
next topology mutation may race the shell's rebuild"
|
||||
),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -318,6 +366,10 @@ fn session_thread(
|
||||
Ok(s) => s,
|
||||
Err(e) => {
|
||||
let _ = setup_tx.send(Err(format!("{e:#}")));
|
||||
// A half-built session can still have ADDED the monitor (`RecordVirtual` succeeded,
|
||||
// the node-id wait didn't) — its connections dropped inside `connect`, so Mutter is
|
||||
// now removing it. Settle that rebuild before the guard releases the lock.
|
||||
settle_topology(None, None).await;
|
||||
return;
|
||||
}
|
||||
};
|
||||
@@ -325,6 +377,10 @@ fn session_thread(
|
||||
// mutates the operator's desktop topology on behalf of a session that, past this point,
|
||||
// would have no way to undo it.
|
||||
if !report_node(&setup_tx, &session).await {
|
||||
// `report_node` already stopped the session — the virtual monitor is being removed.
|
||||
// Settle under the still-held lock (same reasoning as the teardown below).
|
||||
drop(session);
|
||||
settle_topology(None, None).await;
|
||||
return;
|
||||
}
|
||||
// The send can also LAND in the moment the opener's `recv_timeout` gives up — the value sits
|
||||
@@ -338,6 +394,8 @@ fn session_thread(
|
||||
the desktop topology"
|
||||
);
|
||||
let _ = session.rd_session.call_method("Stop", &()).await;
|
||||
drop(session);
|
||||
settle_topology(None, None).await;
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -382,6 +440,11 @@ fn session_thread(
|
||||
}
|
||||
}
|
||||
|
||||
// The lock's promise is "one rebuild at a time", which holds only if the rebuilds THIS
|
||||
// setup caused — the `RecordVirtual` add, the `ApplyMonitorsConfig`, and Mutter's own
|
||||
// auto-revert of any sibling's temporary config — are finished before it is released.
|
||||
// Cheap when Mutter is already quiet (one confirming read + one 150 ms recheck).
|
||||
settle_topology(tracked.as_ref().map(|(dc, _, _)| dc), None).await;
|
||||
drop(topology_guard);
|
||||
|
||||
// Park, keeping `session` (and its zbus connection) alive until told to stop. Every ~5 s,
|
||||
@@ -414,13 +477,96 @@ fn session_thread(
|
||||
// the virtual output disappears and our DisplayConfig connection (in `tracked`) closes — so we
|
||||
// just drop it here and let the revert happen Mutter-side, never touching the layout ourselves.
|
||||
// The Stop (+ the revert it triggers) is a topology mutation too — take TOPOLOGY_LOCK so a
|
||||
// sibling's teardown or setup can't interleave with the rebuild it causes.
|
||||
// sibling's teardown or setup can't interleave with the rebuild it causes. And HOLD it
|
||||
// until the removal has actually settled: `Stop` returns while the shell is still
|
||||
// rebuilding, and the very next thing after this teardown is often a fresh create (the A2
|
||||
// dead-reuse re-create, a mode-switch retire) whose `RecordVirtual` must not land in that
|
||||
// window — that overlap is the reproduced `meta_monitor_manager_rebuild` SIGSEGV.
|
||||
let _topology_guard = TOPOLOGY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
|
||||
let _ = session.rd_session.call_method("Stop", &()).await;
|
||||
let vconn = tracked.as_ref().map(|(_, _, v)| v.clone());
|
||||
// Close our own handles FIRST — the APPLY_TEMPORARY revert waits on the DisplayConfig
|
||||
// connection in `tracked` closing (see above) — then observe the settle on a fresh one.
|
||||
drop(tracked);
|
||||
drop(session);
|
||||
settle_topology(None, vconn.as_deref()).await;
|
||||
});
|
||||
}
|
||||
|
||||
/// Wait, bounded, for Mutter's monitor topology to go QUIET — called at the end of every
|
||||
/// [`TOPOLOGY_LOCK`]-holding mutation section, before the guard drops (see the lock's docs for the
|
||||
/// two field crashes this closes). Two phases, both polled over `GetCurrentState`:
|
||||
///
|
||||
/// 1. when `gone` names a just-removed virtual connector, wait until it is actually absent (its
|
||||
/// `Stop` returned before the shell finished the removal rebuild);
|
||||
/// 2. wait until the config serial holds still across two consecutive reads — the rebuilds we
|
||||
/// caused (add/remove/apply, plus Mutter's own auto-revert of a temporary config) each bump it.
|
||||
///
|
||||
/// `dc` reuses the session's open DisplayConfig proxy when it has one; otherwise a fresh
|
||||
/// short-lived connection is opened (the teardown path deliberately closes its own first — the
|
||||
/// APPLY_TEMPORARY revert waits on that close). Best-effort by design: a read error usually means
|
||||
/// the shell is gone (crashed or logging out), and the deadline keeps an unrelated hotplug storm
|
||||
/// from parking a session forever — both degrade to "proceed", which is exactly the old behavior.
|
||||
async fn settle_topology(dc: Option<&zbus::Proxy<'_>>, gone: Option<&str>) {
|
||||
let fresh;
|
||||
let dc = match dc {
|
||||
Some(p) => p,
|
||||
None => match display_config().await {
|
||||
Ok(p) => {
|
||||
fresh = p;
|
||||
&fresh
|
||||
}
|
||||
Err(_) => {
|
||||
// Nothing to observe (no DisplayConfig — a crashed shell?): a fixed grace still
|
||||
// beats returning into the next mutation instantly.
|
||||
tokio::time::sleep(Duration::from_millis(300)).await;
|
||||
return;
|
||||
}
|
||||
},
|
||||
};
|
||||
let started = Instant::now();
|
||||
let deadline = started + Duration::from_secs(4);
|
||||
if let Some(conn) = gone {
|
||||
loop {
|
||||
match get_state(dc).await {
|
||||
Ok(s) if !connectors(&s).contains(conn) => break,
|
||||
Ok(_) if Instant::now() < deadline => {
|
||||
tokio::time::sleep(Duration::from_millis(100)).await;
|
||||
}
|
||||
_ => break, // read error (shell gone) or deadline — proceed either way
|
||||
}
|
||||
}
|
||||
}
|
||||
let mut last: Option<u32> = None;
|
||||
loop {
|
||||
match get_state(dc).await {
|
||||
Ok(s) => {
|
||||
if last == Some(s.0) {
|
||||
break;
|
||||
}
|
||||
last = Some(s.0);
|
||||
}
|
||||
Err(_) => break,
|
||||
}
|
||||
if Instant::now() >= deadline {
|
||||
tracing::warn!(
|
||||
"mutter: the monitor topology did not settle within 4 s — proceeding (a concurrent \
|
||||
hotplug?)"
|
||||
);
|
||||
break;
|
||||
}
|
||||
tokio::time::sleep(Duration::from_millis(150)).await;
|
||||
}
|
||||
let waited = started.elapsed();
|
||||
if waited > Duration::from_millis(600) {
|
||||
tracing::info!(
|
||||
waited_ms = waited.as_millis() as u64,
|
||||
removed = gone.is_some(),
|
||||
"mutter: waited out a monitor-topology rebuild before releasing the lock"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Record an **existing** monitor by connector — the monitor-mirror path
|
||||
/// (`design/per-monitor-portal-capture.md` L2). Returns the PipeWire node id and the keepalive
|
||||
/// whose drop stops the recording.
|
||||
@@ -1543,10 +1689,13 @@ mod tests {
|
||||
);
|
||||
|
||||
std::thread::sleep(std::time::Duration::from_secs(3));
|
||||
// The keepalive's Drop is synchronous: it returns only once the session thread has run the
|
||||
// Stop and settled the removal rebuild (see `StopGuard`), so no grace sleep is needed.
|
||||
let dropped_at = std::time::Instant::now();
|
||||
drop(out);
|
||||
// The keepalive's Drop only SIGNALS the thread; give it more than one 200 ms tick to run
|
||||
// the Stop + topology revert before the harness exits and takes the process with it.
|
||||
std::thread::sleep(std::time::Duration::from_secs(2));
|
||||
println!("dropped — gnome-shell should have removed the monitor and reverted the topology");
|
||||
println!(
|
||||
"dropped in {:?} — gnome-shell should have removed the monitor and reverted the topology",
|
||||
dropped_at.elapsed()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user