<p dir="ltr">Nice job!<br>
</p>
<div class="gmail_quote">On Jun 15, 2013 9:44 AM, "Stephan Schreiber" <<a href="mailto:info@fs-driver.org">info@fs-driver.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
tags 711107 - help<br>
tags 711107 + patch<br>
thanks<br>
<br>
<br>
Emilio Pozuelo Monfort<<a href="mailto:pochu@debian.org" target="_blank">pochu@debian.org</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
What happens if you do `make check' or `fakeroot make check' again? make check<br>
will set Xvfb differently than xvfb-run.<br>
</blockquote>
<br>
I didn't check that out. I assume the test will always fail with 'make check'.<br>
It doesn't matter what X server you are using, or how xvfb is set.<br>
<br>
The test fails when gtester is invoked:<br>
xvfb-run gtester -k --verbose children<br>
fails, but<br>
xvfb-run ./children<br>
passes the test.<br>
<br>
This is very interesting, but a misleading and useless information ;-).<br>
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:<br>
<br>
static void<br>
test_add_remove (GtkWidget *widget)<br>
{<br>
AtkObject *accessible;<br>
AtkObject *child_accessible;<br>
SignalData add_data;<br>
SignalData remove_data;<br>
SignalData parent_data[3];<br>
STATE state;<br>
gint i, j;<br>
gint step_children;<br>
<br>
state.widget = widget;<br>
accessible = gtk_widget_get_accessible (widget);<br>
<br>
add_data.count = 0;<br>
remove_data.count = 0;<br>
g_signal_connect (accessible, "children_changed::add",<br>
G_CALLBACK (children_changed), &add_data);<br>
g_signal_connect (accessible, "children_changed::remove",<br>
G_CALLBACK (children_changed), &remove_data);<br>
<br>
step_children = atk_object_get_n_accessible_<u></u>children (accessible);<br>
<br>
for (i = 0; i < 3; i++)<br>
{<br>
if (!do_create_child (&state, i))<br>
break;<br>
if (!GTK_IS_ENTRY (widget))<br>
{<br>
parent_data[i].count = 0;<br>
child_accessible = gtk_widget_get_accessible (state.child[i]);<br>
g_signal_connect (child_accessible, "notify::accessible-parent",<br>
G_CALLBACK (parent_notify), &(parent_data[i]));<br>
gtk_container_add (GTK_CONTAINER (widget), state.child[i]);<br>
}<br>
else<br>
child_accessible = atk_object_ref_accessible_<u></u>child (accessible, i);<br>
<br>
g_assert_cmpint (add_data.count, ==, i + 1);<br>
g_assert_cmpint (add_data.n_children, ==, step_children + i + 1);<br>
g_assert_cmpint (remove_data.count, ==, 0);<br>
if (!GTK_IS_ENTRY (widget))<br>
g_assert_cmpint (parent_data[i].count, ==, 1);<br>
if (GTK_IS_SCROLLED_WINDOW (widget) ||<br>
GTK_IS_NOTEBOOK (widget))<br>
g_assert (atk_object_get_parent (ATK_OBJECT (parent_data[i].parent)) == accessible);<br>
else if (GTK_IS_ENTRY (widget))<br>
g_assert (atk_object_get_parent (child_accessible) == accessible);<br>
else<br>
g_assert (parent_data[i].parent == accessible);<br>
<br>
if (GTK_IS_ENTRY (widget))<br>
g_object_unref (child_accessible);<br>
}<br>
for (j = 0 ; j < i; j++)<br>
{<br>
remove_child (&state, j);<br>
g_assert_cmpint (add_data.count, ==, i);<br>
g_assert_cmpint (remove_data.count, ==, j + 1);<br>
g_assert_cmpint (remove_data.n_children, ==, step_children + i - j - 1);<br>
if (parent_data[j].count == 2)<br>
g_assert (parent_data[j].parent == NULL);<br>
else if (!GTK_IS_ENTRY (widget))<br>
{<br>
AtkStateSet *set;<br>
set = atk_object_ref_state_set (ATK_OBJECT (parent_data[j].parent));<br>
g_assert (atk_state_set_contains_state (set, ATK_STATE_DEFUNCT));<br>
g_object_unref (set);<br>
}<br>
}<br>
<br>
g_signal_handlers_disconnect_<u></u>by_func (accessible, G_CALLBACK (children_changed), &add_data);<br>
g_signal_handlers_disconnect_<u></u>by_func (accessible, G_CALLBACK (children_changed), &remove_data);<br>
}<br>
<br>
<br>
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.<br>
Almost all object types are derived from GtkWidget or GtkContainer; GTK_IS_ENTRY() evaluates to false for them.<br>
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.<br>
<br>
<br>
Please take a look at the fifth line within the function:<br>
SignalData parent_data[3];<br>
<br>
Let's assume that we are running the buggy test; widget is a GtkEntry instance; GTK_IS_ENTRY (widget) is true.<br>
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:<br>
<br>
if (!GTK_IS_ENTRY (widget))<br>
{<br>
parent_data[i].count = 0;<br>
child_accessible = gtk_widget_get_accessible (state.child[i]);<br>
g_signal_connect (child_accessible, "notify::accessible-parent",<br>
G_CALLBACK (parent_notify), &(parent_data[i]));<br>
...<br>
}<br>
else<br>
child_accessible = atk_object_ref_accessible_<u></u>child (accessible, i);<br>
<br>
<br>
In the second for loop:<br>
<br>
if (parent_data[j].count == 2)<br>
g_assert (parent_data[j].parent == NULL);<br>
else if (!GTK_IS_ENTRY (widget))<br>
<br>
parent_data is read here without any preceding initialization. Thus, we are running into trouble.<br>
parent_data is not used at all on the GtkEntry case. It must not be in any assertion in that case.<br>
<br>
Second problem:<br>
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.<br>
It isn't executed on the considered last test with the GtkEntry instance since GTK_IS_ENTRY (widget) is true.<br>
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).<br>
<br>
if (parent_data[j].count == 2)<br>
g_assert (parent_data[j].parent == NULL);<br>
else if (!GTK_IS_ENTRY (widget))<br>
{<br>
AtkStateSet *set;<br>
set = atk_object_ref_state_set (ATK_OBJECT (parent_data[j].parent));<br>
g_assert (atk_state_set_contains_state (set, ATK_STATE_DEFUNCT));<br>
g_object_unref (set);<br>
}<br>
<br>
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):<br>
<br>
parent_data[i].count = 0;<br>
g_signal_connect (child_accessible, "notify::accessible-parent",<br>
G_CALLBACK (parent_notify), &(parent_data[i]));<br>
<br>
The last code line causes two subsequent calls of children_changed() - first time when the container is added; second time when it is removed:<br>
<br>
static void<br>
children_changed (AtkObject *accessible,<br>
guint index,<br>
gpointer child,<br>
SignalData *data)<br>
{<br>
data->count++;<br>
data->index = index;<br>
data->n_children = atk_object_get_n_accessible_<u></u>children (accessible);<br>
}<br>
<br>
Since the data parameter points to the appropriate parent_data element, its count member is always 2 after that.<br>
To prove that this is correct, we add an assertion that checks whether that count is always 2.<br>
After deleting the dead code, the code reads:<br>
<br>
static void<br>
test_add_remove (GtkWidget *widget)<br>
{<br>
AtkObject *accessible;<br>
AtkObject *child_accessible;<br>
SignalData add_data;<br>
SignalData remove_data;<br>
SignalData parent_data[3];<br>
STATE state;<br>
gint i, j;<br>
gint step_children;<br>
<br>
state.widget = widget;<br>
accessible = gtk_widget_get_accessible (widget);<br>
<br>
add_data.count = 0;<br>
remove_data.count = 0;<br>
g_signal_connect (accessible, "children_changed::add",<br>
G_CALLBACK (children_changed), &add_data);<br>
g_signal_connect (accessible, "children_changed::remove",<br>
G_CALLBACK (children_changed), &remove_data);<br>
<br>
step_children = atk_object_get_n_accessible_<u></u>children (accessible);<br>
<br>
for (i = 0; i < 3; i++)<br>
{<br>
if (!do_create_child (&state, i))<br>
break;<br>
if (!GTK_IS_ENTRY (widget))<br>
{<br>
parent_data[i].count = 0;<br>
child_accessible = gtk_widget_get_accessible (state.child[i]);<br>
g_signal_connect (child_accessible, "notify::accessible-parent",<br>
G_CALLBACK (parent_notify), &(parent_data[i]));<br>
gtk_container_add (GTK_CONTAINER (widget), state.child[i]);<br>
}<br>
else<br>
child_accessible = atk_object_ref_accessible_<u></u>child (accessible, i);<br>
<br>
g_assert_cmpint (add_data.count, ==, i + 1);<br>
g_assert_cmpint (add_data.n_children, ==, step_children + i + 1);<br>
g_assert_cmpint (remove_data.count, ==, 0);<br>
if (!GTK_IS_ENTRY (widget))<br>
g_assert_cmpint (parent_data[i].count, ==, 1);<br>
if (GTK_IS_SCROLLED_WINDOW (widget) ||<br>
GTK_IS_NOTEBOOK (widget))<br>
g_assert (atk_object_get_parent (ATK_OBJECT (parent_data[i].parent)) == accessible);<br>
else if (GTK_IS_ENTRY (widget))<br>
g_assert (atk_object_get_parent (child_accessible) == accessible);<br>
else<br>
g_assert (parent_data[i].parent == accessible);<br>
<br>
if (GTK_IS_ENTRY (widget))<br>
g_object_unref (child_accessible);<br>
}<br>
for (j = 0 ; j < i; j++)<br>
{<br>
remove_child (&state, j);<br>
g_assert_cmpint (add_data.count, ==, i);<br>
g_assert_cmpint (remove_data.count, ==, j + 1);<br>
g_assert_cmpint (remove_data.n_children, ==, step_children + i - j - 1);<br>
if (!GTK_IS_ENTRY (widget))<br>
{<br>
g_assert_cmpint (parent_data[j].count, ==, 2);<br>
g_assert (parent_data[j].parent == NULL);<br>
}<br>
}<br>
<br>
g_signal_handlers_disconnect_<u></u>by_func (accessible, G_CALLBACK (children_changed), &add_data);<br>
g_signal_handlers_disconnect_<u></u>by_func (accessible, G_CALLBACK (children_changed), &remove_data);<br>
}<br>
<br>
<br>
This is the commit which introduced the bugs.<br>
<a href="https://git.gnome.org/browse/gtk+/commit/tests/a11y/children.c?h=gtk-3-8&id=b7743430aa1471c9ec054606d2234700e2da3eda" target="_blank">https://git.gnome.org/browse/<u></u>gtk+/commit/tests/a11y/<u></u>children.c?h=gtk-3-8&id=<u></u>b7743430aa1471c9ec054606d22347<u></u>00e2da3eda</a><br>
<br>
Although it was seen on ia64 only, all archs are affected. The other archs just had more luck.<br>
<br>
<br>
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?<br>
<br>
Stephan<br>
<br>
</blockquote></div>