1371781 - Assertion failure: origContainer == prevChild->Parent() (Broken tree) [@ mozilla::a11y::DocAccessible::PutChildrenBack]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Found on Ubuntu 16.04 with m-c 20170608162720 e61060be364

This test case requires fuzzPriv plugin. It should also work with enabling screen reader enabled.

Assertion failure: origContainer == prevChild->Parent() (Broken tree), at src/accessible/generic/DocAccessible.cpp:2176

#0 0x7fbd1a960b16 in mozilla::a11y::DocAccessible::PutChildrenBack(nsTArray<RefPtr<mozilla::a11y::Accessible> >*, unsigned int) src/accessible/generic/DocAccessible.cpp:2176:11
#1 0x7fbd1a95fb67 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2136:3
#2 0x7fbd1a907df2 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:809:18
#3 0x7fbd187a1870 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1831:12
#4 0x7fbd187aac2e in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301:7
#5 0x7fbd187aa9fd in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:322:5
#6 0x7fbd187ae875 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:754:5
#7 0x7fbd187ad366 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:667:35
#8 0x7fbd187a9007 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:513:20
#9 0x7fbd1314c35f in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1406:14
#10 0x7fbd13156d20 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:472:10
#11 0x7fbd13ca4075 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21
#12 0x7fbd13bf08e7 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:238:10
#13 0x7fbd13bf0779 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:211:3
#14 0x7fbd182c2c2a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
#15 0x7fbd1aec3331 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
#16 0x7fbd1b01e6c2 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4569:22
#17 0x7fbd1b0202fe in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4749:8
#18 0x7fbd1b0211e8 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4844:21
#19 0x4eca88 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:236:22
#20 0x4ec3a0 in main src/browser/app/nsBrowserApp.cpp:309:16
#21 0x7fbd2fd7682f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
#22 0x41e0d4 in _start (m-c-1496939240-asan-debug/firefox+0x41e0d4)

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Allow <select> accessible children to be put back in list accessible. r?surkov

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #1)
> Created attachment 8891094 [details] [diff] [review]
> Allow <select> accessible children to be put back in list accessible.
> r?surkov

I'm not sure how it works at all if MoveChild operates on a wrong parent. Should we replace GetAccessibleContainer or TrueContainer method similary to AccessibleOrTrueContainer method?

Comment 3

[Eitan Isaacson [:eeejay]]

We do this one line later:
origContainer = prevChild->Parent();

This puts it in the correct container, the assert just needed to be updated to accept that.

It was a strange assert beforehand: "x should equal y. ok, now assign x to y".

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #3)
> We do this one line later:
> origContainer = prevChild->Parent();

right, missed that part

anyway, would it be more correct to go with TrueContainer from the start?

> It was a strange assert beforehand: "x should equal y. ok, now assign x to
> y".

the point is tt happens but it shouldn't happen, this is the assert-and-then-fix approach.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > We do this one line later:
> > origContainer = prevChild->Parent();
> 
> right, missed that part
> 
> anyway, would it be more correct to go with TrueContainer from the start?

Perfect. Didn't see that as an option.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Allow <select> accessible children to be put back in list accessible. r?surkov

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8891543 [details] [diff] [review]
Allow <select> accessible children to be put back in list accessible. r?surkov

Review of attachment 8891543 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/generic/DocAccessible.cpp
@@ +2204,5 @@
>      // Unset relocated flag to find an insertion point for the child.
>      child->SetRelocated(false);
>  
>      int32_t idxInParent = -1;
> +    Accessible* origContainer = AccessibleOrTrueContainer(content->GetParentNode());

I would probably add new TrueContainer method, but non document child has to have a content if it's in the document for sure. This code has a dependency on it already: walker.Seek assumes a non-null node.

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Allow <select> accessible children to be put back in list accessible.

Opted out of adding another DocAccessible method. Removed the "containers" array that is not used in PutChildrenBack,

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2142989aace8
Allow <select> accessible children to be put back in list accessible. r=surkov

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3bdb80960d
Fix eslint issues in test. r=me

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed this and the followup out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=119803943&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/412c52914a1b4035c722d4c36002c2f6dca39e70

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hrm, looks like the followup actually fixed the test failure. Feel free to reland this with the followup folded in.

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Allow <select> accessible children to be put back in list accessible.

Test fixed and eslinted. Attaching here for relanding because the tree is currently closed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7d0f161888
Allow <select> accessible children to be put back in list accessible. r=surkov

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a7d0f161888

Comment 16

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this goes back a ways. We're too late to backport to 55, but is there enough user impact to justify consideration for ESR52?

Comment 17

[Eitan Isaacson [:eeejay]]

No, I don't think so. Although the assertion fails, we recover and do the right thing in non-debug builds.