Make accessibility.force_disabled=-1 less platform specific
Categories
(Core :: Disability Access APIs, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox156 | --- | fixed |
People
(Reporter: Jamie, Assigned: Jamie)
References
Details
Attachments
(4 files)
accessibility.force_disabled=-1 is used to force enable accessibility for testing purposes. Currently, it is implemented differently on every platform (and not at all on some platforms). Even though the pref is read centrally in nsAccessibilityService, that code won't be triggered unless GetOrCreateAccService is called from platform specific code. This is very confusing, makes it hard to trust the pref across platforms and makes the code hard to reason about. It wouldn't surprise me if different platforms end up creating the service at different times. Also, I'm fairly sure it just isn't implemented on Android at all.
|
Assignee | |
Updated•20 days ago
|
|
|
Assignee | |
Comment 1•20 days ago
•
|
||
|
||
Comment 2•20 days ago
|
||
On some platforms you can have a11y enabled on a single app and examine it via ATs or other tools. I don't think that is the case in Android, or at least it wasn't in the past. If you try to dispatch an a11y event to the platform it will throw an exception. So the use of forcing a11y on in Android is limited to the in-process piece. That is also how the junit tests work. This means that force_disabled=-1 would mean something different there.
|
|
Assignee | |
Comment 3•19 days ago
|
||
(In reply to Eitan Isaacson [:eeejay] from comment #2)
I don't think that is the case in Android, or at least it wasn't in the past. If you try to dispatch an a11y event to the platform it will throw an exception.
Thanks for the info. I see we already catch that exception to deal with XPCOM instantiation in mochitests. So I don't think there's anything new we'd need to do to handle this.
So the use of forcing a11y on in Android is limited to the in-process piece. ... This means that force_disabled=-1 would mean something different there.
That's true and it's probably worth noting that somewhere. I still think this is useful to have though, particularly when you're trying to reproduce a crash or something locally in a non-a11y test. It seems CI sometimes instantiates accessibility at the OS level, but that seems to be much harder to do on a local machine.
|
|
Assignee | |
Updated•18 days ago
|
|
|
Assignee | |
Comment 4•18 days ago
|
||
accessibility.force_disabled=-1 is meant to force enable accessibility, but previously, nothing acted on it centrally.
GetOrCreateAccService only checked the force disabled case, leaving the force enabled case to platform specific code.
Aside from being hard to reason about, this meant there was different (sometimes broken) behaviour on different platforms.
Windows was the only platform that proactively started accessibility in nsWindow::Create.
On Mac and Linux, the force enabled state allowed accessibility to be enabled where it otherwise wouldn't have been (e.g. no detected client), but it still only activated based on specific platform signals.
Android never read the pref at all.
To fix this, add a11y::MaybeStartForceEnabled, called once from nsAppRunner.cpp just before the main run loop starts.
This only ever runs in the parent process.
Calling PlatformDisabledState() there unconditionally also ensures the pref-change callback gets registered even when nothing else would have triggered that registration.
This required an additional fix on Android.
Previously, SessionAccessibility::Settings unconditionally shut accessibility down when a session's accessibility was first attached to a view with no assistive technology running.
We now prevent that shutdown when force enabled via the pref.
|
|
Assignee | |
Comment 5•18 days ago
|
||
This is now redundant.
a11y::MaybeStartForceEnabled, called centrally from nsAppRunner.cpp, covers this on every platform including Windows.
|
|
Assignee | |
Comment 6•18 days ago
|
||
PrefChanged already force shuts accessibility down when the pref changes to 1.
However, it previously did nothing on a change to -1, so force enabling required a restart.
Give MaybeStartForceEnabled an async flag and call it from PrefChanged with that mode when the pref changes to force enabled.
Async is needed because this callback runs synchronously wherever the pref is set.
We don't want accessibility init happening inside arbitrary code.
Also update the comment describing the pref.
|
|
Assignee | |
Comment 7•13 days ago
|
||
Previously, we didn't unregister from ATK when shutting down.
That was fine because we never supported starting the engine up again after it was shut down, but we soon will.
At that point, failure to unregister from ATK will cause subsequent registration attempts to do nothing, so clients can't find us.
To fix this, call atk_bridge_adaptor_cleanup when shutting down.
|
||
Updated•13 days ago
|
|
||
Comment 10•12 days ago
|
||
Backed out for causing ThreadSanitizer failures
Backout link
Push with failures
Failure log(s)
|
|
Assignee | |
Comment 11•11 days ago
|
||
I confess I don't have much of an understanding of the DBus and ATK code, but this appears to be a bug in DBus. Analysis below (heavily Claude assisted):
The failure was a TSAN lock-order-inversion abort.
TSAN detected a 3-mutex lock-order-inversion cycle (M0 -> M1 -> M2 -> M0) across two threads:
- Audio IPC thread (T97), inside audio_thread_priority::rt_linux::get_limits:
holds M0, acquires M1 inside dbus_bus_register (talking to rtkit over the system bus for realtime scheduling). - Main thread, inside nsAccessibilityService::FullInit -> PlatformInit -> atk_bridge_adaptor_init:
holds M1, acquires M2 inside dbus_connection_send_with_reply_and_block (AT-SPI registration). - Main thread, inside nsAccessibilityService::Shutdown -> PlatformShutdown -> atk_bridge_adaptor_cleanup:
holds M2, acquires M0 while closing the AT-SPI bus connection (dbus_connection_close).
Root cause, traced into libdbus and at-spi2-atk source:
- M0 and M1 are file-scope global mutexes in dbus/dbus-bus.c ("bus" and "bus_datas"), shared by every DBusConnection obtained via dbus_bus_get()/dbus_bus_register(), regardless of bus type or shared vs private connections. internal_bus_get() nests them as bus -> bus_datas -> connection-lock.
- notify_disconnected_unlocked() in dbus-connection.c runs for any connection disconnect and nests the connection's own lock -> bus, the reverse order. This is a genuine inversion inside libdbus itself, not specific to our usage.
- atk_bridge_adaptor_cleanup() (at-spi2-atk bridge.c) calls dbus_connection_close() on the AT-SPI bus connection, triggering the disconnect path above.
Options considered and rejected:
- Use a private D-Bus connection for atk-bridge or audio_thread_priority: does not help. internal_bus_get() takes the "bus" lock unconditionally regardless of the private flag, and any connection close goes through the same disconnect path.
- Patch audio_thread_priority (vendored, Mozilla-authored crate) to avoid dbus_bus_get()/dbus_bus_register() for its rtkit call: technically avoids that thread's contribution to the cycle, but only patches one side and leaves the actual libdbus lock-order defect and our own repeated connection churn in place.
- File upstream with libdbus: likely a legitimate report (two of its own functions nest the same two global locks in opposite order), but not something we can rely on for this bug's timeline.
- Get atk-bridge to expose a lighter "reannounce the app without closing the connection" API: the underlying protocol already separates connection setup from app registration (register_application/deregister_application send plain messages over an already-open connection), but this is not exposed publicly. Only atk_bridge_adaptor_init()/atk_bridge_adaptor_cleanup() are the public entry points, and atk-bridge is a system library loaded via dlopen, not something Firefox can patch or extend.
- TSAN suppression: bug 1824768 already suppresses unrelated libdbus mutex noise from external-library startup issues, a different suppression category to this lock-order-inversion. However, this papers over a real hazard rather than fixing it.
|
|
Assignee | |
Comment 12•11 days ago
|
||
|
||
Comment 13•10 days ago
|
||
|
||
Comment 14•10 days ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/24746728bb6b
https://hg.mozilla.org/mozilla-central/rev/5dbd841c7767
https://hg.mozilla.org/mozilla-central/rev/c0e18acfdcbe
https://hg.mozilla.org/mozilla-central/rev/e59e6960ee85
Description
•