I haven't used Valgrind in a while so apologies if this is a silly question: Is there any way to get more info from it about what it thinks is wrong? or the widget layout that causes it?

Because maybe I'm missing it, but I can't see any way in which an uninitialised value could be used within gtk_adjustment_clamp_page(); all local variables and members that it uses must are assigned, or were initialised by its new() or init() functions.

Right, the argument is --track-origins. My current command is:
 $ G_SLICE=always-malloc valgrind --num-callers=30 --track-origins=yes evolution

and then I double-click one message in the message list, which opens a new window with it. That's the time when the warning from valgrind is shown on the console. It's not only that one, there are more of them, I only pasted the first above. The above command adds basically this, with gtk 3.22.16:

  Uninitialised value was created by a stack allocation
    at 0x5D05A9D: gtk_container_real_set_focus_child (gtkcontainer.c:2671)

I changed the code in gtk+ and tried to initialize the x,y with zeros, like:
      gint x = 0, y = 0;
and it avoided the valgrind complaint, thus it seems that gtk_widget_translate_coordinates() didn't set that x, neither that y. Maybe a longer backtrace of the claim will make more sense? This one is
with --num-callers=51:

Thread 1:
Conditional jump or move depends on uninitialised value(s)
   at 0x5C8357D: gtk_adjustment_clamp_page (gtkadjustment.c:927)
   by 0x5D05DB5: gtk_container_real_set_focus_child (gtkcontainer.c:2720)
   by 0x694E2A2: g_cclosure_marshal_VOID__OBJECTv (gmarshal.c:2102)
   by 0x6949C4F: g_type_class_meta_marshalv (gclosure.c:1024)
   by 0x6949811: _g_closure_invoke_va (gclosure.c:867)
   by 0x69641E0: g_signal_emit_valist (gsignal.c:3300)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
   by 0x5D05628: gtk_container_set_focus_child (gtkcontainer.c:2497)
   by 0x5FE0CA0: gtk_widget_real_grab_focus (gtkwidget.c:8096)
   by 0x694C623: g_cclosure_marshal_VOID(intXX_t &&) volatile (gmarshal.c:905)
   by 0x6949C4F: g_type_class_meta_marshalv (gclosure.c:1024)
   by 0x6949811: _g_closure_invoke_va (gclosure.c:867)
   by 0x69641E0: g_signal_emit_valist (gsignal.c:3300)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
   by 0x5FE0A1F: gtk_widget_grab_focus (gtkwidget.c:8012)
   by 0x3A926406: mail_browser_message_selected_cb (e-mail-browser.c:290)
   by 0x694D9D9: g_cclosure_marshal_VOID__STRING (gmarshal.c:1754)
   by 0x6949579: g_closure_invoke (gclosure.c:804)
   by 0x6965AEB: signal_emit_unlocked_R (gsignal.c:3635)
   by 0x6964E22: g_signal_emit_valist (gsignal.c:3391)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
   by 0x3A9C45D1: message_list_select_uid (message-list.c:1235)
   by 0x3A9755F8: mail_reader_set_message (e-mail-reader.c:3431)
   by 0x3A927E63: mail_browser_set_message (e-mail-browser.c:899)
   by 0x3A979C0D: e_mail_reader_set_message (e-mail-reader.c:4924)
   by 0x3A96AD40: e_mail_reader_open_selected (e-mail-reader-utils.c:1490)
   by 0x3A963608: mail_paned_view_open_selected_mail (e-mail-paned-view.c:1104)
   by 0x3A962536: mail_paned_view_reader_open_selected_mail (e-mail-paned-view.c:662)
   by 0x3A979CFC: e_mail_reader_open_selected_mail (e-mail-reader.c:4937)
   by 0x4259042E: mail_shell_content_open_selected_mail (e-mail-shell-content.c:343)
   by 0x3A979CFC: e_mail_reader_open_selected_mail (e-mail-reader.c:4937)
   by 0x3A97145D: action_mail_message_open_cb (e-mail-reader.c:974)
   by 0x694C590: g_cclosure_marshal_VOID(intXX_t &&) volatile (gmarshal.c:875)
   by 0x6949579: g_closure_invoke (gclosure.c:804)
   by 0x6965AEB: signal_emit_unlocked_R (gsignal.c:3635)
   by 0x6964E22: g_signal_emit_valist (gsignal.c:3391)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
   by 0x5C00CB3: _gtk_action_emit_activate (gtkaction.c:909)
   by 0x5C00DA2: gtk_action_activate (gtkaction.c:942)
   by 0x3A9735BE: mail_reader_double_click_cb (e-mail-reader.c:2662)
   by 0x9809FE7: e_marshal_VOID__INT_POINTER_INT_BOXED (e-marshal.c:1369)
   by 0x6949579: g_closure_invoke (gclosure.c:804)
   by 0x6965AEB: signal_emit_unlocked_R (gsignal.c:3635)
   by 0x6964E22: g_signal_emit_valist (gsignal.c:3391)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
   by 0x98CBA99: item_double_click (e-tree.c:937)
   by 0x9809937: e_marshal_VOID__INT_INT_BOXED (e-marshal.c:1081)
   by 0x6949579: g_closure_invoke (gclosure.c:804)
   by 0x6965AEB: signal_emit_unlocked_R (gsignal.c:3635)
   by 0x6964E22: g_signal_emit_valist (gsignal.c:3391)
   by 0x6965364: g_signal_emit (gsignal.c:3447)
 Uninitialised value was created by a stack allocation
   at 0x5D05A9D: gtk_container_real_set_focus_child (gtkcontainer.c:2671)


The code in question in Evolution looks like this:
https://git.gnome.org/browse/evolution/tree/src/mail/e-mail-reader-utils.c#n1483

It basically:
a) creates the EMailBrowser window, which is a GtkWindow
b) sets some values to it
c) then shows the window

The b) also contains e_mail_reader_set_message(), which is above in the backtrace from valgrind. It's called when the window is still invisible.

Great, thanks for all that detail!

gtk_container_real_set_focus_child() does not check the return gboolean of gtk_widget_translate_coordinates(), which indicates whether the output x, y arguments were ever set.

I guess set_focus_child() should check this and not use the translated x, y if FALSE - rather, just force them to 0.

(In reply to Daniel Boles from comment #3)
> gtk_container_real_set_focus_child() does not check the return gboolean of
> gtk_widget_translate_coordinates(), which indicates whether the output x, y
> arguments were ever set.

and they will not be in various cases, including

  *  if (!ancestor || !_gtk_widget_get_realized (src_widget) ||
         !_gtk_widget_get_realized (dest_widget))
  * if any ancestor lacks a window

Anyway, the call and the allocation getters are:

          gtk_widget_translate_coordinates (focus_child, priv->focus_child,
                                            0, 0, &x, &y);

          _gtk_widget_get_allocation (priv->focus_child, &allocation);
          x += allocation.x;
          y += allocation.y;

          _gtk_widget_get_allocation (focus_child, &allocation);

but priv->focus child is set equal to focus_child above that... so doesn't this mean no translation is performed, and the same allocation is gotten twice? If the intention is really to pass the same widget twice, you'd think the code would write that. This seems fishy, but then I feel like somehow it must end up being correct, otherwise lots of stuff wouldn't work (as it is very old code)...


Setting that weirdness aside for the moment, the simplest fix is probably to change

  /* check for h/v adjustments
   */
  if (priv->focus_child)

to

  /* Check for h/v adjustments and scroll to the focus child, if realized */
  if (_gtk_widget_get_realized (container) && priv->focus_child)

but I can't test this atm

(In reply to Daniel Boles from comment #5)
> but priv->focus child is set equal to focus_child above that... so doesn't
> this mean [blah blah blah]

...remind me to not set up a confusing split screen that makes me miss the obvious detail: the while loop above this dives further into the hierarchy until it finds a non-container, so focus_child can and often will end up being different from priv->focus_child, and that's OK

Created attachment 357126 [details] [review]
Container: Don’t use coords of unrealized child

(master version, but gtk-3-22 is the same here)

This is a total no-brainer, so I'll just push it later.

Created attachment 357127 [details] [review]
Widget: Remove obsolete hack-arounds for HandleBox

bonus

Review of attachment 357126 [details] [review]:

::: gtk/gtkcontainer.c
@@ +2058,3 @@
             focus_child = gtk_widget_get_focus_child (focus_child);
 
+          if (!gtk_widget_translate_coordinates (focus_child, focus_child,

errr. This was already here, and changes the sense of what GTK+ 3 does. Should we not still be translating between the nominated focus child, and its innermost non-container child, rather than from the latter to itself?

pinging Timm about the above, since it arose in commit 885bcd9fe4b6b4ecb003570ea0520cf42ec737a9

Created attachment 357128 [details] [review]
Container: Fix real_set_focus_child()

untested sketch, but seems important

    Commit 885bcd9fe4b6b4ecb003570ea0520cf42ec737a9 trampled the bit here
    that is meant to translate between the nominated focus child and the
    actual innermost one that is needed for updating the h/v adjustments.

    So, we need to save the passed focus child before diving into its
    children, then translate and get allocations between them both. This
    makes GTK+ 4 behave like GTK+ 3 again: instead of priv->focus_child and
    focus_child, we now have passed_focus_child and focus_child, serving the
    roles of the nominated focus child and its innermost focus child resp.

(In reply to Milan Crha from comment #2)
> The code in question in Evolution looks like this:
> https://git.gnome.org/browse/evolution/tree/src/mail/e-mail-reader-utils.
> c#n1483
> 
> It basically:
> a) creates the EMailBrowser window, which is a GtkWindow
> b) sets some values to it
> c) then shows the window
> 
> The b) also contains e_mail_reader_set_message(), which is above in the
> backtrace from valgrind. It's called when the window is still invisible.

I guess this tends not to manifest as typically if you have code that grabs focus on a child, the window is already realized. In this case your window is trying to scroll to the message, but that's not possible if it's not realized. Of course, GTK+ should be fixed here, but maybe you should show your window beforehand too?

I've committed these (slightly tweaked at a couple of areas) and presume it will make Valgrind happy. Thanks for the report!

Comment on attachment 357127 [details] [review]
Widget: Remove obsolete hack-arounds for HandleBox

reverted; I was looking at the wrong tree; HandleBox is only deprecated in GTK+ 3, not removed

(In reply to Daniel Boles from comment #12)
> (In reply to Milan Crha from comment #2)
> > The b) also contains e_mail_reader_set_message(), which is above in the
> > backtrace from valgrind. It's called when the window is still invisible.
> 
> I guess this tends not to manifest as typically if you have code that grabs
> focus on a child, the window is already realized. In this case your window
> is trying to scroll to the message, but that's not possible if it's not
> realized. Of course, GTK+ should be fixed here, but maybe you should show
> your window beforehand too?

That's a generic code. In this particular case, the MessageList instance is not shown at all in the EMailBrowser, it's there hidden only to be able to scroll between messages.

Did your changes reach 3.22 too, or not? Since which version of gtk+ should be the fixes expected, please?

(In reply to Milan Crha from comment #15)
> Did your changes reach 3.22 too, or not? Since which version of gtk+ should
> be the fixes expected, please?

Yes, I picked it to gtk-3-22 as commit cf955a545969346b350d9f941bd092f0f3879581 which was made just before 3.22.18 was released. And 3.22.19 has just come out.