1455745 - Crash in mozilla::a11y::CreateHolderFromAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is
report bp-c195137e-f7db-4a10-8924-150880180419.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::a11y::CreateHolderFromAccessible accessible/ipc/win/COMPtrTypes.cpp:54
1 xul.dll mozilla::a11y::DocAccessible::DoInitialUpdate accessible/generic/DocAccessible.cpp:1474
2 xul.dll mozilla::a11y::DocAccessibleWrap::DoInitialUpdate accessible/windows/msaa/DocAccessibleWrap.cpp:146
3 xul.dll mozilla::a11y::NotificationController::WillRefresh accessible/base/NotificationController.cpp:666
4 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:1882
5 xul.dll mozilla::RefreshDriverTimer::TickDriver layout/base/nsRefreshDriver.cpp:337
6 xul.dll mozilla::InactiveRefreshDriverTimer::TickOne layout/base/nsRefreshDriver.cpp:958
7 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:701
8 xul.dll nsTimerEvent::Run xpcom/threads/TimerThread.cpp:290
9 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1096

=============================================================

#3 Windows top crash for the 4-17 Nightlies, though that is mostly from a single user. These crashes are all
  MOZ_RELEASE_ASSERT((((HRESULT)(hr)) >= 0))

The last comment in bug 1399557 is "No, we've since landed some patches to get rid of those crashes on Nightly." but it looks like this is still happening almost two months later.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Jamie, do you think this is another location that we should convert to MOZ_ASSERT?

Comment 2

[James Teh [:Jamie]]

I think so. While I'd obviously prefer to fix this properly, these crashes aren't helping us achieve this at this stage.

I think we'd also need to convert the assert in accessible/generic/DocAccessible.cpp:1475.

Comment 3

[James Teh [:Jamie]]

Looking at the crash dump from comment 0, the HRESULT is 0x80040153 "Invalid value for registry". That's... intriguing. Any ideas at all, Aaron? The obvious answer is that something COM or IAccessible related is pretty badly messed up in the user's registry, but I'm not sure if we have much chance of figuring out exactly what. Our activation context should supply most of what we need.

I'll still go ahead and submit a patch to convert these to asserts.

Comment 4

[James Teh [:Jamie]]

Attachment: Bug 1455745: Remove more accessibility related diagnostic crashes on Nightly.

This condition is rare and does indicate a problem which breaks accessibility.
However, we aren't getting any closer to diagnosing this as a result of this crash, so it causes user pain without any gain to us.

Review commit: https://reviewboard.mozilla.org/r/238820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238820/

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8970067 [details]
Bug 1455745: Remove more accessibility related diagnostic crashes on Nightly.

https://reviewboard.mozilla.org/r/238820/#review244620

::: accessible/generic/DocAccessible.cpp:1481
(Diff revision 1)
>          int32_t childID = AccessibleWrap::GetChildIDFor(this);
>  #else
>          int32_t holder = 0, childID = 0;
>  #endif
>          tabChild->SendPDocAccessibleConstructor(ipcDoc, nullptr, 0, childID,
>                                                  holder);

should you actually proceed with it having IsNull() holder?

Comment 6

[James Teh [:Jamie]]

Comment on attachment 8970067 [details]
Bug 1455745: Remove more accessibility related diagnostic crashes on Nightly.

https://reviewboard.mozilla.org/r/238820/#review244620

> should you actually proceed with it having IsNull() holder?

This will certainly cause broken a11y (failing calls) from the client's perspective, but our code handles this without crashing:
1. It will trigger an assert in the parent process, but that's already a MOZ_ASSERT, not a MOZ_DIAGNOSTIC_ASSERT. See https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#972
2. See protection in https://searchfox.org/mozilla-central/source/accessible/ipc/win/ProxyAccessible.h#48

Comment 7

[alexander :surkov (:asurkov)]

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

> > should you actually proceed with it having IsNull() holder?
> 
> This will certainly cause broken a11y (failing calls) from the client's
> perspective, but our code handles this without crashing:
> 1. It will trigger an assert in the parent process, but that's already a
> MOZ_ASSERT, not a MOZ_DIAGNOSTIC_ASSERT. See
> https://searchfox.org/mozilla-central/source/dom/ipc/TabParent.cpp#972
> 2. See protection in
> https://searchfox.org/mozilla-central/source/accessible/ipc/win/
> ProxyAccessible.h#48

ok, I see, thank you for the pointers

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8970067 [details]
Bug 1455745: Remove more accessibility related diagnostic crashes on Nightly.

https://reviewboard.mozilla.org/r/238820/#review246544

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to James Teh [:Jamie] from comment #3)
> Looking at the crash dump from comment 0, the HRESULT is 0x80040153 "Invalid
> value for registry". That's... intriguing. Any ideas at all, Aaron? The
> obvious answer is that something COM or IAccessible related is pretty badly
> messed up in the user's registry, but I'm not sure if we have much chance of
> figuring out exactly what. Our activation context should supply most of what
> we need.
> 
> I'll still go ahead and submit a patch to convert these to asserts.

Yeah, some stuff must be pretty messed up. We do have logging in place in certain areas to annotate crash reports with the contents of the registry, but we don't have those in this case.

Comment 10

[Mozilla Autoland]

hg error in cmd: hg pull upstream: pulling from https://hg.mozilla.org/integration/autoland
searching for changes
abort: HTTP Error 500: Internal Server Error

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c36098a8e83f
Remove more accessibility related diagnostic crashes on Nightly. r=aklotz

Comment 12

[Andreea Pavel [:apavel]]

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