Crash on monitor hotplugging

galibert commented 7 months ago

Upfront Information

Please provide the following information by running the command and providing the output.

Fvwm3 version (run: fvwm3 --version)

fvwm3 1.1.0 (1.0.9-15-g4af6dbe6) with support for: ReadLine, XPM, PNG, SVG, Shape, XShm, SM, Bidi text, XRandR, XRender, XCursor, XFT, NLS

Linux distribution or BSD name/version

Arch

Platform (run: uname -sp)

Linux unknown

Expected Behaviour

Not crash :-)

Actual Behaviour

My laptop has one external screen connected to a dock and another to the hdmi port. When disconnecting the dock and cable nothing much happens (for some reason the desktop is not automatically reconfigured by X, maybe to allow reconnecting?). Then I go under arandr to switch off the screens and change the primary to the laptop screen. Fvwm3 segfaults at that point.

This happens because toggle_prev_monitor_state gets prev = nullptr due to the monitor not existing anymore, hence monitor_resolve_name(prev_focused_monitor) returning nullptr.

Ideally fvwm3 should follow the RANDR events, but at least if it didn't crash it would be possible to put things back in place through a simple restart. If I do arandr, restart then unplug I have no crash.

Does the problem also happen with Fvwm2?

fvwm2 randomly crashes in xcb in such a configuration, either at restart time or when a notification happens.

ThomasAdam commented 6 months ago

Hi @galibert

Yeah, OK. Presumably the following helps?

diff --git a/fvwm/events.c b/fvwm/events.c
index 270fa1b4a..0487e48c6 100644
--- a/fvwm/events.c
+++ b/fvwm/events.c
@@ -2292,6 +2292,12 @@ void HandleEnterNotify(const evh_args_t *ea)
                pfm = monitor_resolve_name(prev_focused_monitor);
                this_m = monitor_get_current();

+               /* Don't toggle the previous monitor if there isn't one, or
+                * the two monitors are the same.
+                */
+               if ((pfm == NULL) || (pfm == this_m))
+                       return;
+
                /* Send MX_MONITOR_FOCUS event. */
                toggle_prev_monitor_state(this_m, pfm, NULL);

galibert commented 6 months ago

It does indeed, thanks :-)

fvwmorg / fvwm3