1791152 - Crash in [@ mozilla::a11y::Accessible::IsLocal]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/08e245ee-3c68-49bb-a309-0c1a60220915

Reason: SIGSEGV / SEGV_MAPERR

Top 10 frames of crashing thread:

0 libxul.so mozilla::a11y::AccessibleWrap::SetVirtualViewID accessible/android/AccessibleWrap.cpp:574
1 libxul.so mozilla::a11y::SessionAccessibility::UnregisterAll accessible/android/SessionAccessibility.cpp:1100
2 libxul.so mozilla::a11y::DocAccessibleWrap::Shutdown accessible/android/DocAccessibleWrap.cpp:57
3 libxul.so mozilla::a11y::DocManager::HandleEvent accessible/base/DocManager.cpp:361
4 libxul.so {virtual override thunk} accessible/base/DocManager.cpp
5 libxul.so mozilla::EventListenerManager::HandleEventSubType dom/events/EventListenerManager.cpp:1316
6 libxul.so mozilla::EventListenerManager::HandleEventInternal dom/events/EventListenerManager.cpp:1506
7 libxul.so mozilla::EventTargetChainItem::HandleEventTargetChain dom/events/EventDispatcher.cpp:480
8 libxul.so mozilla::EventDispatcher::Dispatch dom/events/EventDispatcher.cpp:1119
9 libxul.so mozilla::dom::Document::DispatchPageTransition dom/base/Document.cpp:11530

This is a use-after-free access via the aAccessible pointer here.

Comment 1

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 AArch64 and ARM crashes on nightly

For more information, please visit auto_nag documentation.

Comment 2

[Eitan Isaacson [:eeejay]]

This looks like a fission crash. Once bug 1789474 lands I am curious to see if this persists since there were some doc accessible lifecycle issues that were discovered with Fission.

Comment 3

[James Teh [:Jamie]]

Bug 1793126 is almost certainly a duplicate.

Comment 4

[Maire Reavy [:mreavy]]

Hi Eitan, It looks like the crash is still present (looking at the most recent crash-stats).

Comment 5

[Eitan Isaacson [:eeejay]]

Yeah I first thought this was fission only which is not the case. Trying to reproduce and come up with something.

Comment 6

[Eitan Isaacson [:eeejay]]

While I wasn't able to reproduce anything locally, it does look like we occasionally get into a state where the parent's PresShell is gone before a RemoteAccessible is shut down. This intermittent failure is evidence of that.

That MOZ_ASSERT should arguably be changed to a diagnostic assert since it means we failed to remove a raw pointer reference from SessionAccessibility's table, and we can potentially have a UAF.

My proposed fix is to stop relying on a top-level PresShell for retrieving any given accessible's SessionAccessibility instance, but instead to rely on the parent's document to hold a strong reference to SessionAccessibility. This should help eliminate any shutdown bugs where the PresShell disappears before an accessible or its doc.

I have a higher degree of confidence that an outer doc will be available because there is a debug assert for that as well, and that hasn't been tripped.

Comment 7

[Eitan Isaacson [:eeejay]]

For the record, this is the "regressing" bug in that it enables caching in beta/release: https://bugzilla.mozilla.org/show_bug.cgi?id=1786584

Comment 8

[BugBot [:suhaib / :marco]]

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

Comment 9

[Daniel Veditz [:dveditz]]

That doesn't seem right: bug 1786584 landed in 106, but this signature goes back to 104. The signature change was because Socorro learned how to handle inlined functions.

Comment 10

[Eitan Isaacson [:eeejay]]

Ah.. the regression is for when it started crashing in release. It was enabled in nightly in bug 1758540, but that was 102.. so I am not sure why it started failing in 104. There is probably some other regressing bug somewhere in the middle.

Comment 11

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1791152 - Drop entire accessible ID map in UnregisterAll. r?Jamie

Comment 12

[Eitan Isaacson [:eeejay]]

I failed to reach the root of the matter and figure out in what circumstances a remote doc shuts down after a root presshell. But it definitely happens. Since this is a shut down crash, I think we should just drop the id/accessible table without trying to recover the used IDs and reset the IDs on the accessibles themselves.

This is just a mitigation of this crash. The next steps after this bug are:

1. Remedy bug 1778916 by trial and error (probably by holding a weak reference to SessionAccessibility in DocAccessibleParent).
2. Once that intermittent is gone, change the debug assertion to a diagnostic assertion here

Comment 13

[Eitan Isaacson [:eeejay]]

Comment on attachment 9301216 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll. r?Jamie

Security Approval Request

• How easily could an exploit be constructed based on the patch?: This UAF is hard to reproduce.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: release,beta
• If not all supported branches, which bug introduced the flaw?: Bug 1771934
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Very easy
• How likely is this patch to cause regressions; how much testing does it need?:
• Is Android affected?: Yes

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9301216 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll. r?Jamie

Approved to land and request uplift

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Landed: https://hg.mozilla.org/integration/autoland/rev/1b65ae3b0371c87843b77103856a827d0af99c3b

Backed out for causing android gv-junit-fis failures in SessionAccessibility: https://hg.mozilla.org/integration/autoland/rev/b733f6a8f2f5164a2439a3c75ccce8acb9cabea1

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=retry%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=1b65ae3b0371c87843b77103856a827d0af99c3b&selectedTaskRun=Y-t414XQTJStK_RRN4GYaw.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=395284186&repo=autoland
PROCESS-CRASH | org.mozilla.geckoview.test.AccessibilityTest#testLiveRegion | application crashed [@ mozilla::a11y::SessionAccessibility::UnregisterAccessible(mozilla::a11y::Accessible*)]

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Drop entire accessible ID map in UnregisterAll. r=Jamie
https://hg.mozilla.org/integration/autoland/rev/f449b92ac6006c3abd4e8c41194f4ceaea2be69c
https://hg.mozilla.org/mozilla-central/rev/f449b92ac600

Comment 17

[BugBot [:suhaib / :marco]]

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

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

For more information, please visit auto_nag documentation.

Comment 18

[Eitan Isaacson [:eeejay]]

Curiously this crash doesn't appear in beta, but it does in release. I think this is because this is such a rare crash with fission off that the small beta audience isn't big enough. But it is strange that it is completely absent. Nonetheless, I recommend we uplift it to beta so it will be merged into release later.

Comment 19

[Eitan Isaacson [:eeejay]]

Comment on attachment 9301216 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll. r?Jamie

Beta/Release Uplift Approval Request

• User impact if declined: Rare crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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 removes non-essential code at shutdown.
• String changes made/needed:
• Is Android affected?: Yes

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9301216 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll. r?Jamie

Approved for 107.0RC1

Comment 21

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/566661b8a4fa

Comment 22

[Donal Meehan [:dmeehan]]

Backed out of beta for introducing test failures
https://treeherder.mozilla.org/jobs?repo=mozilla-beta&revision=fe6275ca5dc25cbb4e530920608d1e590934b161&selectedTaskRun=E2rLPLkETueBadOwnjw28A.0

Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/6e19ea4b7cd2fa50866c2d52134c9cf2992f62df

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1791152 - Drop entire accessible ID map in UnregisterAll.

Comment 24

[Eitan Isaacson [:eeejay]]

Comment on attachment 9302367 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll.

Beta/Release Uplift Approval Request

• User impact if declined: Potential shutdown crash
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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 is a simplified shutdown routine
• String changes made/needed:
• Is Android affected?: Yes

Comment 25

[Eitan Isaacson [:eeejay]]

Got a 401 error from bugzilla when trying to fill out the uplift form in phabricator.

Comment 26

[Donal Meehan [:dmeehan]]

Comment on attachment 9302367 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll.

Switching beta uplift request to release, 107 is now in RC.
Keeping on the radar for potential uplift later for a ride-along.

Comment 27

[Donal Meehan [:dmeehan]]

(In reply to Eitan Isaacson [:eeejay] from comment #25)

Got a 401 error from bugzilla when trying to fill out the uplift form in phabricator.

The Phabricator uplift request flow is still in development.
We are still using the Bugzilla flow until the Phabricator uplift flow is ready.

Comment 29

[Donal Meehan [:dmeehan]]

Comment on attachment 9302367 [details]
Bug 1791152 - Drop entire accessible ID map in UnregisterAll.

Approved for 107.0RC2

Comment 30

[Donal Meehan [:dmeehan]]

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