Created attachment 257863 [details] [review]
GtkWidget: Avoid lingering clock frame updates

For some widgets, like GtkTreeView, which setup a clock frame
update during realize, it was possible to call
gdk_frame_clock_begin_updating() twice, but only ever disconnecting
from it once.

Keep the signal handler ID from us connecting to the "update" signal
to avoid connecting to it twice.

This fixes high wake-up count from any application using GtkTreeView,
even idle ones.

Created attachment 257864 [details] [review]
GtkScrolledWindow: Disconnect from frame clock properly

The tick callback IDs from GtkWidget aren't timeouts, so
use the correct function to disconnect from them.

Spotted by Benjamin Otte <otte@redhat.com>

The backtrace for the patch in comment 1:
Breakpoint 1, gdk_frame_clock_begin_updating (frame_clock=0x73a670) at gdkframeclock.c:317
317	{

+ Trace 232658

And the test case:
#include <gtk/gtk.h>

int main (int argc, char **argv)
{
        GtkWindow *window;
        GtkWidget *treeview;

        gtk_init (&argc, &argv);

        window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
        treeview = gtk_tree_view_new (); 
        gtk_container_add (GTK_CONTAINER (window), treeview);
        gtk_widget_realize (treeview);

        gtk_widget_show_all (GTK_WIDGET (window));

        gtk_main (); 

        return 0;
}

And to see the excessive and short timeouts, against glib:
diff --git a/glib/gmain.c b/glib/gmain.c
index a66739e..ea805b2 100644
--- a/glib/gmain.c
+++ b/glib/gmain.c
@@ -3362,6 +3362,10 @@ g_main_context_prepare (GMainContext *context,
 
                   if (source_timeout < 0 || timeout < source_timeout)
                     source_timeout = timeout;
+
+                  if (source_timeout > 0 && source_timeout <= 20)
+                    g_message ("Source %p '%s' has very low timeout %d",
+                              source, g_source_get_name (source), source_timeout);
                 }
             }

Review of attachment 257864 [details] [review]:

Looks good. Should we add a warning to the end of gtk_widget_remove_tick_callback() to catch such problems in the future?

Review of attachment 257863 [details] [review]:

For the comment I'd add to the first paragraph - "this happens because the realized flag is set at an unpredictable time by the widget's realize implementation"

This seems like an OK approach to me - the main other approach would be to move the code to connect/disconnect the frame clock from realize/unrealize into gtk_widget_set_realized() - but it would be hard to figure out why that's "special" compared to everything else that the realize wrapper function does.

There's a few nits in the checks where I think you have things that are extraneous

::: gtk/gtkwidget.c
@@ +4734,3 @@
     }
 
+  if (priv->tick_callbacks == NULL && priv->realized && priv->clock_tick_id)

the priv->realized check here is actually extraneous - this is OK because in gtk_widget_disconnect_frame_clock you removed the check on priv->tick_callbacks, but it would be clearer to omit it here.

@@ +4836,3 @@
   priv = widget->priv;
 
+  if (priv->tick_callbacks == NULL && priv->realized && !priv->clock_tick_id)

This reads strange a bit strange  - the priv->tick_callbacks == NULL is a transition check, but we're already checking for the transition with priv->clock_tick_id, so it would be better to omit the priv->tick_callbacks == NULL check here.

Review of attachment 257864 [details] [review]:

Somebody using gtk_widget_remove_tick_callback() instead of g_source_remove() seems unlikely, but we could add one to g_source_remove(), but it returns FALSE when the source wasn't found (which either it did in those cases, or removed some random other source).

Created attachment 257882 [details] [review]
GtkWidget: Avoid lingering clock frame updates

For some widgets, like GtkTreeView, which setup a clock frame
update during realize, it was possible to call
gdk_frame_clock_begin_updating() twice, but only ever disconnecting
from it once. This happens because the realized flag is set at an
unpredictable time by the GtkWidget's realize implementation.

Keep the signal handler ID from us connecting to the "update" signal
to avoid connecting to it twice.

This fixes high wake-up count from any application using GtkTreeView,
even idle ones.

Review of attachment 257882 [details] [review]:

Looks good

Review of attachment 257882 [details] [review]:

Looks good

Attachment 257864 [details] pushed as 255fafb - GtkScrolledWindow: Disconnect from frame clock properly
Attachment 257882 [details] pushed as 7c12e64 - GtkWidget: Avoid lingering clock frame updates

Pushed to gtk-3-8, gtk-3-10 and master. Thanks for the reviews.

(In reply to comment #6)
> Review of attachment 257864 [details] [review]:
> 
> Somebody using gtk_widget_remove_tick_callback() instead of g_source_remove()
> seems unlikely, but we could add one to g_source_remove(), but it returns FALSE
> when the source wasn't found (which either it did in those cases, or removed
> some random other source).

Yeah I was thinking about it backwards.

If g_source_remove() doesn't already warn, there are doubtless thousands of
places out there in the wild where applications remove sources twice or try to
remove source 0 - I don't think it makes sense to introduce a warning and have
to go clean those all up - and as you say the return value implies that it is
legitimate to remove an ID that is already removed, though I can't think of any
reasonable reason to actually do that and test the return value.