search

linuxmint / muffin

The window management library for the Cinnamon desktop (libmuffin) and its sample WM binary (muffin)
GNU General Public License v2.0
197 stars 93 forks source link

Ignore XRRSetMonitor() errors and delete existing monitors with same name #693

Closed jknockel closed 6 months ago

jknockel commented 6 months ago

Ignore server errors on XRRSetMonitor(). A similar fix appears in mutter [1].

The RandR protocol [2] states concerning RRSetMonitor that:

If 'name' matches an existing Monitor on the screen, the existing one will be deleted as if RRDeleteMonitor were called.

Unfortunately, this behavior is not the case in practice, and if an existing monitor with the same name exists the server generates a BadValue error [3]. While this was fixed very recently in the Xorg xserver [3], it is unclear when these changes will make it to most distributions due to how long it has been since a formal Xorg xserver major release. Therefore, we must ignore this error to prevent muffin from aborting if a monitor with the same name already exists, a common condition upon restarting cinnamon or calling cinnamon --replace.

I used the low-level XSync() and XSetErrorHandler() calls because to my knowledge no MetaDisplay handles are available here and therefore APIs like meta_x11_error_trap_push() appear unavailable. However, if they were available these APIs would be a cleaner approach. The original mutter fix uses an API (mtk_x11_error_trap_push) that does not yet appear in muffin [1].

A word of warning to those that think that xcb may be the perfect tool for this job: a crashing bug was only recently fixed in libxcb [4] which caused all calls to xcb_randr_set_monitor() to crash or otherwise cause the calling program to behave in an undefined manner. While the fix is released in libxcb 1.17, this version is not even available in Ubuntu 24.04, for instance. Therefore, it may be some time before we can reliably call xcb_randr_set_monitor().

Unfortunately, the above is not sufficient for correct behavior. The aim of ignoring XRRSetMonitor() errors was merely to prevent muffin from aborting if a monitor with the same name as one we are setting already exists. Merely ignoring the problem is fine if the monitor outputs that we tried to set happen to be the ones already set on that monitor. However, if XRRSetMonitor() fails, then the existing monitor may have different outputs than the ones which we tried to set.

To work around this, we first try to delete any monitor with the new monitor's name using XRRDeleteMonitor(). This might fail if no such monitor exists, so ignore server errors from this call as well. Then call XRRSetMonitor() as before.

[1] https://gitlab.gnome.org/GNOME/mutter/-/commit/8d3696f39a0b3af725b7615f7e2ac74ce5e0bcbf [2] https://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt [3] https://gitlab.freedesktop.org/xorg/xserver/-/commit/146bb9b2c19fb75b7629b65d5871969b7fcef97a [4] https://gitlab.freedesktop.org/xorg/lib/libxcb/-/commit/038636786ad1914f3daf3503ae9611f40dffbb8f

jknockel commented 6 months ago

Thinking about this some more, another option may be to use XCB for the RRDeleteMonitor() call, since that becomes the risky operation, and then use traditional Xlib without any XSync() or XSetErrorHandler() for the RRSetMonitor() call, since that is no longer risky. I don't know if that is any cleaner though.

Another thing I forgot to mention is, why not just try to use a unique monitor name each time? This works too, but the downside is that these monitors never get deleted and you end up leaking them (e.g., they are persistently visible in xrandr --listactivemonitors).

mtwebster commented 6 months ago

Hi, you should be safe to #include "core/display-private.h" in this file, which contains meta_get_display () to allow you to use the meta pop/push functions.

mtwebster commented 6 months ago

I think it's ok as you have it - you're working around the current randr bug, and it should continue to work even if/when the xserver fix is released.

I'd rather keep it simple for now, especially since I assume this isn't a 100% repeatable bug in typical situations is it?

jknockel commented 6 months ago

Hi, you should be safe to #include "core/display-private.h" in this file, which contains meta_get_display () to allow you to use the meta pop/push functions.

For some reason meta_get_display () returns NULL when I try calling it here, so I've gone with switching out to XCB for calling DeleteMonitor. The push/pop functions if working would be even simpler, but XCB seems still simpler than my previous approach.

I think it's ok as you have it - you're working around the current randr bug, and it should continue to work even if/when the xserver fix is released.

I'd rather keep it simple for now, especially since I assume this isn't a 100% repeatable bug in typical situations is it?

I believe that it is not repeatable in typical user setups which do not involve tiled monitors, but when plugged into my tiled monitor a fatal Xlib error is 100% repeatable on cinnamon --replace or Troubleshoot -> Restart Cinnamon.