Created attachment 68933 [details] [review]
Patch so gnome-session starts at-spi-registryd

Here are the latest changes to work for enabling gnome-session starting at-spi-registryd. This one includes some comments by Bill about not needing to start the component since at-spi-registryd already does that.

Ariel, I think there may be a problem with race conditions, since you are using gsm_assistive_tech_exec() to launch the registry, since gsm_assistive_tech_exec uses g_spawn_command_line_async().  Don't we need to block until the IOR has been set to the X atom, before continuing to load the session?  (I'm not sure how to detect when the X atom has been set).

Micheal?

I'm not sure on this one. At least here I have not caught the problem and my machine does it right but it might appear on a slower computer. I don't see much of a problem because the gtk modules (which are the ones requiring gail and the bridge) are load way after the registry is launched.

We can something like loop and check if the X property has been set and then continue or maybe even put back again the bonobo code to ensure not only that we have a IOR set but also that it can be used to load the object.

What do you mean "gtk modules... are loaded way after the registry is launched..." ?

Any apps that get launched and might potentially load before the at-spi-registryd has initialized will load the atk-bridge, which will try to read the IOR.  So this certainly looks like an important race condition.

If we "put back the bonobo code" then we should remove the g_spawn code since that would just be another race, otherwise...

I think it makes more sense to block until the IOR has been set.  We should not be setting the IOR atom until a valid IOR is available, and I think once the IOR is available and string-ified, any client will be able to successfully use it, so I don't see a distinction between "having an IOR set" and "it can be used to load the object."

Bill

Ok I understand let me work on this.

I think we need to XSelectInput with PropertyNotify mask on the root window, then maybe install a gdk_window_add_filter() on it (if we don't already have a filter for the root window in gnome-session...), then we can detect when the atom is set...
We should wait until it's set before launching any assistive technologies, and before launching the session apps.  If we think there's a likelihood that the IOR setting might sometimes fail (perhaps due to a bug in at-spi-registryd?) then perhaps we should set some kind of g_timer as a failsafe, so make sure we don't block forever.

Created attachment 69139 [details] [review]
New patch with some of Bills latest comments

I made this patch. After launching the registry I am on a loop until I am sure I have the IORpresent. Currently, I keep checking directly if the ior was set. For some reason I never enter the filter funciont set by gdk_window_add_filter. My guess is that I need to check for the atom after gnome_program_init. I think it's only a matter of playing a little more, but just wanted to make sure this is going in the right direction.

I have tryed to make the gdk_window_add_filter to work but I have found that I only get the X events after gnome_program_init has been started. Unfortunately by that time is too late and gnome-session dies. I think what I posted on the previous patch is good enough and covers the race condition.

You probably want to do gdk_window_set_events() on the root window or something to make the propery change events to work. There's no reason it shouldn't be possible to make it work

You want to run the main loop from gsm_assistive_registry_start() and quit it again from filter_watch_ior() ... then once the mainloop has been quit you don't to check the property again. Don't sleep and poll

Extra points for setting up a long timeout and displaying an error (offering the choice between continuing and logging out) if the registry fails to set the IOR ... 

Ok. I replaced the gdk filter functions with their X equivalent. And everything works fine now:

     gsm_watch_ior (); ---> Do XSelectInput
     gsm_assistive_tech_exec ("/usr/libexec/at-spi-registryd"); 
     gsm_filter_property (); --> Wait until property notification 

Created attachment 69588 [details] [review]
We now wait for XEvent to know when registry has been set.

Is Ariel's 7/25/06 patch acceptable?  If not, then what needs to be done?

This bug is blocking 163132 and 345434, and the code cut off for Gnome 2.16 is approaching.   Will the maintainer accept the proposed patch in time?  :-)  Thanks.

Still not happy with the approach:

  - We block until we receive any PropertyChange on the root window and
    then poll every 1ms until the property is set.

    That's clearly bogus. If we have the ability to block until the property
    is set, we should do that in a reliable manner and get rid of the polling.

    Something like this should work:

    static Bool
    gsm_is_ior_property_notify (Display *display,
                                XEvent  *xevent,
                                XPointer dummy)
    {
      return xevent->type   == PropertyNotify &&
             xevent->window == root_window &&
             xevent->atom   == ior_atom;
    }
                                
    while (!gsm_ior_is_present ())
      XIfEvent (gsm_is_ior_property_notify ());

  - We block forever. If something breaks (and at some point, for someone it 
    will), then login just hangs. We need to time out, go ahead and login
    and display a useful error message why a11y is broken.

    However, given that we're approaching code freeze, if someone opened a bug 
    to tracked that TODO item, I'd be happy for the patch to go in. Bonus
    points if that person also agreed to implement it.

And I've many minor nits:

  - Random coding style:

      + Function names should start a new line
      + The opening brace of a function should be on a new line
      + Most of the time, each function argument should be on a new line
      + Separate variable declaration from the initial assignment
      + Don't use braces where they're not needed

    i.e. look around at the surrounding coding style and imitate it
 
  - Don't XInternAtom() each time gsm_ior_is_present() is called. Cache it
    in a module static variable before calling the function for the first time

  - No need for the forward declaration of ior_is_present()

  - No need to pass GDK_ROOT_WINDOW to XIfEvent if you're not using it

I'm unlikely to have to time to review the next iteration. So, please make
a genuine best effort to fix up all of the above, post the patch here, and go
ahead and commit and close the bug.

I think this patch should wait until 2.17 so that it can be tested and reviewed more thoroughly.  I think that there's too much risk of accidentally breaking a11y in 2.16 with this patch otherwise.  Too bad, but I think it's wiser to wait at this stage. 

For what it's worth, I agree with Mark's comments completely regarding the current patch.  It's a good start but needs work.

I agree with your recommendation to wait until 2.17.  We'll work on the mentioned work items.  Thanks!

Created attachment 72336 [details] [review]
Latest work with Michael's comment

This patch includes the comments made by Michale after looking my previous work.
Still needs to work on the timeout issue but this should be good to go.

Ariel:

This still looks like a tight loop:
 
+     while (!gsm_ior_is_present ())
+	  gsm_filter_property ();
+

I think gnome-session uses the gdk main loop; if so, shouldn't we just be using gdk_window_add_filter (in conjunction with our XSelectInput), to attach a listener for these events (instead of polling for them?)

Comment on attachment 72336 [details] [review]
Latest work with Michael's comment

>? ariel.patch
>? gdm-session-atspi-5.patch
>? gnome-session-atspi-3.patch
>? gnome-session-atspi-4.patch
>? gnome-session-atspi.patch
>? gnome-session.patch
>Index: gsm-at-startup.c
>===================================================================
>RCS file: /cvs/gnome/gnome-session/gnome-session/gsm-at-startup.c,v
>retrieving revision 1.5
>diff -u -r1.5 gsm-at-startup.c
>--- gsm-at-startup.c	24 Apr 2006 21:55:25 -0000	1.5
>+++ gsm-at-startup.c	6 Sep 2006 19:26:23 -0000
>@@ -4,12 +4,17 @@
> #include "gsm-at-startup.h"
> #include "util.h"
> 
>+#include <gtk/gtk.h>
>+#include <gnome.h>
> #include <gdk/gdk.h>
>+#include <gdk/gdkx.h>
> #include <libgnome/libgnome.h>
> #include <gconf/gconf-client.h>
> 
> #define AT_STARTUP_KEY    "/desktop/gnome/accessibility/startup/exec_ats"
> 
>+static Atom AT_SPI_IOR;
>+
> static void
> gsm_assistive_tech_exec (gchar *exec_string)
> {
>@@ -19,6 +24,75 @@
>     g_spawn_command_line_async (exec_string, &error);
>     g_free (s);
>   }
>+}
>+
>+
>+static gboolean 
>+gsm_ior_is_present (void)
>+{
>+     Atom actual_type;
>+     int actual_format;
>+     unsigned char *ior;
>+     unsigned long nitems;
>+     unsigned long leftover;
>+
>+     if (!AT_SPI_IOR)
>+	  AT_SPI_IOR = XInternAtom (GDK_DISPLAY (), "AT_SPI_IOR", False); 
>+     
>+     XGetWindowProperty (GDK_DISPLAY (), GDK_ROOT_WINDOW (), AT_SPI_IOR, 0L, 
>+			 (long) BUFSIZ, False, 
>+			 (Atom) 31, &actual_type, &actual_format,
>+			 &nitems, &leftover, &ior); 
>+     if (ior != NULL)
>+	  return TRUE;
>+     
>+     return FALSE;
>+}
>+
>+
>+static Bool 
>+gsm_wait_notify (Display *d, 
>+		 XEvent *e, 
>+		 char *arg) 
>+{
>+
>+     if (e->type == PropertyNotify)
>+     {
>+	  XPropertyEvent *property = &e->xproperty;
>+
>+	  return property->atom == AT_SPI_IOR; 
>+     }
>+     
>+     return  FALSE;
>+}
>+
>+static void
>+gsm_watch_ior (void)
>+{
>+	Display *d = GDK_DISPLAY ();
>+	Window w = DefaultRootWindow (d);
>+
>+	XSelectInput (d, w, PropertyChangeMask);
>+	
>+}
>+
>+static void 
>+gsm_filter_property (void)
>+{
>+     XEvent event;
>+     XIfEvent (GDK_DISPLAY(), &event, gsm_wait_notify, NULL);	
>+}
>+
>+void 
>+gsm_assistive_registry_start (void)
>+{
>+     gsm_watch_ior ();
>+     gsm_assistive_tech_exec ("/usr/libexec/at-spi-registryd");
>+   
>+     if (!gsm_ior_is_present ())
>+	  gsm_filter_property ();
>+
>+     return;
> }
> 
> void 
>Index: gsm-at-startup.h
>===================================================================
>RCS file: /cvs/gnome/gnome-session/gnome-session/gsm-at-startup.h,v
>retrieving revision 1.2
>diff -u -r1.2 gsm-at-startup.h
>--- gsm-at-startup.h	10 Jan 2006 00:48:20 -0000	1.2
>+++ gsm-at-startup.h	6 Sep 2006 19:26:23 -0000
>@@ -1,6 +1,7 @@
> #ifndef GSM_AT_STARTUP_H
> #define GSM_AT_STARTUP_H
> 
>+void gsm_assistive_registry_start (void);
> void gsm_assistive_technologies_start (void);
> void gsm_at_set_gtk_modules (void);
> 
>Index: main.c
>===================================================================
>RCS file: /cvs/gnome/gnome-session/gnome-session/main.c,v
>retrieving revision 1.78
>diff -u -r1.78 main.c
>--- main.c	24 Apr 2006 21:50:44 -0000	1.78
>+++ main.c	6 Sep 2006 19:26:23 -0000
>@@ -395,9 +395,11 @@
>   gconf_client_add_dir (gconf_client, GSM_GCONF_CONFIG_PREFIX, GCONF_CLIENT_PRELOAD_ONELEVEL, NULL); 
>   a_t_support = gconf_client_get_bool (gconf_client, ACCESSIBILITY_KEY, NULL);
> 
>-  if (a_t_support)
>-    gsm_at_set_gtk_modules ();
>-
>+  if (a_t_support){
>+       gsm_assistive_registry_start ();
>+       gsm_at_set_gtk_modules ();
>+  }
>+    
>   gnome_program_init("gnome-session", VERSION, LIBGNOMEUI_MODULE, argc, argv, 
> 		  GNOME_PARAM_POPT_TABLE, options,
> 		  NULL);

Bill,

There's actually no need for the while. I have edited the patch to be something like: 
>+gsm_filter_property (void)
>+{
>+     XEvent event;
>+     XIfEvent (GDK_DISPLAY(), &event, gsm_wait_notify, NULL);	
>+}
>+
>+void 
>+gsm_assistive_registry_start (void)
>+{
>+     gsm_watch_ior ();
>+     gsm_assistive_tech_exec ("/usr/libexec/at-spi-registryd");
>+   
>+     if (!gsm_ior_is_present ())
>+	  gsm_filter_property ();
>+
>+     return;
> }

XIfEvent will block until we receive the PropertyChange event and the property been changed is the AT_SPI_IOR atom. I think leaving gsm_ior_is_present before blocking is a good idea to be able to continue if for some reason the atom has been set before we have set the XIfEvent.

ariel

Ariel:

XIfEvent will only block until the next X event is received.  If another X event is received before the notification your patch will fail.

What is desired is a non-polling event filter for X events received by gnome-session.  Is there some reason why my suggestion in comment #18 doesn't make sense to you?

So from what I get from commetn #18 I now have:
static GdkFilterReturn
filter_watch (GdkXEvent *xevent, GdkEvent *event, gpointer data){
     XEvent *xev = (XEvent *)xevent;

     if (xev->xany.type == PropertyNotify &&
         xev->xproperty.atom == AT_SPI_IOR){
          gtk_main_quit ();

          return GDK_FILTER_REMOVE;
     }

     return GDK_FILTER_CONTINUE;
}

void
gsm_assistive_registry_start (void)
{
     GdkWindow *w = gdk_get_default_root_window ();

     gdk_window_set_events (w,  GDK_PROPERTY_CHANGE_MASK);
     gsm_assistive_tech_exec ("/usr/libexec/at-spi-registryd");
     gsm_ior_is_present ();
     gdk_window_add_filter (w, filter_watch, NULL);

     gtk_main ();
}

We enter another gtk_main and exit until filter_watch finds the AT_SPI_IOR. From this it should be easier to add the timeout and it's much cleaner.

Created attachment 72785 [details] [review]
Add timeout and nice error dialog.

Bill,
To the previous work I added a timeout func. If after two seconds we have not found the registry an error dialog appears saying something lke "no registry found we are going to continue w/o a11y" and continues to start the session.

Hi Ariel:

The patch looks nice and clean; there are a couple of minor typos in the error dialog message, I'd suggest this:

"Assistive technology support has been requested for this session, but the accessibility registry was not found.  Please ensure that the AT-SPI package is installed.  Your session has been started without assistive technology support."

Also: the message should be marked for translation with the _() macro, and it should not include newlines ('\n').  

I'd like to get feedback from some other people on whether it's reasonable for gnome-session to run gtk_main twice, as it now does.  I would have some concerns that this might have odd side-effects; because of this I think the above patch needs to be tested in several different configurations before approval.

Thanks for working on this!

Created attachment 73434 [details] [review]
Include use of _() macro

Include use of _() macro and use of msg suggested per Bill's latest comment.

Using gtk_main() twice as done here shouldn't be a problem.

However, there are still some issues:

 + in case of error, the session loading will block on gtk_dialog_run(). I don't think we should block on it, but we should display the dialog and continue loading the session. This could be tricky wrt to gtk_main(), though, since we might have entered the second gtk_main(). The doc says: "You can nest calls to gtk_main(). In that case gtk_main_quit() will make the innermost invocation of the main loop return."

 + is it really a good thing to set the accessibility key to false if there's an error? It means the user won't have accessibility on next login, and maybe the issue with registryd would have been resolved then...

 + I'd love to see the functions use a good namespace, like, say, gsm_at_ or gsm_assistive, or gsm_assistive_registry...

 + minor nitpick: there's a missing space in "if (a_t_support){" (it should be "if (a_t_support) {"). It happens at least twice in the patch.

 + you probably need to remove the filter in filter_watch. It'd be bad to enter this a second time and calling gtk_main_quit() again ;-)

 + gsm_assistive_tech_exec ("/usr/libexec/at-spi-registryd");
   The path is hardcoded here. This is bad for installation outside of /usr

Created attachment 74433 [details] [review]
Include some of latest Vincent comments

This patch includes the following:
-name space fucntions gsm_assisitive_bla...
-remove filter in filter_watch
-remove hardcoded path

Created attachment 74594 [details] [review]
Recover the previous value of ayy1 key if registryd was not found

I have made one more change to the patch. Now, if the registry is not found you continue to load the session and set the a11y to false but a new "recover" keyis created. So, next time user logs in if this recover key is set to true, then we try to start a11y support.

This looks good to me except perhaps for the 'accessibility_recover' gconf key.  Wouldn't it be better to just make atk-bridge fail gracefully (i.e. not segfault) if the registry is not found?  The additional gconf key means that if someone's at-spi is broken, and they then turn off accessibility, it's likely to restart anyway (because the 'recover' key will have been set by gnome-session)...

I suggest just removing the recover key stuff, not turning off the gconf key if accessibility loading fails (that seems unfriendly anyway, we shouldn't be messing with user configurable gconf keys just because something breaks, unless there is no alternative).

If you remove that, Ariel, then I think you will have addressed all the problems Vincent noted.  Then we/I can just fix the atk-bridge so that a missing/broken registry doesn't cause all the apps to SEGV :-/  (which it does now, so submitting this patch without changing gconf keys should not cause an end-user regression).

Bill

Created attachment 74887 [details] [review]
If no registry found we do not turn off a11y gconf flag

If no registry found we do not turn off a11y gconf flag

Bill/Vincent

Can you please take a look at this patch?

ariel

I've committed the patch, but I prefer to leave the bug open since it could still be improved in CVS. I'll look at it.

With this patch I'm seeing a warning dialog on login when running under valgrind. The dialog goes away when not running the session under valgrind. Maybe some race condition?

Indeed, there is a (2 second) timeout in the new patch, to prevent hanging the desktop session if something goes badly wrong with the at-spi-registryd launch.

Perhaps we need to add an env or command-line option to extend the timeout, or to suppress the timeout altogether?

Um, guys ... is this breaking GNOME apps running under KDE?

  http://www.redhat.com/archives/fedora-devel-list/2006-November/msg00227.html

Hmm, well that could happen IF the user had turned on assistive technology support in their Gnome session, then logged into KDE and ran a Gnome app from there.

The failure should not be fatal, so that IMO is the main issue here; if the registry is not found, the atk-bridge should complain, but not cause the app to exit or segv.  

Also, update POTFILES.in:

"
I have tested module gnome-session (CVS branch HEAD) which lives
in GNOME CVS for missing translatable files with `intltool-update -m` command.

Here is a content of the 'missing' file:

gnome-session/gsm-at-startup.c
gnome-session/gsm-remove-client.c

Please put these files into POTFILES.in or POTFILES.skip files. Thanks.
"

I added the missing files in POTFILES.in.

I think it would be best to suppress the warning or to put timeout to 10 seconds (it should be enough even for slow systems).

Can we please do something to get rid of the annoying dialog on login? at-spi-registryd is running after I click "Ok" so the warning seems to be plain wrong also...

Even if you have a running at-spi-registryd that does not mean you have an at-spi-registryd that has set the IOR as an x atom property. This can happen if you are running and older version of at-spi

ariel

It is happening with current version of at-spi-registryd which is setting IOR as X atom ....

Frédéric: I believe the current timeout is 2 seconds. 10 really sounds a lot. Waiting 5 should be enough, I believe.

Also, it should be possible to automatically close the error dialog if we detect that the IOR is set. Anyone wanting to cook a patch? :-)

I've tested with a 5s timeout and dialog doesn't appear on my slow to hell test system :)

I've committed timeout change to 5s, with Vincent approval.

Reopening since I wanted to keep the bug open as a reminder for other improvements.

Vincent, please open a new bug for those improvements.  It would be much preferred on our end.  Thanks.

Sure. This is bug 408034.

Thanks Vincent!

I'm still experiencing this problem with gnome-session trunk and at-spi trunk. Example output:

  Bonobo accessibility support initialized
  GTK Accessibility Module initialized
  ** (eog:14601): WARNING **: AT_SPI_REGISTRY was not started at session startup.
  ** (eog:14601): WARNING **: IOR not set.
  ** (eog:14601): WARNING **: Could not locate registry

If I start $prefix/libexec/at-spi-registryd manually the errors "vanish".

Furthermore, accessibility is DISABLED in gconf, so I'm not sure why this happens...

Wouter: I believe what you're seeing is not related to this bug. Can you open another bug? I'm seeing the same issue, fwiw.

It seems something is setting the GNOME_ACCESSIBILITY environment variable to 1, which enables accessibility. I don't think it's a gnome-session bug, so maybe opening it against atk or at-spi so accessibility people are aware of it will help find the root of this issue.

Yay, found out what's going on. I bet you're running jhbuild? It unconditionally sets GNOME_ACCESSIBILITY to 1 (see jhbuild/config.py), but it seems gnome-session only starts at-spi-registryd if the gconf key is checked, which was not the case on my setup, leading to the errors seen above.

Created attachment 88496 [details] [review]
Proposed fix

Check for GNOME_ACCESSIBILITY environment variable as well.

Please let me know whether I should commit this.

Reopening (again, sorry Vincent), the issue is related to this report, which boils down to "gnome-session should start at-spi-registryd if appropriate".

Thanks. I committed a slightly modified version of this.

This is what I get, after GDM login I get a short notice teling me to see if at-spi is installed . It is installed version 1.21.5-1 to be precise. Gnome-session is 2.21.91-0ubuntu2 . I get errors like :-


(totem:9648): atk-bridge-WARNING **: AT_SPI_REGISTRY was not started at session startup.

(totem:9648): atk-bridge-WARNING **: IOR not set.

(totem:9648): atk-bridge-WARNING **: Could not locate registry

(totem:9648): atk-bridge-WARNING **: AT_SPI_REGISTRY was not started at session startup.

(totem:9648): atk-bridge-WARNING **: IOR not set.

(totem:9648): atk-bridge-WARNING **: Could not locate registry


Looking forward for replies.