GNOME Bugzilla – Bug 696522
only one user can invoke gnome_idle_monitor_add_user_active_watch() at a time
Last modified: 2013-03-27 17:16:36 UTC
Though gnome_idle_monitor_add_user_active_watch() returns a watch ID, it's a constant 1. Internally the function calls g_hash_table_insert() which will overwrite the previous watch.
Created attachment 239704 [details] [review] idleMonitor: Properly increment watch id in add_user_active_watch We should use get_next_watch_serial() to get a watch_id. --- Does this help?
Review of attachment 239704 [details] [review]: No, other places have USER_ACTIVE_WATCH_ID hardcoded.
Created attachment 239705 [details] [review] idleMonitor: Properly increment watch id in add_user_active_watch We should use get_next_watch_serial() to get a watch_id.
(In reply to comment #3) > Created an attachment (id=239705) [details] [review] > idleMonitor: Properly increment watch id in add_user_active_watch > > We should use get_next_watch_serial() to get a watch_id. OK how about this? (Only compiled tested).
That shouldn't have compiled either, and it's also wrong, as we still use g_hash_table_find to run watches for an alarm, and now there's more than one watch for an alarm. Patch set incoming.
(In reply to comment #5) > That shouldn't have compiled either, and it's also wrong, as we still use > g_hash_table_find to run watches for an alarm, and now there's more than one > watch for an alarm. Patch set incoming. That's not a reason for it not to compile but ok.
It shouldn't have compiled because you didn't change the use of USER_ACTIVE_WATCH_ID in get_next_watch_serial.
Created attachment 239706 [details] [review] idle-monitor: Allow multiple watches for an alarm This doesn't do anything yet as we don't have multiple watches for one alarm, but we'll remove the special USER_ACTIVE_WATCH_ID soon so we can have multiple watches on it.
Created attachment 239707 [details] [review] idle-monitor: Use an incrementing watch ID for the active alarm as well This makes sure that: add_user_active_alarm (callback1); add_user_active_alarm (callback2); doesn't remove callback1 from getting an alarm, as we won't overwrite the alarm IDs.
Created attachment 239708 [details] [review] idle-monitor: Reindent
Created attachment 239709 [details] [review] idle-monitor: Refactor code a bit Merge two similar methods.
(In reply to comment #7) > It shouldn't have compiled because you didn't change the use of > USER_ACTIVE_WATCH_ID in get_next_watch_serial. I didn't remove the constant.
Comment on attachment 239708 [details] [review] idle-monitor: Reindent Looks fine to commit to master.
Review of attachment 239706 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ -54,3 @@ typedef struct { - Display *display; Can you remove this in a separate patch please? @@ +168,3 @@ } + watches = g_hash_table_get_values (monitor->priv->watches); Use g_hash_table_foreach() directly, avoids the temporary list. @@ +252,2 @@ if (watch->id != USER_ACTIVE_WATCH_ID) { + XSyncDestroyAlarm (watch->monitor->priv->display, watch->xalarm); Separate patch, as per above.
Review of attachment 239707 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ +236,3 @@ get_next_watch_serial (void) { + static guint32 serial = 1; You can start at 0 as you increment it before returning it. @@ +257,3 @@ + if (watch->xalarm != monitor->priv->user_active_alarm) { + XSyncDestroyAlarm (monitor->priv->display, watch->xalarm); This change (the ->display change) should have happened in the previous patch, though I asked for the changes to be split off.
Review of attachment 239709 [details] [review]: Looks good for master.
(In reply to comment #14) > Review of attachment 239706 [details] [review]: > > ::: libgnome-desktop/gnome-idle-monitor.c > @@ +168,3 @@ > } > > + watches = g_hash_table_get_values (monitor->priv->watches); > > Use g_hash_table_foreach() directly, avoids the temporary list. The caller may call gnome_idle_monitor_remove_watch in the callback, which will modify the hash table during the foreach, triggering an assert.
(In reply to comment #17) > (In reply to comment #14) > > Review of attachment 239706 [details] [review] [details]: > > > > ::: libgnome-desktop/gnome-idle-monitor.c > > @@ +168,3 @@ > > } > > > > + watches = g_hash_table_get_values (monitor->priv->watches); > > > > Use g_hash_table_foreach() directly, avoids the temporary list. > > The caller may call gnome_idle_monitor_remove_watch in the callback, which will > modify the hash table during the foreach, triggering an assert. Can you make a comment to that effect above the call so nobody tries to "optimise" it later on?
Created attachment 239803 [details] [review] idle-monitor: Store the monitor in the private watch struct Since we'll need to access more things than just the Display on it, let's clean up the code for the access right now.
Created attachment 239804 [details] [review] idle-monitor: Allow multiple watches for an alarm This doesn't do anything yet as we don't have multiple watches for one alarm, but we'll remove the special USER_ACTIVE_WATCH_ID soon so we can have multiple watches on it.
Created attachment 239805 [details] [review] idle-monitor: Use an incrementing watch ID for the active alarm as well This makes sure that: add_user_active_alarm (callback1); add_user_active_alarm (callback2); doesn't remove callback1 from getting an alarm, as we won't overwrite the alarm IDs.
Created attachment 239806 [details] [review] idle-monitor: Don't leak the user active alarm on dispose
Created attachment 239807 [details] [review] idle-monitor: Reindent
Created attachment 239808 [details] [review] idle-monitor: Refactor code a bit Merge two similar methods.
Review of attachment 239803 [details] [review]: Looks safe and fine to me.
Review of attachment 239804 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ +172,1 @@ } Now we're rescheduling alarms even if we don't happen to have a watch set up for them. The gnome-shell process has at least 3 of these. While it's likely harmless for each Gnome.IdleMonitor to reschedule each other's user active watch, we should probably avoid it. Right?
Review of attachment 239805 [details] [review]: Looks correct.
Review of attachment 239806 [details] [review]: Makes sense.
Review of attachment 239807 [details] [review]: Ok.
Created attachment 239812 [details] [review] idle-monitor: Allow multiple watches for an alarm This doesn't do anything yet as we don't have multiple watches for one alarm, but we'll remove the special USER_ACTIVE_WATCH_ID soon so we can have multiple watches on it.
Review of attachment 239808 [details] [review]: This also looks right to me
Created attachment 239813 [details] [review] idle-monitor: Use an incrementing watch ID for the active alarm as well This makes sure that: add_user_active_alarm (callback1); add_user_active_alarm (callback2); doesn't remove callback1 from getting an alarm, as we won't overwrite the alarm IDs.
Created attachment 239814 [details] [review] idle-monitor: Don't leak the user active alarm on dispose
Created attachment 239815 [details] [review] idle-monitor: Reindent
Created attachment 239816 [details] [review] idle-monitor: Refactor code a bit Merge two similar methods.
Attachment 239803 [details] pushed as 0939890 - idle-monitor: Store the monitor in the private watch struct Attachment 239815 [details] pushed as 29f08dd - idle-monitor: Reindent Let's push these two for now.
Review of attachment 239812 [details] [review]: ::: libgnome-desktop/gnome-idle-monitor.c @@ +104,3 @@ +#else + return val; +#endif Urgh. In practice, no system GNOME is going to run on ships with sizeof(long) < sizeof(gpointer). Or to flip it around, only Win64 is "LLP". If we really wanted to microoptimize, we'd probably provide a set of macros in GLib to optimize for the common case and just use NULL/g_direct_hash for 32 bit and LP64 (all known Unix variants), and only fall back to g_malloc() on Win64/LLP. So let's drop this ugly stuff, add: G_STATIC_ASSERT(sizeof(long) == sizeof(gpointer)) and just pass NULL, NULL to g_hash_table_new()? @@ +328,3 @@ + g_hash_table_destroy (monitor->priv->alarms); + monitor->priv->alarms = NULL; + } g_clear_pointer (&monitor->priv->alarms, g_hash_table_destroy);
Created attachment 239822 [details] [review] idle-monitor: Use g_clear_pointer
Created attachment 239823 [details] [review] idle-monitor: Allow multiple watches for an alarm This doesn't do anything yet as we don't have multiple watches for one alarm, but we'll remove the special USER_ACTIVE_WATCH_ID soon so we can have multiple watches on it.
Created attachment 239824 [details] [review] idle-monitor: Use an incrementing watch ID for the active alarm as well This makes sure that: add_user_active_alarm (callback1); add_user_active_alarm (callback2); doesn't remove callback1 from getting an alarm, as we won't overwrite the alarm IDs.
Created attachment 239825 [details] [review] idle-monitor: Don't leak the user active alarm on dispose
Created attachment 239826 [details] [review] idle-monitor: Refactor code a bit Merge two similar methods.
Review of attachment 239822 [details] [review]: Looks good.
Review of attachment 239823 [details] [review]: I only have one minor comment (and feel free to keep as is). ::: libgnome-desktop/gnome-idle-monitor.c @@ +140,3 @@ { + GnomeIdleMonitorWatch *watch = data; + XSyncAlarm *alarm = user_data; Now that we're using casts we could do: XSyncAlarm alarm = (XSyncAlarm) user_data; @@ +179,3 @@ + g_list_foreach (watches, + fire_watch, + &alarm); And we'd cast to (gpointer) here rather than using &.
Review of attachment 239824 [details] [review]: This also looks right to me.
Review of attachment 239825 [details] [review]: Right.
Review of attachment 239826 [details] [review]: This one is still fine.
Attachment 239822 [details] pushed as fffcc2b - idle-monitor: Use g_clear_pointer Attachment 239823 [details] pushed as 634ef5e - idle-monitor: Allow multiple watches for an alarm Attachment 239824 [details] pushed as 7baf585 - idle-monitor: Use an incrementing watch ID for the active alarm as well Attachment 239825 [details] pushed as 0c60eec - idle-monitor: Don't leak the user active alarm on dispose Attachment 239826 [details] pushed as 6654c59 - idle-monitor: Refactor code a bit
Created attachment 239845 [details] [review] 0001-monitor-Be-defensive-against-callbacks-unreffing-mon.patch
Created attachment 239846 [details] [review] 0001-monitor-Be-defensive-against-callbacks-unreffing-mon.patch With less whitespace and a buglink
Review of attachment 239846 [details] [review]: Yes.
Comment on attachment 239846 [details] [review] 0001-monitor-Be-defensive-against-callbacks-unreffing-mon.patch Ok, so I think at this point we should consider merging the patches to gnome-3-8. Let's get r-t approval.
Patches look fine to me. I wonder if we shouldn't have a patch to test-idle-monitor.c to test the behaviour change here, though. Also, should there be a patch to add some details to the docs ? We've discovered quite a bit of subtle behaviour here that should really be mentioned in the docs.
cherry-picked for 3.8
See also https://bugzilla.gnome.org/show_bug.cgi?id=696719 for an important follow up patch.