1750550 - Crash in [@ PLDHashTable::Iterator::Iterator | ConvertToAtkTextAttributeSet]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/bbc9def3-fa0f-45a3-8538-02af50220117

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so PLDHashTable::Iterator::Iterator xpcom/ds/PLDHashTable.cpp:755
1 libxul.so ConvertToAtkTextAttributeSet accessible/atk/nsMaiInterfaceText.cpp:74
2 libxul.so getDefaultAttributesCB accessible/atk/nsMaiInterfaceText.cpp:309
3 libatk-bridge-2.0.so.0 impl_GetAttributeRun /usr/src/debug/at-spi2-atk-2.38.0-3.fc35.x86_64/atk-adaptor/adaptors/text-adaptor.c:796
4 libatk-bridge-2.0.so.0 handle_message /usr/src/debug/at-spi2-atk-2.38.0-3.fc35.x86_64/droute/droute.c:601
5 libdbus-1.so.3 dbus_connection_dispatch /usr/src/debug/dbus-1.12.20-5.fc35.x86_64/dbus/dbus-connection.c:4576
6 libatspi.so.0 message_queue_dispatch /usr/src/debug/at-spi2-core-2.42.0-1.fc35.x86_64/atspi/atspi-gmain.c:89
7 libglib-2.0.so.0 g_main_context_dispatch /usr/src/debug/glib2-2.70.2-1.fc35.x86_64/glib/gmain.c:4099
8 libglib-2.0.so.0 g_main_context_iterate.constprop.0 /usr/src/debug/glib2-2.70.2-1.fc35.x86_64/glib/gmain.c:4175
9 libglib-2.0.so.0 g_main_loop_run /usr/src/debug/glib2-2.70.2-1.fc35.x86_64/glib/gmain.c:4373

Comment 1

[Joanmarie Diggs]

I don't see the steps to reproduce in the crash report. So just in case, here they are again:

1. Launch Orca master*
2. Load app.element.io and log in
3. Press control+home and press Down Arrow until focus is in a search entry (for me it's "Filter all spaces")

Firefox crashes for me reliably.

*Orca master now queries the AtkText attributes during navigation. That change in Orca appears to be what is triggering this crash.

Comment 2

[James Teh [:Jamie]]

Just to double check, do you have accessibility.cache.enabled set to false (the default)? I know we were playing with that for a previous bug.

Assuming it's false:

1. This suggests RemoteAccessible::DefaultTextAttributes is returning nullptr.
2. HyperTextAccessible::DefaultTextAttributes can't return nullptr.
3. DocAccessibleChild::RecvDefaultTextAttributes can return nullptr if the id does not exist.
4. Given that, the only way I think this could happen is if an Accessible died in the content process before DefaultTextAttributes was called (and before that RemoteAccessible was removed in the parent process).

I originally wondered if this might be a regression from bug 1730096, but I can't see any change there that would cause RemoteAccessible::DefaultTextAttributes to return nullptr where it didn't before.

Comment 3

[Joanmarie Diggs]

accessibility.cache.enabled is false. Plus this issue was pointed out to me by a user who I assume hasn't changed the setting. And I don't know if this is a regression in Gecko or not. It could be a long-standing issue that I'm only just now triggering via Orca. Any chance of a speculative fix that I could test or do you need more info from me?

Comment 4

[James Teh [:Jamie]]

No, that's all the info I need, thank you. I just wanted to make sure that pref wasn't enabled because attributes calls take a completely different code path when that pref is true.

Comment 5

[James Teh [:Jamie]]

Attachment: Bug 1750550: Null check aAttributes in ConvertToAtkTextAttributeSet.

Comment 6

[James Teh [:Jamie]]

I'm pretty sure this should fix it, but here's a try build if you wanted to take it for a spin.

Comment 7

[Joanmarie Diggs]

That solves the crash, thanks! Any chance that could be backported to existing stable branches (included ESR if appropriate)? Before too long I'm going to do an Orca release and worry about breaking the universe. (Oops!) grins

On a related note: While this change addresses the crash, Orca's debugging output still suggests something bad is happening with an element on that page (aka item 4 from comment 2). Any guesses or do I need to try to create a minimal test case?

Comment 8

[James Teh [:Jamie]]

(In reply to Joanmarie Diggs from comment #7)

That solves the crash, thanks! Any chance that could be backported to existing stable branches (included ESR if appropriate)?

I'll see what I can do. I'm fairly certain I can get it backported to beta, but ESR 91 might be a harder sell.

On a related note: While this change addresses the crash, Orca's debugging output still suggests something bad is happening with an element on that page (aka item 4 from comment 2). Any guesses or do I need to try to create a minimal test case?

My guess is that the act of focusing (or scrolling to) the search entry deliberately causes a node to be removed from the DOM. IN that case, there's a race condition: Orca might (sync) query text attributes before Firefox has (async) notified the parent process that the Accessible died.

Even if this wasn't a race between our parent and content processes, I still think there'd be a possible race between Orca and Firefox: Orca could still try to query an Accessible which died just before the query could be answered.

What does your debug logging show that concerns you?

As to exactly what node we're talking about here, I see there's a div showing the shortcut key (Ctrl K), but that div gets removed (or maybe just hidden, but either way removed from the a11y tree) when the entry gets focus. Perhaps that could be the Accessible in question?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12322ebeb764
Null check aAttributes in ConvertToAtkTextAttributeSet. r=eeejay

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/12322ebeb764

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Adding a null check doesn't sound scary. Feel free to nominate for Beta/ESR uplift.

Comment 12

[James Teh [:Jamie]]

Comment on attachment 9259637 [details]
Bug 1750550: Null check aAttributes in ConvertToAtkTextAttributeSet.

Beta/Release Uplift Approval Request

• User impact if declined: Crashes for Orca screen reader users.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple null check.
• String changes made/needed:

Comment 13

[James Teh [:Jamie]]

Comment on attachment 9259637 [details]
Bug 1750550: Null check aAttributes in ConvertToAtkTextAttributeSet.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crashes.
• User impact if declined: Crashes for Orca screen reader users.
• Fix Landed on Version: 98, requested uplift to 97
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple null check.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9259637 [details]
Bug 1750550: Null check aAttributes in ConvertToAtkTextAttributeSet.

Trivial crash fix. Approved for 97.0b9 and 91.6esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/22c97455454b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/2b9c35892ea9