Attempt to fix a panic in accesskit

[ogoffart]

On unix, the panic happens since the upgrade from zbus 3.10 to zbus 3.14

thread 'test_window_accessor' panicked at 'called `Option::unwrap()` on a `None` value', /home/olivier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_consumer-0.15.0/src/tree.rs:274:45
stack backtrace: [...]
   3: core::option::Option<T>::unwrap
   4: accesskit_consumer::tree::Tree::new
             at /home/olivier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_consumer-0.15.0/src/tree.rs:274:19
   5: accesskit_unix::adapter::Adapter::new
             at /home/olivier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_unix-0.5.0/src/adapter.rs:53:20
   6: accesskit_winit::platform_impl::platform::Adapter::new
             at /home/olivier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_winit-0.14.1/src/platform_impl/unix.rs:21:23
   7: accesskit_winit::Adapter::with_action_handler
             at /home/olivier/.cargo/registry/src/index.crates.io-6f17d22bba15001f/accesskit_winit-0.14.1/src/lib.rs:170:23
   8: i_slint_backend_winit::accesskit::AccessKitAdapter::new
             at ./accesskit.rs:62:20
   9: i_slint_backend_winit::winitwindowadapter::WinitWindowAdapter::new::{{closure}}
             at ./winitwindowadapter.rs:163:32
  10: alloc::rc::Rc<T>::new_cyclic
             at /rustc/90c541806f23a127002de5b4038be731ba1458ca/library/alloc/src/rc.rs:461:20
  11: i_slint_backend_winit::winitwindowadapter::WinitWindowAdapter::new
             at ./winitwindowadapter.rs:148:23
  12: i_slint_backend_winit::window_factory_fn
             at ./lib.rs:71:5
  13: <i_slint_backend_winit::Backend as i_slint_core::platform::Platform>::create_window_adapter
             at ./lib.rs:220:9

The problem is that we get called to create the tree directly from WinitWindowAdapter::new before the WinitWindowAdaptor is initialized, So of course, the upgrade from weak is still the pseudo-weak from Rc::new_cyclic, so the upgrade fails, resulting in a default constructed TreeUpdate which is invalid for the first call as it doesn't have a tree.

Just give it a tree with a dummy id

[ogoffart]

Please review carefully, I have no idea what i'm doing.
CC @mwcampbell
Note that I do not have any accessibility backend configured. So it is strange that AccessKit tries to query the tree in that case.

[mwcampbell]

I see this and will look into it either this evening or tomorrow, after I return home from a trip.

[tronical]

I wonder how this ever worked, now that I see this.

If connecting to the a11y bus fails, AccessKit returns and never calls the initial tree state function.

That must've happened before, somehow. The fact that it happens for you seems like a bug, unless perhaps you do have an at-spi bus running on your system?

But what's worse is this when there is intentionally a bus running, the current code will indeed always panic - due to the symptoms you described.

On Windows and macOS the callback is invoked later, but AccessKit behaves different on Linux here.

In other words: this never worked :(

This patch works around the panic successfully, but I'm concerned it breaks it breaks the case when an accessibility tree is really wanted: we must make sure to deliver a new and complete tree later - as a workaround for the AccessKit_unix behavior. That workaround is missing in this patch AFAICS.

[tronical]

In a way it would be nice if AccessKit on Linux also called the initial tree callback at a later point, but I can see that this is not documented behavior.

So the only other solution I can see is to delay the creation of the AccessKitAdapter on our end at a later point.

[tronical]

I think this fixes the panic on Linux and it should not impact windows and macOS as there by the time it is invoked the window adapter upgrade should work.

When accessibility on Linux is really needed, this won’t panic anymore but I think the initial tree will be completely wrong unless an update replaces it. But that’s not worse than panicking as before.

[ogoffart]

unless perhaps you do have an at-spi bus running on your system?

I don't.

It started failing when upgrading from to zbus 3.14 (It was using zbus 3.10 before (that's what in the Cargo.lock for the 1.1.0 release for C++). intermediate version of zbus are not selected by Cargo because zbus was pinning a dependency in a way that would make the dependency resolution fails)

IMHO AccessKit should not query the tree unless there is an actual at-spi bus active. Otherwise we pay the overhead for nothing. (In light of that, i wonder if we should really have the feature on by default)

[mwcampbell]

Please do not disable the feature by default. I was willing to allow the Unix adapter in particular to be disabled by default in the Bevy game engine due to that adapter's impact on code size and dependency count, because for the most part, games will not be automatically accessible anyway. But in a general-purpose GUI toolkit, accessibility should be enabled by default, so we don't have to beg every application developer to turn it on. We do try to minimize the overhead of AccessKit when accessibility is not being used, but in this case, it does look like @ogoffart must be running an AT-SPI bus.

Creating a dummy tree if the callback is called immediately is fine, as long as it's guaranteed that a tree update with a real tree will be issued very soon afterward. I don't know enough about slint to know if that's the case. Please check this.

Finally, the dummy tree update should include an entry for the dummy node with ID 1 in the TreeUpdate::nodes field. I'd just make it a node with role Window and no other properties. As it is now, you're creating a tree with a non-existent root node, and that's technically invalid.

[ogoffart]

I was willing to allow the Unix adapter in particular to be disabled by default in the Bevy game engine due to that adapter's impact on code size and dependency count, because for the most part, games will not be automatically accessible anyway. But in a general-purpose GUI toolkit, accessibility should be enabled by default, so we don't have to beg every application developer to turn it on.

Yeah, that's a valid point. But it's also not enabled by default in egui.

it does look like @ogoffart must be running an AT-SPI bus.

I am not. (at least i didn't setup one, i don't think it's the case)

Perhaps it is a bug in zbus that do not report errors while it should?
(since this panic appeared with an update of zbus)

We do try to minimize the overhead of AccessKit when accessibility is not being used,

Also, it appear like accesskit will do blocking dbus when the window is shown. (if i'm not mistaken) Perhaps that's not critical, but that's also not ideal. With winit, it is possible to write a Waker that calls winit::event_loop::EventLoopProxy.
Or perhaps Accesskit should offer async API.
But anyway, here is probably not the place to discuss this.

Finally, the dummy tree update should include an entry for the dummy node with ID 1 in the TreeUpdate::nodes field. I'd just make it a node with role Window and no other properties. As it is now, you're creating a tree with a non-existent root node, and that's technically invalid.

yeah, do we really need to have a valid tree at start? Could accesskit not allow None as a tree, and we would call an update with a new tree later when we have one?

[mwcampbell]

It's true that the base egui crate has AccessKit disabled by default, and I accepted that because in many projects that use egui, particularly in many projects with custom egui integrations, accessibility is likely to be impractical anyway. However, in the higher-level eframe crate, AccessKit is enabled by default. I believe that slint is more like eframe than the base egui crate.

Having an initial tree with a dummy root node is the accepted workaround for cases where the callback is called too early. It's what I did in eframe. Please add the dummy root node. I'm not sure it would be a good idea to complicate AccessKit itself to deal with this situation. Also, I need assurance that a tree update with a real, non-empty tree is guaranteed to be issued soon after the initial dummy tree.

[mwcampbell]

@ogoffart I recognize that this PR is probably not the right place to discuss the unexpected fact that AccessKit is active on your machine even though you're not running an assistive technology, and to decide what we're going to do about it. But we have to discuss it somewhere, and I can't accept turning AccessKit off by default. What do you propose? Do you want to start a discussion thread or open an issue?

[mwcampbell]

I'm quite sure that the AccessKit Unix adapter will not call the tree source callback unless the adapter successfully connects to a running AT-SPi bus. @ogoffart Can you please check whether at-spi2-registryd is running on your machine even though you're not running any assistive technology? If so, we should debug that problem elsewhere, and determine whether it's a common condition on most users' machines, or whether your configuration is unusual, e.g. because you did test with an assistive technology at some point.

[ogoffart]

Yes you're actually right, i have at-spi2-registryd running.
In fact, running a Slint app does start it. (There is a file in /usr/share/dbus-1/services/org.a11y.Bus.service that will make dbus automatically start the at-spi things when Slint queries for it via accesskit)

[mwcampbell]

OK, now I'm wondering if AccessKit's current behavior is wrong. Maybe the socket-activated startup of the AT-SPi registry was only intended to allow assistive technologies to lazily activate the registry, not applications. @federicomenaquintero Can you please clarify how this is supposed to work? If AccessKit's behavior is wrong, then I'll open an AccessKit issue and get it fixed as quickly as possible.

[federicomenaquintero]

Applications are meant to do this:

1. Connect to the user's session bus as usual.
2. Find the address of the accessibility bus (i.e. the dbus-daemon used exclusively for accessibility, not the normal user's session dbus) by calling GetAddress on the org.a11y.Bus interface, /org/a11y/bus object path, org.a11y.Bus service name.
3. Open a bus connection to that address.
4. On the accessibility bus, register with the atspi registry by calling the Embed method on the org.a11y.atspi.Socket interface, /org/a11y/atspi/accessible/root object path, org.a11y.atspi.Registry service name.

Step (2) will launch the accessibility bus on demand. Step (4) will launch the atspi registry daemon on demand.

If you want to play with things by hand, you can accomplish step 2 by running this on the command line:

# gdbus call --session --dest org.a11y.Bus --object-path /org/a11y/bus --method org.a11y.Bus.GetAddress
('unix:path=/run/user/1000/at-spi/bus,guid=c661d9fe1de4614270ec29c964a34109',)

If you then look at a process listing, you should see something like this:

federico  3183  0.0  0.0   8112  3840 ?        S    Jul03   0:00 /usr/bin/dbus-daemon --config-file=/usr/share/defaults/at-spi2/accessibility.conf --nofork --print-address 11 --address=unix:path=/run/user/1000/at-spi/bus

Note that that is not the usual dbus-daemon --session bus, it's the accessibility bus that runs separately.

You will also see a /usr/libexec/at-spi2/at-spi-bus-launcher process. What happens is this:

1. When you call GetAddress on the org.a11y.Bus service, that service name is set up to launch at-spi2-bus-launcher. That is a tiny daemon that runs a secondary dbus-daemon or dbus-broker (depending on your distro), which becomes the "accessibility bus". The tiny daemon is the one that replies with the GetAddress result, which is the unix:path gunk above.
2. When you call Embed on the Socket interface in the org.a11y.atspi.Registry interface via the accessibility bus, that is set up to launch at-spi2-registryd. The registry starts up if it hasn't already, registers your app, and starts the atspi protocol via dbus.

The lifetime of those three daemons (at-spi-bus-launcher, the accessibility dbus daemon, and at-spi2-registryd) is controlled by the user's session, that is, they use the gnome-session dbus protocol to be notified when the session is terminating and they should exit. Please see this script from at-spi2-core's CI for inspiration; it uses a mock gnome-session, but the gdbus calls may be useful if you want to simulate things by hand.

[ogoffart]

I'm going to go ahead and merge this for now because more and more users are reporting the panic.
(We're going to make a release early next week)