Open Bug 1748450 Opened 3 years ago Updated 3 years ago

Grabbing focus and/or setting caret should either not cause object destruction or should do it more quickly

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

REOPENED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: jdiggs, Assigned: Jamie, NeedInfo)

References

Details

Attachments

(2 files)

Attached file link-in-heading.py (deleted) —

Steps to reproduce:

  1. Launch the attached at-spi2 accessible-event listener in a terminal
  2. Load and reload https://docs.microsoft.com/en-us/dotnet/standard/async-in-depth, important: focus must be in the document and NOT in the location bar

Expected results: Either no objects would reported as defunct by the listener (preferred), or all objects would be.

Actual results: Some objects report being defunct; others don't. It changes from reload to reload. See sample output at the end of this comment.

Question/Aside: If focus is in the location bar, nothing gets destroyed. It's only when focus is in the document. So is destruction absolutely necessary?

Use case/scenario: Orca's structural navigation commands are by side effect triggering objects on (at least) the site in question to be destroyed and recreated. More specifically, as the user moves by heading, Orca sets the caret in that heading. For focusable objects like links, navigation causes Orca to do a focus grab. If the object gets destroyed before Orca can present it to the user, presentation is broken.

(Related aside: Orca moves the caret and/or grabs focus because it doesn't have a virtual buffer and relies upon the browser's native functionality for things like text selection, link activation, etc. In other words, Firefox needs to know where the caret and focus are prior to the user doing Shift+Arrows or Return.)

I have just committed a workaround in Orca master to deal with this: I save the path within the accessibility tree, update the location (set caret, grab focus, etc.), check for object destruction and then use the path to get the replacement object from AT-SPI2 if need be. That brings me to the following:

In order for this workaround to work, AT-SPI2 must have already received the children-changed event from Gecko. That event causes AT-SPI2 to update its accessibility tree and give Orca the right object. I am seeing instances where the event has not yet been fired/received at the moment Orca seeks the replacement object. When that happens, the user experience is still broken. So.... If these objects really do need to be destroyed, could that destruction and recreation be prioritized? smiles

Sample output:
==============
7 matching headings found
Heading is now defunct. Old name: ' What does this mean for a server scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 27]
Link is now defunct. Old name: ' What does this mean for a server scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, -1, -1]
2 objects out of 14 are now defunct.
==============
7 matching headings found
Heading is now defunct. Old name: ' What does this mean for a server scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 27]
Link is now defunct. Old name: ' What does this mean for a server scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 27, 0]
2 objects out of 14 are now defunct.
==============
7 matching headings found
Heading is now defunct. Old name: ' Deeper Dive into Tasks for an I/O-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 13]
Link is now defunct. Old name: ' Deeper Dive into Tasks for an I/O-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 12, 0]
Heading is now defunct. Old name: ' What does this mean for client scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 33]
Link is now defunct. Old name: ' What does this mean for client scenario?' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 33, 0]
Heading is now defunct. Old name: ' Deeper Dive into Task and Task<T> for a CPU-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 37]
Link is now defunct. Old name: ' Deeper Dive into Task and Task<T> for a CPU-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 36, 0]
6 objects out of 14 are now defunct.
==============
7 matching headings found
Link is now defunct. Old name: ' Deeper Dive into Tasks for an I/O-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 13, 0]
Heading is now defunct. Old name: ' Deeper Dive into Task and Task<T> for a CPU-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 37]
Link is now defunct. Old name: ' Deeper Dive into Task and Task<T> for a CPU-Bound Operation' New name: '' Path: [-1, 0, 16, 0, 0, 0, 7, 0, 0, 37, 0]
3 objects out of 14 are now defunct.

I'm wondering whether I fixed this in bug 1739050. However, that is only enabled for our new caching architecture which is still heavily under development.

Would you mind testing this with latest Nightly with accessibility.cache.enabled set to true in about:config? Please note that other things might break with that pref enabled, but I'm hoping it works enough for you to be able to test this.

Flags: needinfo?(jdiggs)

Huh. It seems to -- after I restarted Firefox. I didn't see any indication that this was necessary, but did so after the problem persisted. Cool.

Flags: needinfo?(jdiggs)

My bad. You do indeed need to restart after toggling that pref. I should have mentioned that.

The bad news is that we're many months away from being able to turn on the new architecture. :(

Eitan, do you think it's reasonable to do the RemoteAccessible move stuff from bug 1739050 regardless of the cache pref? It doesn't depend on any of the caching architecture. I just put it behind that pref because it's a bit risky and I figured we might want to test it alongside the rest of the cache. However, it fixes a pretty nasty issue for Orca, so that seems like a reasonable justification to try having it always enabled.

Flags: needinfo?(eitan)

Yeah, I think it makes sense to not have this depend on cache.

Flags: needinfo?(eitan)

This fixes problems for Orca caused by object destruction when grabbing focus or setting the caret.

This also fixes an oversight where moved LocalAccessibles were being tracked even in the parent process if the cache was enabled.
While that was harmless, it was also unnecessary, since we only need to do that in order to send IPC notifications.

Assignee: nobody → jteh
Status: NEW → ASSIGNED
Depends on: 1739050
Severity: -- → S3
Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/52345a440234 If an Accessible is moved, reuse the RemoteAccessible even if the cache is disabled. r=eeejay
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Regressions: 1751268

Backed out changeset 52345a440234 (Bug 1748450) for causing crashes (Bug 1751268).
Backout link: https://hg.mozilla.org/integration/autoland/rev/6d69e93c177218faa4783a018ee7c215830c7824

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 98 Branch → ---

Backout has been grafted to central

Pushed by jteh@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e13d8a7199f If an Accessible is moved, reuse the RemoteAccessible even if the cache is disabled. r=eeejay
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch

Jamie: I'm still seeing this issue using Firefox Nightly and the original steps to reproduce the problem using the pyatspi listener. Am I missing something or should we re-open this bug?

Flags: needinfo?(jteh)

That is... concerning. If you flip accessibility.cache.enabled to true and restart, does that (still) fix the problem?

Flags: needinfo?(jteh)

TL;DR: Yup. Still fixes the problem.

Here is Firefox nightly with cache enabled and restarted, using the steps in the opening report:

==============
7 matching headings found
0 objects out of 14 are now defunct.
==============
7 matching headings found
0 objects out of 14 are now defunct.
==============
7 matching headings found
0 objects out of 14 are now defunct.

And here is the same test done after restoring the setting to the default false and restarting:

7 matching headings found
Heading is now defunct. Old name: ' Server' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 26]
Link is now defunct. Old name: ' Server' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, -1, -1]
2 objects out of 14 are now defunct.
==============
7 matching headings found
Link is now defunct. Old name: ' Tasks for an I/O-bound operation' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 12, 0]
Link is now defunct. Old name: ' Server' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 26, 0]
Link is now defunct. Old name: ' Why does async help here?' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 44, 0]
3 objects out of 14 are now defunct.
==============
7 matching headings found
Link is now defunct. Old name: ' Tasks for an I/O-bound operation' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 12, 0]
Heading is now defunct. Old name: ' Server' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, 26]
Link is now defunct. Old name: ' Server' New name: '' Path: [-1, 0, 15, 0, 0, 0, 6, 0, 0, -1, -1]
3 objects out of 14 are now defunct.

I did some investigation here. I think accessibility.cache.enabled "fixing" this is a red herring. With the cache enabled, I still see these objects die and get re-created. I think it's just that the timing is different. With the cache disabled, ATK calls are synchronously dispatched to the content process, so they always get the latest state. With the cache enabled, the cache push is async, so there might be a slight delay while that happens. My guess is that if you put a time.sleep after the focus call, the object will go defunct.

Of course, none of that is particularly helpful. I still don't quite understand why re-creation is happening in this case. Bug 686400 was supposed to deal with this kind of nonsense (and it normally does).

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

I see the same behaviour in Chromium. Do you?

Note that Chromium's behaviour is more like what you get with accessibility.cache.enabled in Firefox, so my comment 16 re timing probably applies; i.e. if you delay slightly, I believe you will see defuncts.

Flags: needinfo?(jdiggs)
Regressions: 1759600
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: