GNOME Bugzilla – Bug 345434
GDM needs to start at-spi's registryd
Last modified: 2006-10-27 03:43:37 UTC
Bug 163132 might that gdm starts registryd so a11y login continue to work with the proposed patches attached to that bug.
It sounds like there is a need for GDM to start at-registryd-spi. Previously, I believe it was getting started because when a11y is turned on in GDM, the GDM GUI programs are launched with the gail and atk-bridge GTK_MODULES which seemed to start it for the gdm user. There are a couple of ways I can imagine we could make GDM launch at-registryd-spi. Probably any of these methods would be acceptable. 1) AT programs are launched by the gesture listeners (gui/modules/*.c) so there is probably only a need to run at-registryd-spi if a gesture has been recognized and an AT program is going to be launched. Perhaps it makes sense to launch at-registryd-spi in the gesture listener modules just before launching the AT program. The problem with this approach is that we don't want to start the at-registryd-spi multiple times if the user completes multiple gestures. So this approach probably only makes sense if it is possible to tell that the at-registryd-spi program hasn't yet been started and only launch it for the first gesture. 2) The gui/gdmlogin.c and gui/greeter/greeter.c files could be modified to just start the at-registryd-spi process if the GDM_KEY_ADD_GTK_MODULES config value is set to true. Note that this is very similar to what is done with GDM_KEY_PRE_FETCH_PROGRAM and you could use similar logic to launch at-registrdy-spi. We'd still probably need some logic to make sure at-registryd-spi isn't launched if it is already running. Note below I discuss you can switch between gdmchooser and the login GUI, so we'd want to make sure we don't keep launching more processes if it is already running. Some testing would need to be done. Note that from the login screen that you can switch to the "Remote login via XDMCP" which causes the Xserver to restart and the gdmchooser program is started. If doing this causes the at-registryd-spi to exit, it may also be necessary to launch the at-registryd-spi process from gdmchooser. With approach #1 this probably wouldn't be a big deal since you have to re-launch the gestures when switching between the login GUI and gdmchooser anyway. Still, I'd recommend testing this with either approach and verify that GDM is smart enough to know when to launch at-registryd-spi when this switch happens. We probably want to make sure that multiple at-registryd-spi's are not accidently launched when switching back and forth. Likewise you can switch to gdmsetup by picking "Configure Login Manager" from the login GUI, so I'd recommend testing this also. I wouldn't anticipate this would break since X isn't restarted in this case and gdmsetup is just run with focus so it is in front of the login GUI. So there probably isn't a need to restart the at-registryd-process when gdmsetup is started. Note that when you change certain configuration settings in gdmsetup (such as switching between gdmlogin and gdmgreeter by changing the Style dropdown on the Local tab between "Themed" and "Plain"), that this causes the GDM GUI to restart itself. We probably want to make sure this restarting doesn't cause multiple at-registryd-spi processes to get launched. I'd be happy with either approach. The advantage of approach #1 is that it only starts the at-registryd-process when it is needed (when a gesture is completed).
Created attachment 68081 [details] [review] Some preliminary work Brian et al, I'd done some partial work on proposal #2 and want to get some feedback from you. I have done the followin changes: gdmcommon.c: Added functions to launch the registry if GDM_KEY_ADD_GTK_MODULES. gdmlogin.c: Get GDM_KEY_ADD_GTK_MODULES and at the end call gdm_atspi_launch () defined on gdmcommon.c greeter/greeter.c: Get GDM_KEY_ADD_GTK_MODULES Anything more I am missing? One remaining issue I see is that on gnome-session we first run at-spi-registryd and the we need to start the component, the latter is done by doig bonobo_activation_from_id. I don't think is a good idea to put bonobo code on gdm so we can either create a small app that starts the component or better yet let the bridge do this only if you are not running under gnome-session and are inside gdm (or something like that).
Your patch needs work, but you already said it is partial and preliminary, so I'll let you know what more you need to do: 1) You didn't include daemon/gdm.h and daemon/gdmconfig.c in the patch, which I'm sure had to change for this to work. Did you notice the comments at line 141 in daemon/gdm.h which explain what work you should do when you change or add a configuration value? 2) In the pre_atspi_launch function you are using variables with "pre_fetch" in the variable name. Please rename these to atspi or something to distinguish this code from the prefetch code. 3) Why do you not include this key in the reread function? If somebody changes the value and calls "gdmflexiserver --command="SET_CONFIG keyname" don't you want the running GUI's to restart and notice the change? Note in the reread function there is a big if-test that causes the GUI to restart if any of a large number of keys change value. Probably okay to add this key to that. 4) Note in setup_accessibility_tab and acc_modules_toggled, that the Accessibility checkbox is turned on based on the settings of the GtkModules and GtkModulesList keys. Probably should make these functions also use/modify the values of the new key so it gets turned on/off when the a11y enable checkbox is changed. 5) You did not add the new key to the docs/C/gdm.xml file. GDM policy is not to accept patches that affect configuration unless the patch includes the documentation changes to this file. 6) in gui/greeter/greeter.c you read the config value, but you do not actually call the gdk_common_atspi_launch function. This can't be right. Probably also want to add the key to the reread function here as well.
I used GDM_KEY_ADD_GTK_MODULES assuming that if that is enabled and your loading the modules you have a11y on. It might make sense to have another one (something like GDM_KEY_ATSPI_REGISTRY) and then follow your commentts on adding the key.
Okay, forget comments #1, #4 and #5 above. The other issues still apply, though.
Okay, forget comments #1, #4 and #5 above. The other issues still apply, though. Since this key wasn't previously accessed by the GUI gdmlogin and gdmgreeter programs, the key should probably be added to both the read and reread functions, causing a restart if reread notices that it changed. Also issue #6 seems like a bug. Not even actually calling the gdm_common_atspi_launch_function seems broken.
Any progress?
I am having a problem. I do gdm-restart and gdm dyes two times. Looking at the code I found that slave.c first tryes to use gdmgreeter and after a few times of failing it tryes using gdmlogin. Interesting to notice is that gdmlogin works correctly and at-spi-registryd is also launched correctly. Im basically doing the same on both gdmlogin and greeter. I had even tryed to launch the registry from slave.c but that doesn't work either -I prolly doesn't make sense to be here anyway. I have basically the same code I posted only that now I start theregistry almost after calling the config_read so I can start the registry as early as possible. Any idea?
There could be a lot of reasons why gdmgreeter is failing to start. Try logging into your session and set DOING_GDM_DEVELOPMENT=1 and run gdmgreeter. Does it generate any useful error messages explaining why it won't start? If this doesn't reveal anything, please turn on debug in your configuration file, and allow GDM to fail to start gdmgreeter and share the messages that GDM sends to the system log (/var/log/messages or /var/adm/messages). Does gdmgreeter generate a core file? Look in the root directory, the /var/lib/log/gdm, /var/lib/gdm, and the GDM home directory (if the gdm user has a home directory defined in /etc/passwd).
I enter as user ariel and ran /usr/libexec/gdmgreeter and got: [ariel@paula gdm2]$ /usr/libexec/gdmgreeter GTK Accessibility Module initialized Xlib: extension "XEVIE" missing on display ":0.0". Gtk-CRITICAL **: gtk_tree_view_get_selection: assertion `GTK_IS_TREE_VIEW (tree_view)' failed aborting... Aborted (gdb) bt
+ Trace 69309
Could you test and see if user_list is NULL when the function is called? Does it work better if you just return from the function if the user_list value is NULL? Are you using a greeter theme with Face Browser enabled? user_list should only not be NULL when using a theme with a face browser (such as happygnome-list).
I now have: if (user_list == NULL) return; and inside my session gdmgreeter comes to the screen. If I reboot it keeps crashing and ultimately starts gdmlogin.
This is really strange. It should generate a core file. What is the stack track for the generated core file? You weren't having this problem before, were you? Are you sure that changes you made to the code aren't causing the problem? If you rebuild GDM without your code changes, does it still fail?
Ok so I went to Style->THemed->Circles so I went and changed that to Plain. Restarted gdm and the greeter comes to the screen! Everyhting looks to be correct. The at-spi-registry is running, I got the sounds, I can log in... But I still think there might be something not right. The original gdm I had was from fc5 so now I am compiling from CVS head I will get a fresh tree and compile that one without my changes to see what happens.
So I download a fresh tree, move back the at-spi packages (w/out my changes) and they work. Also I made another finding. Actually on the last comment I was right, there is still something wrong. That is, if I have greeter selected it fails and the gdmlogin appears. But if I select gdmlogin at the start then it fails and the one appearing is gdmgreeter. So my guess right now is that there is something wanting to use the atk-bridge before either the greeter or the login and that one is failing. It can be slave.c but just don't know. I'll look more into it.
I did more work and I think I have a working version now. I'll clean the code and send a patch later today. Still not sure what the problem was, tho.
Created attachment 68934 [details] [review] let gdm start at-spi-registryd Ok. I found the problem. I have something like this on greeter.c and gdmlogin.c: gdm_common_atspi_launch (); gtk_init (&argc, &argv); The problem is that the first function launches the at-spi-registryd asynchronously so when the code arrive to the gtk_init and try to load the gtk modules, the atk-bridge looked for at-spi-registryd which was not yet loaded. So after a few times of trying gdm switchtes to gdmlogin but does not includes the modules. So there's also the reason why if you started with gdmlogin it will not work and after a few times switchetd to the greeter and that worked and viceversa. So I solve this adding a g_usleep (1) right after doing g_spawn_async(at-spi-registryd). This patch uses GDM_KEY_ADD_GTK_MODULE as the key to start the registry. Although it still might be better to add another one at least for now this should be good.
This patch looks much better than the last patch. I don't like the sleeping for 1 second. This doesn't really fix the race condition, just makes it more likely that the race won't cause a failure. If it is necessary for this call to finish before GTK starts, then why are we starting it async? Shouldn't we run it "sync" and wait for it to finish before going on to bring up the GUI? This would be a better fix, I'd think.
Ok. I am trying to do the following (on greeter.c): gdm_common_atspi_launch (); sleep (1); gtk_init (&argc, &argv); gsm_watch_ior (flagg); gsm_watch_ior is defined as: void gsm_watch_ior (gboolean flag){ XSelectInput (GDK_DISPLAY (), GDK_ROOT_WINDOW (), PropertyChangeMask); gdk_window_add_filter (gdk_get_default_root_window (), filter_watch_ior, (gpointer) &flag); } If I remove the sleep then in the moment we enter gtk_init the app crashes since it tryes to load the modules (gail, atkbridge et al) It breaks since the bridge requires the registry to be running. If I move gsm_watch_ior () before gtk_init then I get a crash since I the calls for the functions it require only start working after do not actually start working until you have called gtk_init. Im having a similar problem with gnome-session. Maybe, I am not using add_filter correctly. I am just not sure. ariel
You don't have to use a gdk function to wait for the X property to be changed? Why not call X functions directly?
So I change gsm_watch_ior to this: void gsm_watch_ior (gboolean flag){ Display *d = XOpenDisplay (getenv("DISPLAY")); Window w = DefaultRootWindow(d); XEvent event; XSelectInput (d, w, PropertyChangeMask); XIfEvent(d, &event, WaitForNotify, (char*) w); } I am getting something similar. I get NULL when calling XOpenDisplay and then when trying to get DefaultRootWindow the greeter dyes. If I call similar lines after gtk_init (using the sleep) then I get a correct value for the display.
It sounds like the right solution is to fix the atk-bridge so that it is smarter about waiting for at-registryd-spi to start rather than putting a sleep() into GDM hoping this will resolve a race condition. Unless we can think of a better way to identify when at-spi-registryd has actually started. Is there any other mechanism to notice when it has started aside from the X-property?
Is Ariel's 7/14/06 patch acceptable? If not, then how about replacing g_usleep() in pre_atspi_launch() with the following: if (kill(pid, 0) < 0) { fprintf(stderr, "at-spi-registryd not running: %s\n", error->message); return FALSE; } Atom AT_SPI_IOR = XInternAtom (GDK_DISPLAY (), "AT_SPI_IOR", False); for (i = 0; i < 3; i++) { 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; } } fprintf(stderr, "at-spi-registryd IOR not found\n"); return FALSE;
That's a good suggestion, though it might not eliminate the race condition. If the bridge is launched before at-spi-registryd posts the Atom, it would still fail - I'd think. It seems that the most reliable fix would be to fix the bridge so that is waits a bit before deciding to fail. Using g_usleep() seems like a really bad way to try and deal with a race condition, anyway.
I think there is a better way. Currently both the greeter and gdmlogin receive the gtk modules on argv and then calling gtk_init with that makes the bridge to fail since there is yet no registry and Atom initialized. What about if we do something like: if (argv == NULL) gtk_init (argc, argv); else { gtk_init (argc, NULL); gsm_wait_for_ior (); // init here the modules so they don't break // I think this is way better. ariel
Any update on this?
Sorry, I've been distracted. This is my top coding priority. I agree with a recommendation that I received that this should be postpone until gnome 2.17; however, i will try to work on as soon as possible.
Per comment 22 from Brian, the bridge has been fixed. The current patch should be code complete.
If the bridge has been fixed, then the call to g_usleep () should not be needed. Could you confirm and please repost the patch without the g_usleep() call? Also is the modification to gui/greeter/greeter_item_ulist.c still necessary? I think this was a hack we implemented to work around a crash that you were seeing? But I wonder if the crash was really due to the problems with the registry daemon not starting properly? I'm guessing this probably works now without changing the code now that we have fixed the bridge? Lastly, it would be better if you updated docs/C/gdm.xml so that the documentation for the GDM_KEY_ADD_GTK_MODULES key should be updated to mention that if this is on that GDM will start the registry daemon, starting with version GDM 2.17. Also probably good to add a sentence or two about this in the Accessibility section of the manual. I am leaving the existing patch (comment #17) as "needs-work" and hopefully you'll be able to provide an updated patch with these minor changes.
Also, George, I am not sure anyone with an interest in a11y has reviewed the Accessibility section of the manual. If you would review and any suggestions for improving (making more clear, etc.) would be appreciated.
With the work on the bridge done as latest comment on http://bugzilla.gnome.org/show_bug.cgi?id=163132 says, there is no longer a need for modifying gdm. However, I think that gdm.xml should note that setting GDM_KEY_ADD_GTK_MODULES will launch the registry via the atk-bridge so gdm can use it for a11y login.
So why did George suggest the current patch is code complete? I assumed he meant that this patch should go into GDM?
Created attachment 73673 [details] [review] Remove polling on patch. Brian / Bill, I reworked this patch following the solution for gnome-session. They are basically the same although here I first do gdk_init so I can get the reference to the X display an X root window. The text that appears on the error dialog should be different and I accept your suggestions for that. regards ariel
The patch looks good, but you did not add any comments to the docs/C/gdm.xml to explain that GDM will now launch the registry daemon. I would prefer to not add the patch unless you also include documentation updates. The section for GDM_KEY_ADD_GTK_MODULES key should explain that turning this key on will cause GDM GUI to launch the registry daemon. Also good to add a mention in the Accessibility chapter. Also you didn't respond to my comment about the need to modify gui/greeter/greeter_item_ulist.c. Refer to comment #29 above. Also, should this patch go into 2.17 (CVS head) or also 2.16 branch?
Created attachment 73971 [details] [review] Documentation added to gdm.xml and removed code from item_uilist I have included in this patch your latest comments adding some sentences to gdm.xml to reflect the new registry loading. Also I removed the code from gui/greeter/greeter_item_ulist.c as it is as you said no longer necessary. This goes into 2.17 (CVS HEAD).
Thanks. The docs look great. I notice one additional issue with the patch, sorry for not catching it sooner. I notice that you hardcode "libexec" in the code. Note that this will break on Operating Systems like Solaris where libexec is mapped to /usr/lib. Note that you can call configure with --libexecdir to specify a different directory. Note that in gui/Makefile.am that -DLIBEXECDIR is set to the real libexecdir directory and LIBEXECDIR is used in the code in several places in gdmsetup.c. I'd recommend using this method to set the path to call the registry daemon. This could break if users specify different libexecdir directories when building the registry damon and GDM. Might be a little cleaner to add a new configure option so that the location of the registry daemon can be specified (if different than the default), but I don't think we need to worry about this now. If users want to set different libexecdirs for the two different modules, then we can patch the code later to add this sort of additional configure option. Could you update the code to use LIBEXECDIR and verify that the code works and resubmit the patch. If it works, you can go ahead and commit this patch to CVS head. If you don't have CVS commit permissions, then let me know and I'll commit. Thanks.
Created attachment 74022 [details] [review] Include Changelog and and replace path with libexecdir I include Changelog and replaced the path with libexecdir. Alhough I have cvs account per IBM policy I cannot commit code myself.
Looks good. Thanks. Committed to CVS head.
Comment on attachment 74022 [details] [review] Include Changelog and and replace path with libexecdir >+ * gui/gdmlogin.c (main): Launch gdk_init and then lunch the >+ at-spi-registry if GTK_ADD_MODULES is set on. I think that eventually we should add another config output to gdm.conf instead of overloading the meaning of GTK_ADD_MODULES here - seems fragile and potentially a complaint generator. Also there is a small typo in the warning message in error_dialog(). I wonder about the comment about the atk-bridge crashing if the registry is not running - are you sure about that? atk-bridge shouldn't crash in this case, it should just do nothing. Otherwise looks great, thanks Arial and Brian!
This fix seems to be causing core-dumping problems. Did you test this Ariel? Note bug #363697. I'm re-opening this bug. I will back out this change if it is not corrected by the next 2.17 release.
Brian, Yes I have checked and actually my everyday system has been working with this patch for quite some time. The problem should be that we have not added the corresponding at-spi and gnome-session patch that should be available by gnome 2.17.2 ariel
Ariel, I don't like this solution. GDM shouldn't crash when using it with an older version of gnome-session. It should fail to start the daemon more gracefully. Isn't there something GDM could do to check if the version of gnome-session and at-spi is the right version, and only start it up if using 2.17.2 or later? Perhaps we should add a new configuration key, as Bill suggests in comment #39 and turn it off by default until we verify that in 2.17.2 everything works with it turned on. This way people using GDM in older environment could turn the configuration key off. Though I still think it would be better to check at runtime and avoid starting the daemon if it will cause GDM to crash.
Created attachment 75408 [details] [review] Never die for a missing registry I have made some modifications that work for the next test cases: -New gdm new at-spi (with ior): If a11y is selected in case we don't find a registry, the latest at-spi work will prevent crashes. The greeter will work without accessibility. -New gdm old at-spi: Since we will never found a registry we we'll write a message to syslog and the greeter will work with accessibility. Pretty much will be the same as it is now. I have checked that when no a11y is selected no crash happens as in bug #363697.
Thanks, this does seem to fix the problem. Marking as fixed again.