GNOME Bugzilla – Bug 707302
MetaIdleMonitor: fire immediately watches that are already expired
Last modified: 2013-09-16 20:39:49 UTC
The XSync semantics mandate that alarms already expired will not fire until the counter is reset and the alarm triggered again, so clients traditionally called get_idle_time() first to see if they should install the alarm. This is inherently racy, as by the time the call is handled by mutter and the reply received the idle time could be different. Instead, if we see that the watch would have fired in the past, fire it immediately. This is a behavior change, but it's a compatible one, as all legacy clients are calling get_idle_time() first, and it was perfectly possible for the idle time counter to trigger the alarm right after the get_idle_time() call.
Created attachment 253898 [details] [review] MetaIdleMonitor: fire immediately watches that are already expired
Created attachment 253899 [details] [review] MetaIdleMonitor: fire immediately watches that are already expired The XSync semantics mandate that alarms already expired will not fire until the counter is reset and the alarm triggered again, so clients traditionally called get_idle_time() first to see if they should install the alarm. This is inherently racy, as by the time the call is handled by mutter and the reply received the idle time could be different. Instead, if we see that the watch would have fired in the past, fire it immediately. This is a behavior change, but it's a compatible one, as all legacy clients are calling get_idle_time() first, and it was perfectly possible for the idle time counter to trigger the alarm right after the get_idle_time() call. Even better, let's send the dbus reply before the signal, otherwise the client API will not recognize the ID and ignore it.
Review of attachment 253899 [details] [review]: makes sense. ::: src/core/meta-idle-monitor.c @@ +481,3 @@ + + if (meta_idle_monitor_get_idletime (monitor) > (gint64)timeout_msec) + g_idle_add (fire_watch_idle, watch); I think it would better to store the idle id on the monitor. So in the wayland case above you create a GSource, maybe it would make sense to use the same source field for this? Then you can avoid dispatching more than once if there's already one queued, and can clean it up in dispose if one is pending. That has the advantage of not having a struct field sitting around for the non-wayland case that will never get used too.
(In reply to comment #3) > Review of attachment 253899 [details] [review]: > > makes sense. > > ::: src/core/meta-idle-monitor.c > @@ +481,3 @@ > + > + if (meta_idle_monitor_get_idletime (monitor) > (gint64)timeout_msec) > + g_idle_add (fire_watch_idle, watch); > > I think it would better to store the idle id on the monitor. So in the wayland > case above you create a GSource, maybe it would make sense to use the same > source field for this? Then you can avoid dispatching more than once if there's > already one queued, and can clean it up in dispose if one is pending. That has > the advantage of not having a struct field sitting around for the non-wayland > case that will never get used too. I agree with storing the idle id, but not on the GSource, simply because this patch is for the x11 branch, and there is no GSource there.
Created attachment 255020 [details] [review] MetaIdleMonitor: fire immediately watches that are already expired The XSync semantics mandate that alarms already expired will not fire until the counter is reset and the alarm triggered again, so clients traditionally called get_idle_time() first to see if they should install the alarm. This is inherently racy, as by the time the call is handled by mutter and the reply received the idle time could be different. Instead, if we see that the watch would have fired in the past, fire it immediately. This is a behavior change, but it's a compatible one, as all legacy clients are calling get_idle_time() first, and it was perfectly possible for the idle time counter to trigger the alarm right after the get_idle_time() call.
Review of attachment 255020 [details] [review]: ++
Attachment 255020 [details] pushed as 2fc9e1a - MetaIdleMonitor: fire immediately watches that are already expired