1380199 - Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Event coalescence killed the accessible) [@ mozilla::a11y::DocAccessible::ContentRemoved]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_cases.html

Changeset: 6fec4855b5345eb63fef57089e61829b88f5f4eb
Build ID: 20170711160010

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: Event coalescence killed the accessible), at /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2002

#0 0x7f078758d61d in mozilla::a11y::DocAccessible::ContentRemoved(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:1988:3
#1 0x7f07875894c4 in mozilla::a11y::DocAccessible::ContentRemoved(nsIContent*) src/accessible/generic/DocAccessible.cpp:2030:5
#2 0x7f0787550e12 in nsAccessibilityService::ContentRemoved(nsIPresShell*, nsIContent*) src/accessible/base/nsAccessibilityService.cpp:598:15
#3 0x7f0784f43223 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*, nsIContent**) src/layout/base/nsCSSFrameConstructor.cpp:8764:19
#4 0x7f0784f38b9c in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) src/layout/base/nsCSSFrameConstructor.cpp:10028:5
#5 0x7f0784e99965 in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) src/layout/base/RestyleManager.cpp:1544:25
#6 0x7f0784e834c1 in mozilla::GeckoRestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/GeckoRestyleManager.cpp:3478:3
#7 0x7f0784e8299b in mozilla::GeckoRestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) src/layout/base/GeckoRestyleManager.cpp:151:5
#8 0x7f0784ee97b0 in mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint, mozilla::RestyleHintData const&) src/layout/base/RestyleTracker.cpp:94:22
#9 0x7f0784ee7e40 in mozilla::RestyleTracker::DoProcessRestyles() src/layout/base/RestyleTracker.cpp:255:11
#10 0x7f0784e857c4 in mozilla::GeckoRestyleManager::ProcessPendingRestyles() src/layout/base/GeckoRestyleManager.cpp:502:3
#11 0x7f0784ebbaed in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4193:41
#12 0x7f0784ebb3d1 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::FlushType) src/layout/base/PresShell.cpp:4069:3
#13 0x7f0785146d07 in mozilla::ScrollFrameHelper::AsyncScrollPortEvent::Run() src/layout/generic/nsGfxScrollFrame.cpp:4780:7
#14 0x7f0784fde062 in nsRootPresContext::FlushWillPaintObservers() src/layout/base/nsPresContext.cpp:3454:19
#15 0x7f0784ed85c9 in mozilla::PresShell::WillPaint() src/layout/base/PresShell.cpp:8874:20
#16 0x7f0784906762 in nsViewManager::CallWillPaintOnObservers() src/view/nsViewManager.cpp:1130:18
#17 0x7f0784905934 in nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1092:5
#18 0x7f0784e51c45 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2049:11
#19 0x7f0784e5a0be in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:298:7
#20 0x7f0784e59e9f in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:320:5
#21 0x7f0784e5d5a5 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:761:5
#22 0x7f0784e5c536 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:674:35
#23 0x7f0784e58497 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:520:20
#24 0x7f077f6285c2 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1437:14
#25 0x7f077f62e170 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:489:10
#26 0x7f0780192e25 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#27 0x7f07800df247 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:320:10
#28 0x7f07800df0d9 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:293:3
#29 0x7f078496e65a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
#30 0x7f0787af34d1 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:287:30
#31 0x7f0787c4f0c2 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4595:22
#32 0x7f0787c50d1f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4778:8
#33 0x7f0787c51c08 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4873:21
#34 0x4ecaf8 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:237:22
#35 0x4ec410 in main src/browser/app/nsBrowserApp.cpp:310:16
#36 0x7f079d4ad82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#37 0x41e144 in _start (m-c-1499788810-asan-debug/firefox+0x41e144)

Comment 1

[alexander :surkov (:asurkov)]

bug 1368784 should fix this one

Comment 2

[Eitan Isaacson [:eeejay]]

* Pasting from bug 1376825 *

I first thought that this could be remedied by checking if given parent is a show event[1] target before checking for it being a hide event[2] target. That would have indicated a move.

The problem is, that at least in this case, the parent's show event is dropped in favor of a coalesced grandparent show event, so swapping those blocks wouldn't always help.

1. https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/accessible/base/NotificationController.cpp#391
2. https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/accessible/base/NotificationController.cpp#387

Comment 3

[Eitan Isaacson [:eeejay]]

I'm not sure how we want to coalesce a child hide-destroy event with a parent's hide-move event.

Alex, do you have any good ideas?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #3)
> I'm not sure how we want to coalesce a child hide-destroy event with a
> parent's hide-move event.

could you give more detailed description what exactly happens here and in what sequence?

> Alex, do you have any good ideas?

we might have an interim fix here for sure, or we could leave it for bug 1368784

Comment 5

[Eitan Isaacson [:eeejay]]

I suspect what is happening is:
1. A parent is relocated into a new grandparent (eg. DocAccessible::MoveChild).
2. A child is being removed.

There is a race with the parent's hide event (from the move), and the child's.
If the parent has a hide event first, you end up with the "unreachable" condition of a child's hide event being dropped.

My proposed interim fix is to remove this fatal assertion and replace it with a warning instead :)

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Don't coalesce parent's no-shutdown hide events, with child's shutdown hide. r?surkov

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Don't coalesce parent's no-shutdown hide events, with child's shutdown hide.

This is just a one possible solution. Didn't think it all the way through.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Pinging Eitan to sanity check this priority rating.

Comment 9

[Eitan Isaacson [:eeejay]]

Yeah, P3 sounds good.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Regression range:
INFO: Last good revision: 6ec07b1cf72135578fbfe26261be8c66cbe3874a
INFO: First bad revision: 62f4ce5004d4e7874a27c41cc0b18cd31fbf9206
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6ec07b1cf72135578fbfe26261be8c66cbe3874a&tochange=62f4ce5004d4e7874a27c41cc0b18cd31fbf9206

Which is fun. Tells me the video controls change maybe just exposed a latent bug elsewhere (since the testcase also makes use of them). Annnnnyway...

Fix range:
INFO: First good revision: 6240e75545e67d545e583055e95557610a5fadde
INFO: Last bad revision: c85d53f705115d4128246179483e28d5c232ce7c
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c85d53f705115d4128246179483e28d5c232ce7c&tochange=6240e75545e67d545e583055e95557610a5fadde

Which at least seems like it plausibly fixed this for real? Eitan, can you please confirm that when you get a chance? Also, is this testcase worth landing still?

Comment 11

[Eitan Isaacson [:eeejay]]

Yup, you're right. The patch from bug 1376754 fixed this. Alex, can we close this one?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #11)
> Yup, you're right. The patch from bug 1376754 fixed this. Alex, can we close
> this one?

Regression range gives us a solid base for this :) and the patch itself looks a plausible fix of course. I'd be nice though to put a testcase into our test suite as Ryan suggests. I'm curious if anyone is willing to do that.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: testcase

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8986315 [details] [diff] [review]
testcase

Thanks

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d52f133e42f
add a testcase for 'Event coalescence killed the accessible' assertion, r=yzen

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/5d52f133e42f