Nice job!
tags 711107 - help
tags 711107 + patch
thanks
Emilio Pozuelo Monfort<pochu@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=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