Closed Bug 1371781 Opened 3 years ago Closed 2 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: tsmith, Assigned: eeejay)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 3 obsolete files)

Attached file 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)
Assignee: nobody → eitan
(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?
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".
(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.
(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.
Attachment #8891094 - Attachment is obsolete: true
Attachment #8891094 - Flags: review?(surkov.alexander)
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.
Attachment #8891543 - Flags: review?(surkov.alexander) → review+
Opted out of adding another DocAccessible method. Removed the "containers" array that is not used in PutChildrenBack,
Attachment #8891543 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2142989aace8
Allow <select> accessible children to be put back in list accessible. r=surkov
Keywords: checkin-needed
Hrm, looks like the followup actually fixed the test failure. Feel free to reland this with the followup folded in.
Test fixed and eslinted. Attaching here for relanding because the tree is currently closed.
Attachment #8892153 - Attachment is obsolete: true
Flags: needinfo?(eitan)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7d0f161888
Allow <select> accessible children to be put back in list accessible. r=surkov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7a7d0f161888
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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?
Flags: needinfo?(eitan)
No, I don't think so. Although the assertion fails, we recover and do the right thing in non-debug builds.
Flags: needinfo?(eitan)
See Also: → 1388246
You need to log in before you can comment on or make changes to this bug.