Created attachment 153600 [details] [review]
0001-Avoid-a-race-in-detecting-the-idle-reset-alarm.patch

A few people in Ubuntu have reported their display blanking whilst not really idle, and seeing the status icon appear in the notification area which warns about a broken X server (and points to your blog here: http://blogs.gnome.org/hughsie/2009/08/17/gnome-power-manager-and-blanking-removal-of-bodges/) when the display unblanks again.

I've also seen this a couple of times. Firstly, my display dims because I've been idle for a few seconds. I immediately move the mouse, but the display never returns to normal brightness. Eventually, the display blanks momentarily whilst I'm still working and the tooltip tells me that my session isn't idle, but the screen is idle.

This isn't easily reproducible, but when doing some debugging I found a possible race. Basically, what I think might happen is:

1) Idle timeout alarm expires and egg_idletime_event_filter_cb is called
2) User moves mouse, and IDLETIME counter gets reset
3) egg_idletime_set_reset_alarm is called and creates the reset alarm, but we already missed the reset event.

I thought that this might seem far-fetched, as the time between 1 and 3 should be so short that there's no chance of the user performing 2 right in the middle. However, this time can be indeterminate if the kernel preempts gpm here and another task gets some CPU time in-between detecting the initial alarm and creating the reset alarm

I managed to recreate this reliably by adding a "sleep (1)" in between detecting the idle timeout and creating the reset alarm, and moving the mouse within this 1 second period after the display dims.

I've attached a patch which removes this race, and fixes it for the test case above (where I inserted a sleep). The patch creates the reset alarm when creating an EggIdleTime instance, rather than creating it after the idle timeout expires. I've ensured that the instance will not emit a "reset" signal unless the idle timeout alarm has triggered since the reset alarm last expired (so this doesn't change the behaviour of EggIdleTime). As the change means that the reset alarm is not created in response to the idle timeout expiring, the reset timeout has to be set to zero (as opposed to being based on the time of the idle timeout event)