Nice job!
On Jun 15, 2013 9:44 AM, "Stephan Schreiber" <i...@fs-driver.org> wrote:
> tags 711107 - help
> tags 711107 + patch
> thanks
>
>
> Emilio Pozuelo Monfort<po...@debian.org> 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
>
>
