Don't handle most events from background processes by default #3831

Closed
nvaccessAuto opened this Issue Jan 31, 2014 · 40 comments

2 participants

@nvaccessAuto

Reported by jteh on 2014-01-31 07:25
We do filter some events (e.g. MSAA show events are restricted to certain window classes), but for the most part, we unconditionally handle all events we listen for. This can lead to poor performance if an unresponsive (or very slow to respond) background application fires an event. One example is an application performing some task while displaying a progress bar, but the task blocks the GUI for long periods of time. Even if you switch to another app and have reporting of background progress bars disabled, NVDA will freeze for long periods in this situation. To work around this, we should stop handling most events from background processes by default. Because some code might need background events, we should provide a mechanism to request specific events for specific processes.
Blocked by #3849, #3850, #3897, #3899, #3905, #4001
Blocking #3964

@nvaccessAuto

Comment 1 by jteh on 2014-02-05 06:04
I can't think of any code (core or add-ons) that needs background events at this stage. The ability to request events might be useful, but I don't think it needs to block this change.

@nvaccessAuto

Comment 2 by jteh on 2014-02-05 09:29
Changes:
Milestone changed from None to next

@nvaccessAuto

Comment 3 by James Teh <jamie@... on 2014-02-05 09:30
In [4e28381]:
```CommitTicketReference repository="" revision="4e28381c234559acf16af59ff065a31e74d8dde2"
Merge branch 't3831' into next

Incubates #3831.

Changes:
Added labels: incubating
@nvaccessAuto

Comment 6 by Michael Curran <mick@... on 2014-02-07 07:06
In [af418cf]:
```CommitTicketReference repository="" revision="af418cf88d6ae37b0ec38c12c6eb843b4c3c568c"
Merge branch 't3831' into next. Incubates #3831

@nvaccessAuto

Comment 7 by James Teh <jamie@... on 2014-02-13 01:31
In [4f111ab]:
```CommitTicketReference repository="" revision="4f111ab646674a40378965004456046a784e0f70"
Merge branch 't3831' into next

Incubates #3831.

@nvaccessAuto

Comment 8 by James Teh <jamie@... on 2014-02-14 05:56
In [d77629d]:
```CommitTicketReference repository="" revision="d77629df339db70cba410c1e9241f20cad5eaf82"
Merge branch 't3831' into next

Incubates #3831.

@nvaccessAuto

Comment 12 by James Teh <jamie@... on 2014-02-22 08:50
In [e55984b]:
```CommitTicketReference repository="" revision="e55984bccda663452c78462c0a1fe958adf779a1"
Merge branch 't3831' into next

Incubates #3831. Fixes #3899, #3905.

@nvaccessAuto

Comment 13 by camlorn on 2014-02-24 17:11
I think that this issue is part of this ticket, or at least part of something related to it. I've been experiencing a few quite possibly related things recently, all of which are fixed by a restart of NVDA: in Firefox, the object navigator can become untethered from the focus; in command prompt, the review cursor can become untethered from the carrot; all menus (context or menu bar) can stop reading; and NVDA can stop processing some or all of the stuff related to text fields (sometimes, it's just the loss of selection indication, sometimes more, and can be both system-wide or not). Once these start, there is no fix save for an NVDA restart, but NVDA restarts will fix it reliably. I cannot trigger it, nor have I seen a similarity between the times it's happened.
Based off my very limited knowledge of how NVDA works, this "feels" like this ticket. If this needs to be elsewhere, I'll extract it out-I could be quite, quite wrong.

@nvaccessAuto

Comment 14 by jteh (in reply to comment 13) on 2014-02-28 22:06
Replying to camlorn:

I've been experiencing a few quite possibly related things recently, all of which are fixed by a restart of NVDA: in Firefox, the object navigator can become untethered from the focus; in command prompt, the review cursor can become untethered from the carrot; all menus (context or menu bar) can stop reading; and NVDA can stop processing some or all of the stuff related to text fields

I doubt it's this code, as I'd expect things ot faill constantly rather than after some time. That said, here's a try build with next minus the code for this ticket. If you can't reproduce it while running this build, you would appear to be correct and we've at least isolated the issue. If you can, we'll need to look elsewhere.

@nvaccessAuto

Comment 15 by nvdakor on 2014-03-01 03:31
Hi,
A concern is if the user needs to obtain info (via sounds or ui.message) from a background process, and with implementation of this code, this might have been broken for some apps. Specifically, there are some codes in app modules such as SPL Studio which alerts the user that something is going on in SPL Studio from within other apps.
So would it be possible to provide a workaround or an exception system for certain app modules?

@nvaccessAuto

Comment 17 by James Teh <jamie@... on 2014-03-04 03:51
In [646c88c]:
```CommitTicketReference repository="" revision="646c88c352ded72a9f818c1862a23941f594f0aa"
Merge branch 't3831' into next

Incubates #3831.

@nvaccessAuto

Comment 20 by James Teh <jamie@... on 2014-06-24 06:34
In [6f0f19f]:
```CommitTicketReference repository="" revision="6f0f19f1524e2c75cdd29280e7e9640a520f5ca0"
Merge branch 't3831' into next

Incubates #3831. Fixes #4001.

@nvaccessAuto

Comment 21 by jteh (in reply to comment 15) on 2014-06-24 06:42
Replying to nvdakor:

Hi,

A concern is if the user needs to obtain info (via sounds or ui.message) from a background process

As an aside, it's worth noting that this generally shouldn't be done unless a sighted user would get equivalent background notification.

Specifically, there are some codes in app modules such as SPL Studio which alerts the user that something is going on in SPL Studio from within other apps.

Can you be more specific about what events splstudio needs? For example, do you know the window class name from which you would need events and do you know exactly what events you need?
Aside from splstudio, do you know of any other add-ons that currently need this? Do you know what their requirements are?

In the API I'm considering, there would be an eventHandler.requestEvents function that takes eventName, processId and windowClassName. The question is whether windowClassName needs to be optional. I'd prefer that it weren't optional, but if there are already cases where the window class name is very dynamic, it might have to be optional.

@nvaccessAuto

Comment 22 by nvdakor (in reply to comment 21) on 2014-06-24 11:57
Replying to jteh:

Replying to nvdakor:

Hi,

A concern is if the user needs to obtain info (via sounds or ui.message) from a background process

As an aside, it's worth noting that this generally shouldn't be done unless a sighted user would get equivalent background notification.

Specifically, there are some codes in app modules such as SPL Studio which alerts the user that something is going on in SPL Studio from within other apps.

Can you be more specific about what events splstudio needs? For example, do you know the window class name from which you would need events and do you know exactly what events you need?

Aside from splstudio, do you know of any other add-ons that currently need this? Do you know what their requirements are?

In the API I'm considering, there would be an eventHandler.requestEvents function that takes eventName, processId and windowClassName. The question is whether windowClassName needs to be optional. I'd prefer that it weren't optional, but if there are already cases where the window class name is very dynamic, it might have to be optional.

Hi,
In case of Studio, the window class name is TStaticText (or something similar), the class used in status bars and other controls to show status information, with the event being name change. I think possibly Dropbox add-on might need this functionality, though you might want to ask the add-on author to verify this.
Thanks.

@nvaccessAuto

Comment 23 by nvdakor on 2014-06-24 18:15
Hi,
On an unrelated note, since today's snapshot, NVDA does not focus to files opened in Office apps when you open them from file managers.
STR:
1. From start menu/screen, open MS Office programs while today's NvDA next snapshot is running. Office apps opens (such as MS Word saying "document 1").
2. Using file manager of your choice (Windows Explorer, for example), navigate to a folder where you know you have a file that opens with Office (such as an Excel spreadsheet). Open the file in question.
Expected: NvDA focuses on the document.
Actual: a pane is focused with various window classes. One must switch away fro mthe app and back for nVDA to focus on the document.
The window classes that are shown when opening Office apps by opening the files are:

  • Word: OpusApp versus WWG.
  • Excel: XLMAIN versus Excel7. The latter are the ones shown when opening the program from start menu/screen. Thanks.
@nvaccessAuto

Comment 25 by leonarddr on 2014-06-27 15:17
I've been able to run NVDA from source and checkout [the issue i reported in #4001 doesn't occur in that case. In other words, 6f0f19f1524e2c75cdd29280e7e9640a520f5ca0 creates issues for me.

@nvaccessAuto

Comment 26 by James Teh <jamie@... on 2014-08-08 13:04
In [7b6b9d9]:
```CommitTicketReference repository="" revision="7b6b9d9580241c9cbc0ee8a07ef7c4563422e811"
Revert "Fix broken focus when starting certain applications such as Microsoft Word and Excel."

This was causing focus problems for NVDA windows such as the browse mode Find dialog (#4295) and could cause similar problems in other cases. It also wasn't solving many cases of focus problems when starting applications anyway.
This reverts commit d715c51.
Re #4001, #3831. Fixes #4295.

@nvaccessAuto

Comment 27 by jteh on 2014-12-22 06:55
When I eventually get back to this, we'll need the eventHandler changes in dea0a6c from next for Skype notifications (#4741).

@nvaccessAuto

Comment 28 by James Teh <jamie@... on 2015-01-23 07:53
In [522dc25]:
```CommitTicketReference repository="" revision="522dc25109d8c10102872f1f3a75c9be93f68472"
Merge branch 't3831' into next: hopefully fix some focus issues when starting applications.

I'm not sure this is final yet, but it will at least make life nicer for users using next.
Re #3831.

@nvaccessAuto

Comment 29 by James Teh <jamie@... on 2015-01-29 07:02
In [fd3ea4e]:
```CommitTicketReference repository="" revision="fd3ea4ee0b86ae6f0cd6cba506f01d72bd5eccb3"
Merge branch 't3831' into next: Fix more focus issues when the foreground changes.

Incubates #3831.

@nvaccessAuto

Comment 30 by jteh (in reply to comment 23) on 2015-01-29 07:04
Replying to nvdakor:

On an unrelated note, since today's snapshot, NVDA does not focus to files opened in Office apps when you open them from file managers.

Should be fixed in latest next. At least, it works for me now.

@nvaccessAuto

Comment 31 by James Teh <jamie@... on 2015-02-03 07:22
In [2010f39]:
```CommitTicketReference repository="" revision="2010f3984f3b21fb1ec90bf160f6e234c71a47fa"
Merge branch 't3831' into next: Add eventHandler.requestEvents.

Incubates #3831.

@nvaccessAuto

Comment 32 by jteh (in reply to comment 15) on 2015-02-03 07:25
Replying to nvdakor:

A concern is if the user needs to obtain info (via sounds or ui.message) from a background process, and with implementation of this code, this might have been broken for some apps. Specifically, there are some codes in app modules such as SPL Studio which alerts the user that something is going on in SPL Studio from within other apps.

I've just added an eventHandler.requestEvents function to cover this. Could you please test it with splstudio and ensure whether it does what you need?

Btw, once this graduates, you can use hasattr or check for AttributeError in order to support versions of NVDA without this if you need to support both.

@nvaccessAuto

Comment 33 by nvdakor (in reply to comment 32) on 2015-02-03 09:37
Replying to jteh:

I've just added an eventHandler.requestEvents function to cover this. Could you please test it with splstudio and ensure whether it does what you need?

Tested with Studio and it works.
Few observations:

  • I cannot place a pattern, but sometimes name change event is not fired (or not caught by NvDA).
  • This needs to be enabled when NvDA objects are first initialized, from within app module constructor, global plugin constructor and so on, as event_NVDAObject_init is not enough. Thanks - off to rewriting app module specific code to handle this new possibility.
@nvaccessAuto

Comment 34 by nvdakor on 2015-02-03 12:39
Hi,
A major problem found: if an app module requests events from background objects and when it dies, it leaves "zombie" entries around in the accepted events set.
STR (pseudo code):
1. Create an example app module that has an object you wish to monitor in the background.
2. In the app module constructor, tell NVDA's event handler (via the new request events function) to keep an eye on this object.
3. Run the new app module. The newly added object is registered in accepted events set in event handler.
4. Kill the app module and examine the accepted events set.
Expected: the entry for the dead process (PID) should be gone.
Actual: the "zombie" entry is still there.
Possible solution: a short-term fix might be to have a remove function that removes the entry when add-on code calls it. Another solution might be to let the app module handler scrape the accepted events set and remove "zombie" entries, either doing it on its own or utilizing the remove function.
Thanks.

@nvaccessAuto

Comment 35 by jteh on 2015-02-03 21:18
This did occur to me, but I'm not convinced it's necessary to clean it up. My thinking was that if the app were opened again, it would just request the events again anyway. However, I just realised it'll of course have a different process id, so that is a bit annoying.

@nvaccessAuto

Comment 36 by James Teh <jamie@... on 2015-02-04 00:14
In [720c914]:
```CommitTicketReference repository="" revision="720c914570bd0a55e7ecce9679075f5534bd72ed"
Oops. Remove accidentally committed debugging code that was breaking stateChange events.

Incubates #3831. Re #4880.

@nvaccessAuto

Comment 37 by jteh (in reply to comment 33) on 2015-04-08 05:48
Replying to nvdakor:

  • I cannot place a pattern, but sometimes name change event is not fired (or not caught by NvDA).

How often? Are you certain this isn't the case without this branch (e.g. in master)?

  • This needs to be enabled when NvDA objects are first initialized, from within app module constructor, global plugin constructor and so on, as event_NVDAObject_init is not enough.

That makes sense. event_NVDAObject_init is only called when NVDA creates the object; e.g. if an event is fired on it or the user navigates to it. However, if you haven't yet requested events, the event won't be received, so the object will never be created and thus event_NVDAObject_init will never be called. In short, yes, you should generally request events when your plugin is instantiated.

@nvaccessAuto

Comment 38 by nvdakor (in reply to comment 37) on 2015-04-08 16:01
Replying to jteh:

Replying to nvdakor:

  • I cannot place a pattern, but sometimes name change event is not fired (or not caught by NvDA).

How often? Are you certain this isn't the case without this branch (e.g. in master)?

I believe this isn't with master. This is a minor thing that doesn't affect the app module usage.

@nvaccessAuto

Comment 39 by jteh (in reply to comment 34) on 2015-04-09 03:47
Replying to nvdakor:

A major problem found: if an app module requests events from background objects and when it dies, it leaves "zombie" entries around in the accepted events set.

Fixed in aa63df3.

@nvaccessAuto

Comment 40 by James Teh <jamie@... on 2015-04-09 03:52
In [0edc1a9]:
```CommitTicketReference repository="" revision="0edc1a9c7e8e952494dbf5008ff50f569462e5ad"
Merge branch 't3831' into next

Incubates #3831.

@nvaccessAuto

Comment 41 by nvdakor on 2015-04-14 02:26
Hi,
A brief testing shows we might have hit a regression: module constructor is called recursively.
STR:
1. Have a module with a constructor (init).
2. In the constructor, ask event handler to keep an eye on an event for an object via request events function.
Expected: The constructor should be run once.
Actual: The constructor is called recursively, causing recursion depth exception to be thrown.
Possible regression: likely since the last commit to t3831 branch - does not occur in master nor 2015.1 stable.
Thanks.

@nvaccessAuto

Comment 42 by jteh on 2015-04-14 02:34
Oh dear. I guess the constructor calls requestEvents, then requestEvents tries to fetch the AppModule for that process, but because it isn't constructed yet, it ends up creating a new AppModule, which then calls requestEvents... and thus we get into a loop. Ug. I'll need to rethink this.

@nvaccessAuto

Comment 43 by James Teh <jamie@... on 2015-07-01 12:05
In [3293859]:
```CommitTicketReference repository="" revision="3293859600f890b4b143a8b06d6aa8d0e49e0633"
Merge branch 't3831' into next: Fix infinite recursion problem with eventHandler.requestEvents

Incubates #3831.

@nvaccessAuto

Comment 44 by jteh (in reply to comment 41) on 2015-07-01 12:07
Replying to nvdakor:

A brief testing shows we might have hit a regression: module constructor is called recursively.

Joseph, can you please test this now with latest next? The new code seems to work for me.

@nvaccessAuto

Comment 45 by nvdakor on 2015-07-01 17:02
Hi,
Tested via source code (not pushed to users yet), and it works now. Thanks.

@nvaccessAuto

Comment 46 by nvdakor on 2015-07-02 16:43
Hi,
Using the binary snapshot, when moving focus, I get error tones.
STR:
1. Open any app.
2. CLose the newly opened app.
3. Move focus to somewhere else.
Traceback is as follows:

ERROR - eventHandler.executeEvent (09:39:56):
error executing event: gainFocus on <NVDAObjects.Dynamic_TaskListListIAccessibleWindowNVDAObject object at 0x04F30710> with extra args of {}
Traceback (most recent call last):
  File "eventHandler.pyc", line 138, in executeEvent
  File "eventHandler.pyc", line 151, in doPreGainFocus
  File "api.pyc", line 70, in setFocusObject
  File "appModuleHandler.pyc", line 142, in cleanup
  File "eventHandler.pyc", line 212, in handleAppTerminate
KeyError: 4724

Is the pid of the closed app automatically removed from accept events set? Sounds like this is what's happening: event handler attempts to remove a pid that no longer exists. I'm thinking of catching key error and moving on.
Thanks.

@nvaccessAuto

Comment 47 by James Teh <jamie@... on 2015-07-03 00:29
In [53b4bf5]:
```CommitTicketReference repository="" revision="53b4bf58d351b1a5a45eb176565b04750307b560"
Merge branch 't3831' into next: Fix KeyError exception after exiting a process.

Incubates #3831.

@nvaccessAuto

Comment 48 by James Teh <jamie@... on 2015-07-20 07:40
In [f0a4cb5]:
```CommitTicketReference repository="" revision="f0a4cb5db7cdceae025a547b15402794dd77f7ab"
When an application is responding slowly and you switch away from that application, NVDA is now much more responsive in other applications in most cases.

This is done by filtering out events from background windows in most cases to avoid freezes when a background app is unresponsive. One hard-coded exception is when background progress bar reporting is enabled, in which case background valueChange events are allowed.
To facilitate this, eventHandler.shouldAcceptEvent was introduced.
eventHandler.requestEvents was also added to request particular events that are blocked by default; e.g. show events from a specific control or certain events even when in the background.
Fixes #3831.

Changes:
Removed labels: incubating
State: closed
@nvaccessAuto

Comment 49 by jteh on 2015-07-20 07:43
Changes:
Milestone changed from next to 2015.3

@jcsteh jcsteh was assigned by nvaccessAuto Nov 10, 2015
@nvaccessAuto nvaccessAuto added this to the 2015.3 milestone Nov 10, 2015
@jcsteh jcsteh added a commit that referenced this issue Nov 23, 2015
@jcsteh jcsteh When an application is responding slowly and you switch away from tha…
…t application, NVDA is now much more responsive in other applications in most cases.

This is done by filtering out events from background windows in most cases to avoid freezes when a background app is unresponsive. One hard-coded exception is when background progress bar reporting is enabled, in which case background valueChange events are allowed.
To facilitate this, eventHandler.shouldAcceptEvent was introduced.
eventHandler.requestEvents was also added to request particular events that are blocked by default; e.g. show events from a specific control or certain events even when in the background.
Fixes #3831.
f0a4cb5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment