1691930 - Crash in [@ mozilla::a11y::DocAccessibleChild::RecvFocusedChild]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Simon Giesecke [:sg] [he/him]]

Crash report: https://crash-stats.mozilla.org/report/index/f8338216-b611-40d7-a922-66bfe0210210

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 XUL mozilla::a11y::DocAccessibleChild::RecvFocusedChild accessible/ipc/other/DocAccessibleChild.cpp:1540
1 XUL mozilla::a11y::PDocAccessibleChild::OnMessageReceived ipc/ipdl/PDocAccessibleChild.cpp:7550
2 XUL mozilla::dom::PContentChild::OnMessageReceived ipc/ipdl/PContentChild.cpp:16282
3 XUL mozilla::dom::ContentChild::OnMessageReceived dom/ipc/ContentChild.cpp:3603
4 XUL mozilla::ipc::MessageChannel::DispatchMessage ipc/glue/MessageChannel.cpp:2073
5 XUL mozilla::ipc::MessageChannel::MessageTask::Run ipc/glue/MessageChannel.cpp:1956
6 XUL mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal xpcom/threads/TaskController.cpp:753
7 XUL mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal xpcom/threads/nsThreadUtils.h:534
8 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1158
9 XUL mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87

Maybe result->Document() needs to be checked for being non-nullptr?

Comment 1

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

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

Comment 2

[James Teh [:Jamie]]

If result->Document() is indeed null, it's pretty worrying that FocusManager returns an unbound (and presumably defunct) Accessible...

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Crash volume here looks a bit worrying to me. Can we bump the priority?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #2)

If result->Document() is indeed null, it's pretty worrying that FocusManager returns an unbound (and presumably defunct) Accessible...

Seems possible to me since FocusManager holds strong references to accessibles that might have become unbound in the meantime.

Comment 5

[Eitan Isaacson [:eeejay]]

Yes, we set mActiveItem to null when it is unbound from doc. But for that to work, mLastFocus needs to equal mActiveItem and the unbinding accessible. mLastFocus is set on a queued notification, so it may not be updated to our accessible and thus mActiveItem might be left with a defunct accessible. Hard to test this theory, because it involves multiple queued notifications mixed with DOM's own focus notifications.

I'm just going to put in a null check here since this is an actual crasher.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Bug 1691930 - Check that FocusedChild is bound to a doc in RecvFocusedChild. r?Jamie

Added an assert in focus manager. Hopefully fuzzers will help us find
cases where the active item is defunct, if that is indeed what is
happening.

Comment 7

[James Teh [:Jamie]]

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

Yes, we set mActiveItem to null when it is unbound from doc. But for that to work, mLastFocus needs to equal mActiveItem and the unbinding accessible. mLastFocus is set on a queued notification,

True, though mActiveItem is also nulled before that notification is queued (in NotifyOfDOMFocus and ActiveItemChanged). Hmm.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2010ac6007c8
Check that FocusedChild is bound to a doc in RecvFocusedChild. r=Jamie

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2010ac6007c8

Comment 10

[Julien Cristau [:jcristau]]

Please nominate this for beta (and maybe release?) approval.

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 9210841 [details]
Bug 1691930 - Check that FocusedChild is bound to a doc in RecvFocusedChild. r?Jamie

Beta/Release Uplift Approval Request

• User impact if declined: 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 is a simple null check that mitigates a known crash.
• String changes made/needed:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9210841 [details]
Bug 1691930 - Check that FocusedChild is bound to a doc in RecvFocusedChild. r?Jamie

Approved for 88.0b3.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9210841 [details]
Bug 1691930 - Check that FocusedChild is bound to a doc in RecvFocusedChild. r?Jamie

88 is in RC now and there are no plans for an 87 dot release.