1585851 - Assertion failure: aNewIndex <= mChildren.Length() (Wrong new index was given), at src/accessible/generic/Accessible.cpp:2219

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20191002163554
SourceStamp=314a0fee08fdb0cbeb9c9023f634ef95375dab9b

Assertion failure: aNewIndex <= mChildren.Length() (Wrong new index was given), at src/accessible/generic/Accessible.cpp:2219

==106154==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7fb54c8ab94a bp 0x7ffeab0c9b10 sp 0x7ffeab0c9ab0 T0)
==106154==The signal is caused by a WRITE memory access.
==106154==Hint: address points to the zero page.
    #0 0x7fb54c8ab949 in mozilla::a11y::Accessible::RelocateChild(unsigned int, mozilla::a11y::Accessible*) src/accessible/generic/Accessible.cpp:2215:3
    #1 0x7fb54c8c96fe in mozilla::a11y::DocAccessible::MoveChild(mozilla::a11y::Accessible*, mozilla::a11y::Accessible*, int) src/accessible/generic/DocAccessible.cpp:2337:16
    #2 0x7fb54c8c8e50 in mozilla::a11y::DocAccessible::ProcessContentInserted(mozilla::a11y::Accessible*, nsTArray<nsCOMPtr<nsIContent> > const*) src/accessible/generic/DocAccessible.cpp:1946:9
    #3 0x7fb54c8313f3 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:743:16
    #4 0x7fb5499524bd in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1928:12
    #5 0x7fb54996447f in TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
    #6 0x7fb54996447f in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350
    #7 0x7fb549963e40 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
    #8 0x7fb549967683 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:807:5
    #9 0x7fb549967683 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:727
    #10 0x7fb5499669b9 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::VsyncEvent const&) src/layout/base/nsRefreshDriver.cpp:622:9
    #11 0x7fb54a25b6db in mozilla::layout::VsyncChild::RecvNotify(mozilla::VsyncEvent const&) src/layout/ipc/VsyncChild.cpp:65:16
    #12 0x7fb541caba75 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:187:54
    #13 0x7fb54175c5cb in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
    #14 0x7fb540fc2736 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2185:25
    #15 0x7fb540fbd38d in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2109:9
    #16 0x7fb540fbf9b7 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1954:3
    #17 0x7fb540fc0847 in mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1985:13
    #18 0x7fb53fd87e79 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1225:14
    #19 0x7fb53fd8eae8 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:486:10
    #20 0x7fb540fcbb1f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:88:21
    #21 0x7fb540ec49e2 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #22 0x7fb540ec49e2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308
    #23 0x7fb540ec49e2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290
    #24 0x7fb5493be619 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #25 0x7fb54d30dabf in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:934:20
    #26 0x7fb540ec49e2 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #27 0x7fb540ec49e2 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308
    #28 0x7fb540ec49e2 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290
    #29 0x7fb54d30d366 in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:769:34
    #30 0x55aed89f5cea in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #31 0x55aed89f5cea in main src/browser/app/nsBrowserApp.cpp:272

Comment 1

[Marco Zehe (:MarcoZ)]

Jamie, could this be another fall-out from bug 686400?

Comment 2

[James Teh [:Jamie]]

I'd say so, yes. I was hoping this might be fixed by bug 1581589 or bug 1582619, but alas not. I get the RelocateChild crash covered by bug 1578185 when I run this test case in a nightly build, so that bug will probably be fixed by this one.

Comment 3

[James Teh [:Jamie]]

Morgan, do you want to take a stab at debugging this? This goes deep into our tree building code, so I imagine we'll need to collaborate to fully understand this one, but figuring out what code paths we're failing on here would be an enormously good start.

Comment 4

[Andrew Overholt [:overholt]]

Hi! We were looking at our list of regression bugs carried over into 71 and this came up. Do we think we'll be able to get a fix in 71?

Comment 5

[Tyson Smith [:tsmith]]

Please let me know if a Pernosco session would be helpful and I will create one.

Comment 6

[Morgan Reschenberg [:morgan]]

I haven't been able to make much progress on this, so it won't likely land in 71. I would love a Pernosco session, thank you @tsmith :)

Comment 7

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/Pwl_z-Koo__dj1JTicv-JQ/index.html
It will automatically expire in 7 days.

Comment 8

[James Teh [:Jamie]]

Taking this due to p1 71 crash and Morgan having her plate full.

Comment 9

[James Teh [:Jamie]]

Attachment: Bug 1585851: When processing a11y insertions, don't try to move an accessible if its new previous sibling hasn't been moved into its new container yet.

Sometimes, depending on how children were changed, children might be in the insertion list out of order; e.g. [child2, child1].
It's also possible that an earlier child (child1 in the above example) is being moved out of another container.
When processing the earlier insertion (child2), we'll determine we need to move it within its parent and will fetch its new previous sibling so we can move it into the right place.
However, in this case, the new previous sibling (child1) will be in the wrong container.
We can't move in that case; the new previous sibling's index in parent will obviously be wrong, since it's relative to the wrong container.
Therefore, we just skip the move.
Since the previous sibling (child1) is later in the insertion list, the ordering will be corrected when we process that insertion.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61e813c94bc9
When processing a11y insertions, don't try to move an accessible if its new previous sibling hasn't been moved into its new container yet. r=MarcoZ

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/61e813c94bc9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[James Teh [:Jamie]]

Comment on attachment 9105115 [details]
Bug 1585851: When processing a11y insertions, don't try to move an accessible if its new previous sibling hasn't been moved into its new container yet.

Beta/Release Uplift Approval Request

• User impact if declined: Crashes.
• 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): A11y correctness fix which is only relevant in obscure circumstances, but avoids an obvious crash when these circumstances occur.
• String changes made/needed: None.

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61e813c94bc9

Comment 15

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

Comment on attachment 9105115 [details]
Bug 1585851: When processing a11y insertions, don't try to move an accessible if its new previous sibling hasn't been moved into its new container yet.

Fix for a frequent a11y crash on beta, patch baked a week on nightly and proved effective and as tests, uplift approved for 71 beta 8, thanks.

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/339514edfe82