Bug 80495 – [GTK] race condition in run-gtk-tests

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

Between at-spi-bus-launcher being slow at setting up the a11y bus and testatk trying to poke it too fast I think.

One way to fix this is to make run-gtk-tests wait the bus have been started before launching the tests.

[Philippe Normand]

Also, when testatk fails to connect, it blocks the whole thing for 20 minutes...

[Martin Robinson]

Nice catch. We definitely need to fix this, for the sake of the bots!

[Philippe Normand]

I started a patch, making the script run a glib main loop and connecting to the bus's NameOwnerChanged to wait on the a11y bus. I'll rewrite it using gdbus instead of python-dbus.

[Philippe Normand]

Created attachment 130558 [details]
Patch

[Philippe Normand]

Created attachment 130569 [details]
Patch

[Martin Robinson]

Comment on attachment 130569 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130569&action=review

Nice work, though I think this could use a little cleanup.

> Tools/Scripts/run-gtk-tests:22
> +import os, sys, time

Each import should have its own line.

> Tools/Scripts/run-gtk-tests:24
> +
> +from gi.repository import Gio, GLib

No newline necessary here.

> Tools/Scripts/run-gtk-tests:88
> +    def _wait_dbus_service_and_run(self, service_name, handler):

_run_command_when_dbus_service_appears?

How are timeouts handled here?

> Tools/Scripts/run-gtk-tests:113
> +        global timed_out;

Instead of a global variable, why not an instance variable?

> Tools/Scripts/run-gtk-tests:131
> +        def check_bailout():

You should probably rename this to something like: "check_if_tests_have_timed_out"

> Tools/Scripts/run-gtk-tests:138
> +            r = False
> +            if now - start > TIMEOUT:
> +                sys.stdout.write("Tests timed out after %d seconds\n" % TIMEOUT)
> +                sys.stdout.flush()
> +                r = True
> +            return r

You can just use an early return here and avoid the variable:

if now.time() - start <= TIMEOUT:
    return True
sys.stdout.write("Tests timed out after %d seconds\n" % TIMEOUT)
sys.stdout.flush()
return False

> Tools/Scripts/run-gtk-tests:152
> +            # Make sure the accessibility registry daemon is running.
> +            a11y_registryd_running = False
> +            a11y_registryd_path = self._lookup_atspi2_binary(jhbuild_path, 'at-spi2-registryd')
> +            if a11y_registryd_path:
> +                try:
> +                    a11y_registryd = Executive().popen([a11y_registryd_path], env=test_env)
> +                    a11y_registryd_running = True
> +                except:
> +                    sys.stderr.write("Failed to run the accessibility registry\n")
> +                    sys.stderr.flush()
> +

This could probably be a helper. You could avoid the comment that way:

def _ensure_accessibility_daemon_is_running(self):

> Tools/Scripts/run-gtk-tests:161
> +                if check_bailout():
> +                    timed_out = True
> +                    break

This doesn't seem to work in situations where the test never halts, right?

[Philippe Normand]

(In reply to comment #6)
> 
> > Tools/Scripts/run-gtk-tests:88
> > +    def _wait_dbus_service_and_run(self, service_name, handler):
> 
> _run_command_when_dbus_service_appears?
> 
> How are timeouts handled here?
> 

Hum I don't see what could time out here. Can you elaborate please?

> 
> > Tools/Scripts/run-gtk-tests:161
> > +                if check_bailout():
> > +                    timed_out = True
> > +                    break
> 
> This doesn't seem to work in situations where the test never halts, right?

True :( I wish gtester had some way to handle tests timing out.

[Philippe Normand]

Created attachment 130627 [details]
Patch

[Martin Robinson]

Comment on attachment 130627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130627&action=review

Looks good, but consider converting the a11y stuff to use with. Perhaps this could be a follwoup patch.

> Tools/Scripts/run-gtk-tests:149
> +        def run_for_real():

Maybe run_for_real could be called run_tests. :)

> Tools/Scripts/run-gtk-tests:164
> +            if self._a11y_registryd:
> +                self._a11y_registryd.terminate()
> +
> +            a11y_bus_launcher.terminate()

It seems we want to be really sure that the a11y stuff is cleaned up properly when exiting the loop, so I think it makes sense to use "with" here. http://docs.python.org/release/2.5/whatsnew/pep-343.html Without using this, we run the risk of an exception preventing the termination of the a11y stuff. I think I've seen this happen to me.

[Philippe Normand]

Committed r110059: <http://trac.webkit.org/changeset/110059>

[Philippe Normand]

(In reply to comment #9)
> (From update of attachment 130627 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130627&action=review
> 
> Looks good, but consider converting the a11y stuff to use with. Perhaps this could be a follwoup patch.
> 

I'll try this in a follow-up patch, thanks!