1896816 - Crash in [@ mozilla::StaticPrefs::accessibility_uia_enable] caused by JAWS access from wrong thread

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Spun off bug 1871989 comment 9 onwards.

Crash report: https://crash-stats.mozilla.org/report/index/e3d014c6-4960-4b7d-905f-4fb810240429

MOZ_CRASH Reason: MOZ_DIAGNOSTIC_ASSERT(IsAtomic<bool>::value || NS_IsMainThread()) (Non-atomic static pref 'accessibility.uia.enable' being accessed on background thread by getter)

Top 10 frames:

0  xul.dll  mozilla::StaticPrefs::accessibility_uia_enable()  modules/libpref/init/StaticPrefList_accessibility.h:45
1  xul.dll  mozilla::a11y::MsaaAccessible::QueryInterface(_GUID const&, void**)  accessible/windows/msaa/MsaaAccessible.cpp:526
2  oleacc.dll  oleacc.dll@0x68ab
3  oleacc.dll  oleacc.dll@0xadf1
4  oleacc.dll  oleacc.dll@0xae7e
5  oleacc.dll  oleacc.dll@0x5992
6  xul.dll  mozilla::a11y::MsaaAccessible::QueryInterface(_GUID const&, void**)  accessible/windows/msaa/MsaaAccessible.cpp:522
7  oleacc.dll  oleacc.dll@0xa7b5
8  FSDomNodeIAText.DLL  FSDomNodeIAText.DLL@0x24176
9  oleacc.dll  oleacc.dll@0x664f

A client is never supposed to access Gecko UI from any thread other than the main thread. However, it looks like we bring up file picker dialogs in a background thread, and then JAWS (for some strange and unknown reason) tries to traverse into the (disabled) Gecko UI. Even if we make the UIA pref work on a background thread, we're going to hit all sorts of other problems if this happens.

Even if it traversed the tree, JAWS shouldn't be directly calling methods on Gecko objects from this thread. It should have used WM_GETOBJECT and ObjectFromLresult one way or another, which means they should be marshaled across threads by the COM marshaler. I can't be absolutely certain but this smells very much like a JAWS bug.

I could try asking Vispero to fix this, but I don't love our chances of a quick fix. In addition, because of the cost of updates, many JAWS users tend to be running old versions, so we'd still be dealing with that problem.

Comment 1

[James Teh [:Jamie]]

Attachment: Bug 1896816: Fail gracefully if a naughty client tries to QueryInterface on an MsaaAccessible from the wrong thread.

JAWS apparently does this sometimes.
In particular, this was causing a crash when we tried to check the UIA pref because that pref can only be accessed from the main thread.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1881190

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84c756f39b46
Fail gracefully if a naughty client tries to QueryInterface on an MsaaAccessible from the wrong thread. r=eeejay

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/84c756f39b46

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox127 to wontfix.

For more information, please visit BugBot documentation.

Comment 6

[Donal Meehan [:dmeehan]]

:jamie, to add to Comment 11.
I'm wondering about a release uplift too? Though it doesn't seem like this is being hit by users in Fx126 release.

Comment 7

[James Teh [:Jamie]]

Hmm. I thought this was only being hit by Thunderbird users, but the crash stats show Firefox users are hitting it as well. It probably wouldn't hurt to uplift to release if we spin a dot release for other reasons.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1896816: Fail gracefully if a naughty client tries to QueryInterface on an MsaaAccessible from the wrong thread.

JAWS apparently does this sometimes.
In particular, this was causing a crash when we tried to check the UIA pref because that pref can only be accessed from the main thread.

Original Revision: https://phabricator.services.mozilla.com/D210417

Comment 9

[James Teh [:Jamie]]

Oh, I guess we aren't hitting it on release because it's a MOZ_DIAGNOSTIC_ASSERT, which don't trigger on release. However, that could be hiding problems further down the line... though even with this patch, there might be problems further down the line. It's probably still worth an uplift though.

Comment 10

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Crashes in some cases when JAWS screen reader users open file picker dialogs.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Simply adds an early return for a failure case that would otherwise cause a crash.
• String changes made/needed: none
• Is Android affected?: no

Comment 11

[James Teh [:Jamie]]

Attachment: Bug 1896816: Fail gracefully if a naughty client tries to QueryInterface on an MsaaAccessible from the wrong thread.

JAWS apparently does this sometimes.
In particular, this was causing a crash when we tried to check the UIA pref because that pref can only be accessed from the main thread.

Original Revision: https://phabricator.services.mozilla.com/D210417

Comment 12

[Phabricator Automation]

release Uplift Approval Request

• User impact if declined: Crashes in some cases when JAWS screen reader users open file picker dialogs.
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: not applicable
• Risk associated with taking this patch: low
• Explanation of risk level: Simply adds an early return for a failure case that would otherwise cause a crash.
• String changes made/needed: none
• Is Android affected?: no

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4d0175594418

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/acddfd908c81