Keep spawned children off the mpv Wayland proxy - #653
Open
PurpleDizzy wants to merge 1 commit into
Open
Conversation
`start_proxy` overrides the process-wide `WAYLAND_DISPLAY` so in-process
libmpv reaches the proxy instead of the compositor, but every process we
spawn inherits it too. `xdg-open`'s handler therefore connects to the mpv
proxy, where the first `get_xdg_surface` is demoted to a subsurface on the
assumption that the client is mpv. A browser starting cold then exports
that surface via `zxdg_exporter_v2`, the compositor rejects it ("exported
surface had an invalid role"), and the resulting fatal `wl_display` error
takes down the connection we share with mpv. The window disappears with
no crash, no signal and no core.
Clicking Download or any external link reproduces it, but only when the
handler is not already running: a warm browser hands the URL to its
existing instance over the singleton socket and exits before it ever
creates a Wayland surface, which is what makes this look intermittent.
Record the compositor's `WAYLAND_DISPLAY` before the override and set it
on the spawned command, so children reach the compositor while libmpv
still reaches the proxy. `open_url::open` is the only runtime spawn on
Linux, so this covers downloads, external links, About paths and
openConfigDir together. Leaving it unrecorded (X11, or the proxy never
started) keeps the inherited env untouched.
Covered by unit tests over `Command::get_envs` for the three cases:
unrecorded host, a recorded display, and a host that had none.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Clicking Download, or any external link, makes the window vanish instantly.
No crash dialog, no signal, no core dump, nothing in
dmesg. The app tearsitself down cleanly, which is what made this hard to spot.
It only reproduces when the
xdg-openhandler is not already running. Awarm browser hands the URL to its existing instance over the singleton socket
and exits before it ever creates a Wayland surface, so the bug looks
intermittent or config-specific.
Cause
start_proxyoverrides the process-wideWAYLAND_DISPLAYso in-processlibmpv reaches the mpv proxy instead of the compositor. Every process we spawn
inherits it, so
xdg-open's handler connects to the mpv proxy too, where thefirst
get_xdg_surfaceis demoted to a subsurface on the assumption that theclient is mpv. The browser then exports that surface via
zxdg_exporter_v2,the compositor rejects it, and the fatal
wl_displayerror takes down theconnection we share with mpv.
From
--log-level debug(niri, CEF 150.0.17,wayland-1compositor andwayland-2proxy):server_id=Some(91)is the browser, not mpv. mpv's own demote isserver_id=Some(8)at startup.Fix
Record the compositor's
WAYLAND_DISPLAYbefore the override and set it onthe spawned command, so children reach the compositor while libmpv still
reaches the proxy.
open_url::openis the only runtime spawn on Linux, sothis covers downloads, external links, About paths and
openConfigDirtogether. Leaving it unrecorded (X11, or the proxy never started) keeps the
inherited env untouched.
Testing
Command::get_envsfor the three cases: unrecorded host,a recorded display, and a host that had none.
just lintandjust testpass.dies on every Download; after it, the browser cold-starts and the app stays
up, with no second
demotingline and no protocol error.Note
This does not harden the proxy itself against non-mpv clients. Anything that
ends up with
WAYLAND_DISPLAYpointing at the proxy can still take the appdown, so rejecting or passing through clients that aren't mpv may be worth
doing separately. I kept this change to the path that actually spawns them.