1807639 - Android crash in [@ mozilla::a11y::Accessible::AsRemote] in mozilla::a11y::PivotRadioNameRule::Match

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Chris Peterson [:cpeterson]]

Crash report: https://crash-stats.mozilla.org/report/index/b9d9d837-baa1-4953-8fdc-fff9c0221226

The earliest crash reports are from Fenix Nightly 108.0a1. There are about a dozen crash reports from desktop 106 with the same signature, but the rest of their stack are different.

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0  libxul.so  mozilla::a11y::Accessible::AsRemote  accessible/ipc/other/RemoteAccessible.h:101
0  libxul.so  mozilla::a11y::PivotRadioNameRule::Match  accessible/base/Pivot.cpp:655
1  libxul.so  mozilla::a11y::Pivot::AdjustStartPosition  accessible/base/Pivot.cpp:35
2  libxul.so  mozilla::a11y::Pivot::SearchForward  accessible/base/Pivot.cpp:118
3  libxul.so  mozilla::a11y::RemoteAccessibleBase<mozilla::a11y::RemoteAccessible>::RelationByType const  accessible/ipc/RemoteAccessibleBase.cpp:758
4  libxul.so  mozilla::a11y::RemoteAccessible::RelationByType const  accessible/ipc/other/RemoteAccessible.cpp:82
5  libxul.so  mozilla::a11y::RemoteAccessibleBase<mozilla::a11y::RemoteAccessible>::GetPositionAndSetSize  accessible/ipc/RemoteAccessibleBase.cpp:1507
6  libxul.so  mozilla::a11y::Accessible::GroupPosition  accessible/basetypes/Accessible.cpp:216
7  libxul.so  mozilla::a11y::RemoteAccessible::GroupPosition  accessible/ipc/other/RemoteAccessible.cpp:138
8  libxul.so  mozilla::a11y::RemoteAccessibleBase<mozilla::a11y::RemoteAccessible>::Attributes  accessible/ipc/RemoteAccessibleBase.cpp:1181

Comment 1

[Ryan VanderMeulen [:RyanVM]]

The first reports from Fenix Nightly came in around early November. I see a number of changes to RemoteAccessibleBase.cpp around that time, but the crash frequency is low enough that it'll be difficult to pin down a more precise regression range. Morgan, does anything stand out to you from around that time?

Comment 2

[Morgan Reschenberg [:morgan]]

Hmm, my guess is this is related to bug 1787284 based on the call stack and assuming this is from an android build with CtW enabled.

Comment 3

[Morgan Reschenberg [:morgan]]

Hmm okay, so it looks like this happens because aAcc is null here, which happens if the anchor we pass here is null, which happens when either the acc we pass in, or the root the pivot was created with is null.

And that should only happen if we somehow traverse the ancestor chain, from a radio button, without hitting the button's document...?

I don't know how that could happen... Looking at the RemoteParent code we only return null (when this is not a document) when we can't find the given ID in the doc's ID map.

Is that a known problem on android?
I can add an if to make sure we don't create the MEMBER_OF pivot with a null ancestor, but I wonder if this is pointing at a larger issue.

Comment 4

[Eitan Isaacson [:eeejay]]

I support adding an if there with an optional assert. You can't ever assume accessibles are parented anyway but I agree that this is weird.

Comment 5

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1807639: Null check `ancestor` before creating a Pivot r?eeejay

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32f37b8b54ab
Null check `ancestor` before creating a Pivot r=eeejay

Comment 7

[Morgan Reschenberg [:morgan]]

Comment on attachment 9310597 [details]
Bug 1807639: Null check ancestor before creating a Pivot r?eeejay

Beta/Release Uplift Approval Request

• User impact if declined: Users will continue to experience this crash
• 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): This patch adds a null check.
  There is a very short period of time in the document lifecycle where it's possible to get into the situation that caused this crash (when accs are being moved). Even so, this null check prevents us from attempting to work with accs in that state. This only affects users with "cache the world" enabled.
• String changes made/needed:
• Is Android affected?: Yes

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/32f37b8b54ab

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9310597 [details]
Bug 1807639: Null check ancestor before creating a Pivot r?eeejay

Approved for Desktop 109.0b9 & Fenix 109.0b6.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a8d7c16e9f08