1597043 - Crash in [@ mozilla::a11y::RootAccessible::ProcessDOMEvent]

Status: RESOLVED FIXED

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

Description

[Wayne Mery (:wsmwk)]

#122 for Firefox 70.0.1. #27 crash for Thunderbird 68.2.2. The crash rate is trivial before Thunderbird version 68, which makes one wonder whether Bug 1502061 - Crash in mozilla::a11y::RootAccessible::ProcessDOMEvent, fixed for v65, made this better or worse for Thunderbird. Perhaps we have a regression? (the author of that patch isn't around)

bp-b5285f48-9aaa-46cb-99d7-a02670191116 Thunderbird
0 xul.dll mozilla::a11y::RootAccessible::ProcessDOMEvent accessible/generic/RootAccessible.cpp:351
1 xul.dll mozilla::a11y::TNotification<mozilla::a11y::RootAccessible, mozilla::dom::Event, nsINode>::Process accessible/base/NotificationController.h:72
2 xul.dll mozilla::a11y::NotificationController::WillRefresh accessible/base/NotificationController.cpp:837
3 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:1892
4 xul.dll mozilla::RefreshDriverTimer::TickRefreshDrivers layout/base/nsRefreshDriver.cpp:325
5 xul.dll mozilla::RefreshDriverTimer::Tick layout/base/nsRefreshDriver.cpp:342
6 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver layout/base/nsRefreshDriver.cpp:708
7 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run layout/base/nsRefreshDriver.cpp:508
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175
9 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486

bp-81ed8710-566c-4128-ad1c-8a0fe0191116 Firefox 70.0.1
0 xul.dll mozilla::a11y::RootAccessible::ProcessDOMEvent(mozilla::dom::Event*, nsINode*) accessible/generic/RootAccessible.cpp:351
1 xul.dll mozilla::a11y::TNotification<mozilla::a11y::RootAccessible, mozilla::dom::Event, nsINode>::Process() accessible/base/NotificationController.h:72
2 xul.dll mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) accessible/base/NotificationController.cpp:837
3 xul.dll nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1892
4 xul.dll mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) layout/base/nsRefreshDriver.cpp:325
5 xul.dll mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:342
6 xul.dll mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:708

Comment 1

[James Teh [:Jamie]]

(In reply to Wayne Mery (:wsmwk) from comment #0)

makes one wonder whether Bug 1502061 - Crash in mozilla::a11y::RootAccessible::ProcessDOMEvent, fixed for v65, made this better or worse for Thunderbird. Perhaps we have a regression?

Looking at that patch, I don't see how this could be a regression from it. That patch just introduces a null check and early return to prevent a crash earlier in the function. I agree it's an odd coincidence, though.

Comment 2

[James Teh [:Jamie]]

The crash dumps are useless; everything I need is optimised out. From what I can tell from the stack, it looks like calling AsXULMultiSelectControl() on the tree element returns null. I don't understand why that would happen. All XUL trees should implement nsIDOMXULMultiSelectControlElement. The tree has DOM focus (since this code only runs if that is the case), so the element shouldn't be dying.

The best I can do is wallpaper over this in release builds and throw in an assert so we might hopefully one day catch this in a debug build.

Comment 3

[James Teh [:Jamie]]

Attachment: Bug 1597043: RootAccessible::ProcessDOMEvent: Return early if AsXULMultiSelectControl() on a XUL tree element fails.

This really shouldn't be possible.
All XUL trees should have nsIDOMXULMultiSelectControlElement, and the tree is focused at this point, so it shouldn't be dying.
Nevertheless, this sometimes happens in the wild and was causing crashes.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbe5a080a2ad
RootAccessible::ProcessDOMEvent: Return early if AsXULMultiSelectControl() on a XUL tree element fails. r=MarcoZ

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/cbe5a080a2ad

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance.

Comment 7

[James Teh [:Jamie]]

Comment on attachment 9109331 [details]
Bug 1597043: RootAccessible::ProcessDOMEvent: Return early if AsXULMultiSelectControl() on a XUL tree element fails.

Beta/Release Uplift Approval Request

• User impact if declined: Rare crashes. I don't know how to reproduce, so I can't provide a test, nor can I verify the fix. However, the crash stack very strongly suggests that this patch should fix the 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): Straightforward null check.
• String changes made/needed: None.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crashes.
• User impact if declined: Rare crashes. I don't know how to reproduce, so I can't provide a test, nor can I verify the fix. However, the crash stack very strongly suggests that this patch should fix the crash.
• Fix Landed on Version: 72, also uplifting to 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward null check.
• String or UUID changes made by this patch: None.

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9109331 [details]
Bug 1597043: RootAccessible::ProcessDOMEvent: Return early if AsXULMultiSelectControl() on a XUL tree element fails.

Low risk fix for rare crashes, uplift approved for both beta and esr, thanks.

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5bc687cb1607

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/d36df7bc9977

Comment 11

[Wayne Mery (:wsmwk)]

Thanks for the patch and uplift. I can confirm this is gone in current Thunderbird versions

Comment 12

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.