GNOME Bugzilla – Bug 609633
Orca can lock up the desktop when exiting apps if in flat review
Last modified: 2010-08-01 23:09:49 UTC
Created attachment 153517 [details] Example application to reproduce the bug (python app) I have found a case where orca sometimes locks up the desktop if you exit an application while in flat review. This bug doesn't happen everytime but for the attached application (not of practical use, just something I created to explore orca scripting but demonstrates this bug well) I get it quite regular. Steps to reproduce: 1. Run the attach application. 2. use flat review to explore the window. 3. Press alt+f4 to exit. expected: Orca will speak whatever gets focus and let you get on with using the desktop Actual: Orca locks things up, either requiring restarting orca or using the mouse to give something else focus seems to work too. While I have filed this against orca 2.29.x, I also see this on orca 2.28.3.
Created attachment 153519 [details] A debug file of what is going on. Looking at the attached debug file it seems that something is wrong with the event.source object, getting the state from pyatspi raises a LookupError.
Created attachment 153520 [details] [review] Possible fix This patch seems to fix the problem for me. As I am still learning about the internals of orca I can't say if it is the best way to solve this or if it will have bad unexpected problems (I haven't noticed any).
Review of attachment 153520 [details] [review]: Something seems odd here. The try/except block you added is already inside a try/except block that also listens for a LookupError. When the existing code catches this exception, it just spits out a stack trace at LEVEL_WARNING and returns from the method immediately. So, I'm not sure how the patch is fixing the problem. I've also not been able to reproduce the problem, with or without the patch. :-( When testing with the test app and the orca code from git master, I am seeing an additional stack trace on the command line used to launch orca: Traceback (most recent call last):
+ Trace 220542
globalsDict[arg] = self._methodsDict[arg](obj, **args)
parent = obj.parent
return _getAndCache(self, 'parent', self._get_parent)
value = get_method() COMM_FAILURE
The exact method where it seems to emit the trace seems to vary from run to run, most likely because of timing in processing the events. I suspect we might need a try/except block somewhere to catch these things. What strange is that all the likely places where we *should* have try/except blocks, we do. There might be some code path somewhere related to flat review that isn't protected, but I'm not seeing it. The above stack trace, which is the *complete* trace I'm getting, is also odd -- it seems incomplete. Hmm.
+ Trace 220543
This is what I understand to be happening: When I exit line 540 of focus_tracking_presenter.py causes a LookupError when the event.source.getState() call is made. Now without the patch we just run to the except dump error messages and continue, so we don't try and exit flat review or any of the other stuff we do when recieving the window:deactivate event for a object with STATE_DEFUNCT. By catching the exception where my patch does I make it possible to do all the stuff for window:deactivate, which I think cleans things up and allows orca to continue. I have to admit I don't know why pyatspi raises LokupError in this case and what it means. I think there a lot of timing issues here and why it doesn't appear on some machines but others. I will look through my debug file to see whether I get those tracebacks for when starting the app. The only other possibility, is my test app written wrongly and so possibly causing issues where proper use of pygtk wouldn't cause these issues? (In reply to comment #3) > Review of attachment 153520 [details] [review]: > > Something seems odd here. The try/except block you added is already inside a > try/except block that also listens for a LookupError. When the existing code > catches this exception, it just spits out a stack trace at LEVEL_WARNING and > returns from the method immediately. So, I'm not sure how the patch is fixing > the problem. I've also not been able to reproduce the problem, with or without > the patch. :-( > > When testing with the test app and the orca code from git master, I am seeing > an additional stack trace on the command line used to launch orca: > > Traceback (most recent call last): >
(In reply to comment #5) > This is what I understand to be happening: > When I exit line 540 of focus_tracking_presenter.py causes a LookupError when > the event.source.getState() call is made. Now without the patch we just run to > the except dump error messages and continue, so we don't try and exit flat > review or any of the other stuff we do when recieving the window:deactivate > event for a object with STATE_DEFUNCT. By catching the exception where my patch > does I make it possible to do all the stuff for window:deactivate, which I > think cleans things up and allows orca to continue. Thanks for the explanation. Now it makes sense what the patch is doing -- you want the flat review stuff to get cleared regardless. In looking at this entire block of code, something looks odd with the original code, which was added for bug #561548. It looks like it is going to kill flat review if any window goes away, even if the window isn't the one being reviewed (e.g., imagine some other application decided to quit/die). I tried reproducing that, though, and couldn't make that happen either. Maybe it was because I was killing the app via a remote shell and that didn't give the app enough time to die gracefully so it would send out a deactivate event. So, I guess the patch looks OK now that you've explained it (thanks!). It's a little bizarre to see the LookupError exception be caught inside a try block that's also catching it, though. I'll think about this for a little bit and see if any other ideas pop to mind. The main notion is to clear flat review when the thing it is reviewing has disappeared. That could potentially be done somewhere else, but I'm not sure where yet. > I have to admit I don't know why pyatspi raises LokupError in this case and > what it means. I believe this means the object basically no longer exists, which makes sense here. Orca's default event processing mode is asynchronous, so by the time Orca processes the event, the event source may have been destroyed. > The only other possibility, is my test app written wrongly and so possibly > causing issues where proper use of pygtk wouldn't cause these issues? The app looks OK to me. I'm not sure if using gobject instead of glib would make any difference.
Created attachment 153681 [details] Python app to help check if background windows closing cause problems While the patch may look odd in its use of try blocks, the inner one is being very specific so it knows where the exception came from and so we can have a good idea why it was raised. I realise looking back the patch may be puzzling, I wonder if it would be clearer to put some code in the except block (IE. state = [pyatspi.STATE_DEFUNCT]). I really think if the patch were to be used it will need comments to explain why its done. As for whether a background window closing would cause problems, I wonder if the attached python app will help, it closes after a number of seconds you specify on the command line (eg. $ timed_destroy 15 will mean it will close after 15 seconds).
Created attachment 153718 [details] [review] Alternative patch This is an alternative patch that moves the clearing of the flat review context outside the try block. It seems to work well for me. Mike - can you give it a try?
(In reply to comment #7) > Created an attachment (id=153681) [details] > Python app to help check if background windows closing cause problems This app worked great, and I was able to reproduce the hang, both with and without the patch you have provided. What I did was: 1) Run Orca 2) Run the timed_destroy app and press KP_5 to flat review it. 3) Wait for the app to die. 4) Flat review still thinks the app has focus and Orca's behavior goes down the tubes. This new patch seems to work well both under the condition above and also in the condition where I am flat reviewing a different dialog and the timed_destroy application goes away in the background.
This patch works well for me, it seems to solve the problem. Reading what you said about timed_destroy app, it sounds like the issues may be a timing issue and so probably explains why it could have been hard to reproduce reliably. (In reply to comment #8) > Created an attachment (id=153718) [details] [review] > Alternative patch > > This is an alternative patch that moves the clearing of the flat review context > outside the try block. It seems to work well for me. Mike - can you give it a > try?
Comment on attachment 153718 [details] [review] Alternative patch Thanks for testing this. I've committed it to git master for the 2.29.91 release.