Re: Bug#711107: gtk+3.0: FTBFS on ia64 and mips

Nice job!
 On Jun 15, 2013 9:44 AM, "Stephan Schreiber" <[email protected]> wrote:

> tags 711107 - help
> tags 711107 + patch
> thanks
>
>
> Emilio Pozuelo Monfort<[email protected]> wrote:
>
>  What happens if you do `make check' or `fakeroot make check' again? make
>> check
>> will set Xvfb differently than xvfb-run.
>>
>
> I didn't check that out. I assume the test will always fail with 'make
> check'.
> It doesn't matter what X server you are using, or how xvfb is set.
>
> The test fails when gtester is invoked:
> xvfb-run gtester -k --verbose children
> fails, but
> xvfb-run ./children
> passes the test.
>
> This is very interesting, but a misleading and useless information ;-).
> The real problem is in the tests/a11y/children.c source code file within
> the test_add_remove() function; it reads local variables without
> initialization:
>
> static void
> test_add_remove (GtkWidget *widget)
> {
>   AtkObject *accessible;
>   AtkObject *child_accessible;
>   SignalData add_data;
>   SignalData remove_data;
>   SignalData parent_data[3];
>   STATE state;
>   gint i, j;
>   gint step_children;
>
>   state.widget = widget;
>   accessible = gtk_widget_get_accessible (widget);
>
>   add_data.count = 0;
>   remove_data.count = 0;
>   g_signal_connect (accessible, "children_changed::add",
>                     G_CALLBACK (children_changed), &add_data);
>   g_signal_connect (accessible, "children_changed::remove",
>                     G_CALLBACK (children_changed), &remove_data);
>
>   step_children = atk_object_get_n_accessible_**children (accessible);
>
>   for (i = 0; i < 3; i++)
>     {
>       if (!do_create_child (&state, i))
>         break;
>       if (!GTK_IS_ENTRY (widget))
>         {
>           parent_data[i].count = 0;
>           child_accessible = gtk_widget_get_accessible (state.child[i]);
>           g_signal_connect (child_accessible, "notify::accessible-parent",
>                             G_CALLBACK (parent_notify), &(parent_data[i]));
>           gtk_container_add (GTK_CONTAINER (widget), state.child[i]);
>         }
>       else
>         child_accessible = atk_object_ref_accessible_**child (accessible,
> i);
>
>       g_assert_cmpint (add_data.count, ==, i + 1);
>       g_assert_cmpint (add_data.n_children, ==, step_children + i + 1);
>       g_assert_cmpint (remove_data.count, ==, 0);
>       if (!GTK_IS_ENTRY (widget))
>         g_assert_cmpint (parent_data[i].count, ==, 1);
>       if (GTK_IS_SCROLLED_WINDOW (widget) ||
>           GTK_IS_NOTEBOOK (widget))
>         g_assert (atk_object_get_parent (ATK_OBJECT
> (parent_data[i].parent)) == accessible);
>       else if (GTK_IS_ENTRY (widget))
>         g_assert (atk_object_get_parent (child_accessible) == accessible);
>       else
>         g_assert (parent_data[i].parent == accessible);
>
>       if (GTK_IS_ENTRY (widget))
>         g_object_unref (child_accessible);
>     }
>   for (j = 0 ; j < i; j++)
>     {
>       remove_child (&state, j);
>       g_assert_cmpint (add_data.count, ==, i);
>       g_assert_cmpint (remove_data.count, ==, j + 1);
>       g_assert_cmpint (remove_data.n_children, ==, step_children + i - j -
> 1);
>       if (parent_data[j].count == 2)
>         g_assert (parent_data[j].parent == NULL);
>       else if (!GTK_IS_ENTRY (widget))
>         {
>           AtkStateSet *set;
>           set = atk_object_ref_state_set (ATK_OBJECT
> (parent_data[j].parent));
>           g_assert (atk_state_set_contains_state (set, ATK_STATE_DEFUNCT));
>           g_object_unref (set);
>         }
>     }
>
>   g_signal_handlers_disconnect_**by_func (accessible, G_CALLBACK
> (children_changed), &add_data);
>   g_signal_handlers_disconnect_**by_func (accessible, G_CALLBACK
> (children_changed), &remove_data);
> }
>
>
> When you take a look at the main() function in children.c, you realize
> that test_add_remove() is called multiple times; it gets via its widget
> paramter object instances of different types.
> Almost all object types are derived from GtkWidget or GtkContainer;
> GTK_IS_ENTRY() evaluates to false for them.
> The last type is GtkEntry (created by gtk_entry_new()); GTK_IS_ENTRY()
> evaluates to true for it. This is the test we have the trouble with.
>
>
> Please take a look at the fifth line within the function:
>   SignalData parent_data[3];
>
> Let's assume that we are running the buggy test; widget is a GtkEntry
> instance; GTK_IS_ENTRY (widget) is true.
> The first time you see some code for parent_data[].count is in the first
> for loop. But if GTK_IS_ENTRY (widget) is true, parent_data isnt'touched at
> all. No pointer to parent_data is provided to somewhere else:
>
>       if (!GTK_IS_ENTRY (widget))
>         {
>           parent_data[i].count = 0;
>           child_accessible = gtk_widget_get_accessible (state.child[i]);
>           g_signal_connect (child_accessible, "notify::accessible-parent",
>                             G_CALLBACK (parent_notify), &(parent_data[i]));
>           ...
>         }
>       else
>         child_accessible = atk_object_ref_accessible_**child (accessible,
> i);
>
>
> In the second for loop:
>
>       if (parent_data[j].count == 2)
>         g_assert (parent_data[j].parent == NULL);
>       else if (!GTK_IS_ENTRY (widget))
>
> parent_data is read here without any preceding initialization. Thus, we
> are running into trouble.
> parent_data is not used at all on the GtkEntry case. It must not be in any
> assertion in that case.
>
> Second problem:
> If we take a closer look at the entire if-else statement, we realize that
> the block after the else-if statement is never executed. Dead code.
> It isn't executed on the considered last test with the GtkEntry instance
> since GTK_IS_ENTRY (widget) is true.
> It isn't executed on the other tests with a GtkWidget or GtkContainer
> instance, because on that tests the term parent_data[j].count is *always* 2
> (see below).
>
>       if (parent_data[j].count == 2)
>         g_assert (parent_data[j].parent == NULL);
>       else if (!GTK_IS_ENTRY (widget))
>         {
>           AtkStateSet *set;
>           set = atk_object_ref_state_set (ATK_OBJECT
> (parent_data[j].parent));
>           g_assert (atk_state_set_contains_state (set, ATK_STATE_DEFUNCT));
>           g_object_unref (set);
>         }
>
> The following code lines are executed for each used element of
> parent_data[] on the tests with GtkWidget/GtkContainer (GTK_IS_ENTRY
> (widget) is false):
>
>           parent_data[i].count = 0;
>           g_signal_connect (child_accessible, "notify::accessible-parent",
>                             G_CALLBACK (parent_notify), &(parent_data[i]));
>
> The last code line causes two subsequent calls of children_changed() -
> first time when the container is added; second time when it is removed:
>
> static void
> children_changed (AtkObject  *accessible,
>                   guint       index,
>                   gpointer    child,
>                   SignalData *data)
> {
>   data->count++;
>   data->index = index;
>   data->n_children = atk_object_get_n_accessible_**children (accessible);
> }
>
> Since the data parameter points to the appropriate parent_data element,
> its count member is always 2 after that.
> To prove that this is correct, we add an assertion that checks whether
> that count is always 2.
> After deleting the dead code, the code reads:
>
> static void
> test_add_remove (GtkWidget *widget)
> {
>   AtkObject *accessible;
>   AtkObject *child_accessible;
>   SignalData add_data;
>   SignalData remove_data;
>   SignalData parent_data[3];
>   STATE state;
>   gint i, j;
>   gint step_children;
>
>   state.widget = widget;
>   accessible = gtk_widget_get_accessible (widget);
>
>   add_data.count = 0;
>   remove_data.count = 0;
>   g_signal_connect (accessible, "children_changed::add",
>                     G_CALLBACK (children_changed), &add_data);
>   g_signal_connect (accessible, "children_changed::remove",
>                     G_CALLBACK (children_changed), &remove_data);
>
>   step_children = atk_object_get_n_accessible_**children (accessible);
>
>   for (i = 0; i < 3; i++)
>     {
>       if (!do_create_child (&state, i))
>         break;
>       if (!GTK_IS_ENTRY (widget))
>         {
>           parent_data[i].count = 0;
>           child_accessible = gtk_widget_get_accessible (state.child[i]);
>           g_signal_connect (child_accessible, "notify::accessible-parent",
>                             G_CALLBACK (parent_notify), &(parent_data[i]));
>           gtk_container_add (GTK_CONTAINER (widget), state.child[i]);
>         }
>       else
>         child_accessible = atk_object_ref_accessible_**child (accessible,
> i);
>
>       g_assert_cmpint (add_data.count, ==, i + 1);
>       g_assert_cmpint (add_data.n_children, ==, step_children + i + 1);
>       g_assert_cmpint (remove_data.count, ==, 0);
>       if (!GTK_IS_ENTRY (widget))
>         g_assert_cmpint (parent_data[i].count, ==, 1);
>       if (GTK_IS_SCROLLED_WINDOW (widget) ||
>           GTK_IS_NOTEBOOK (widget))
>         g_assert (atk_object_get_parent (ATK_OBJECT
> (parent_data[i].parent)) == accessible);
>       else if (GTK_IS_ENTRY (widget))
>         g_assert (atk_object_get_parent (child_accessible) == accessible);
>       else
>         g_assert (parent_data[i].parent == accessible);
>
>       if (GTK_IS_ENTRY (widget))
>         g_object_unref (child_accessible);
>     }
>   for (j = 0 ; j < i; j++)
>     {
>       remove_child (&state, j);
>       g_assert_cmpint (add_data.count, ==, i);
>       g_assert_cmpint (remove_data.count, ==, j + 1);
>       g_assert_cmpint (remove_data.n_children, ==, step_children + i - j -
> 1);
>       if (!GTK_IS_ENTRY (widget))
>         {
>           g_assert_cmpint (parent_data[j].count, ==, 2);
>           g_assert (parent_data[j].parent == NULL);
>         }
>     }
>
>   g_signal_handlers_disconnect_**by_func (accessible, G_CALLBACK
> (children_changed), &add_data);
>   g_signal_handlers_disconnect_**by_func (accessible, G_CALLBACK
> (children_changed), &remove_data);
> }
>
>
> This is the commit which introduced the bugs.
> https://git.gnome.org/browse/**gtk+/commit/tests/a11y/**
> children.c?h=gtk-3-8&id=**b7743430aa1471c9ec054606d22347**00e2da3eda<https://git.gnome.org/browse/gtk+/commit/tests/a11y/children.c?h=gtk-3-8&id=b7743430aa1471c9ec054606d2234700e2da3eda>
>
> Although it was seen on ia64 only, all archs are affected. The other archs
> just had more luck.
>
>
> The patch corrects the problem which we have on ia64. The problem on mips
> is likely a completely different one. Please could you file a new separate
> bug report for that?
>
> Stephan
>
>