Created attachment 192336 [details] [review]
patch to fix the clock getting out of sync

Glib implements its g_timeout_* timers using CLOCK_MONOTONIC, which, at least on Linux, completely ignores suspend/resume. Therefore, updating the panel clock using g_timeout_add() once per minute (as gnome-screensaver is currently doing) results in the wrong time being displayed on resume from suspend.

This patch adds a cheap (low impact on power, and no UI updates in the default case) 2-second timer to check that the panel clock has not gotten out of sync with the machine's clock (e.g. when the machine resumes from suspend or hibernate, or if the machine's clock is reset by ntp).

In addition, the patch fixes the watchdog timer to use g_timeout_add_seconds() to save a bit of power.

I'm not sure I like the idea of having two timeouts.

I say we either drop the optimization here in the code:

tatic void•
queue_clock_update (GSWindow *window)•
{•
        int timeouttime;•
        struct timeval tv;•
•
        gettimeofday (&tv, NULL);•
        timeouttime = (G_USEC_PER_SEC - tv.tv_usec) / 1000 + 1;•
•
        /* timeout of one minute if we don't care about the seconds */•
        timeouttime += 1000 * (59 - tv.tv_sec % 60);•
•
        window->priv->clock_update_id = g_timeout_add (timeouttime, (GSourceFunc)update_clock_timer, window);•
}•

(so just do g_timeout_add_seconds (1, ...))

or we use timerfd_create(CLOCK_REALTIME,  TFD_TIMER_CANCELON_SET) instead of g_timeout_add .

I think the former is easiest for now, we should potentially think about adding the latter to gdatetime in glib.

i filed Bug 655129 to handle the glib rfe

(In reply to comment #2)
> (so just do g_timeout_add_seconds (1, ...))

I don't like the idea of a clock on my computer that might be up to 1 second off. It's not a 15-year old microwave oven with a dodgy rtc chip, it's a modern laptop with ntp. Its clocks are supposed to be *accurate*.

> or we use timerfd_create(CLOCK_REALTIME,  TFD_TIMER_CANCELON_SET) instead of
> g_timeout_add .
> 
> I think the former is easiest for now, we should potentially think about adding
> the latter to gdatetime in glib.

That would be the "proper" solution, and having such an API in glib would be extremely useful, but it would mean users don't see the result until gnome-3.2.


Incidentally: my memory may be failing me, but I don't think the gnome2 clock applet suffered from the skew problem. Unfortunately, I don't have a gnome-2 machine to test.

That's fine, the add_seconds wasn't my main point.  My points are:

1) We don't need two timeouts running at the same time.
2) We don't have a way right now to get notifications about skew, but will if/when bug 655129 get fixed
3) the minutes-only case has the bug, but the minutes-seconds case doesn't, since it updates every second anyway.
4) in the interim, before bug 655129 gets fixed, we should just make the minutes-only case wake up as frequently or nearly as frequently as the minutes-seconds case

Interested in cooking a patch without the second timeout?

Created attachment 192523 [details] [review]
patch to fix the clock getting out of sync, v2

OK, here is a patch that uses only one timeout, both to check whether the clock has gotten out of sync (using g_timeout_add_seconds) and to update the clock on predicted minute boundaries (using g_timeout_add). Plus, it should be a tiny bit more efficient than my previous patch since it's allocating GDateTime objects once per minute instead of once every 2 seconds.

Created attachment 192524 [details] [review]
patch to fix the clock getting out of sync, v3

Same as above, but without an unused variable (somehow missed it until the patch was already uploaded).

Thanks for the help, I've pushed your patch with minor changes.

http://git.gnome.org/browse/gnome-screensaver/commit/?id=742b77d217dced55b8b07616bf6a70ec167ffc01
http://git.gnome.org/browse/gnome-screensaver/commit/?id=9f98f3a20b5eec6a1fa13e5a4972eadc2d728acf