Closed
Bug 1371781
Opened 7 years ago
Closed 7 years ago
Assertion failure: origContainer == prevChild->Parent() (Broken tree) [@ mozilla::a11y::DocAccessible::PutChildrenBack]
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla56
People
(Reporter: tsmith, Assigned: eeejay)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 3 obsolete files)
383 bytes,
text/html
|
Details | |
4.11 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•7 years ago
|
Assignee: nobody → eitan
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8891094 -
Flags: review?(surkov.alexander)
Comment 2•7 years ago
|
||
(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?
Assignee | ||
Comment 3•7 years ago
|
||
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•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8891543 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•7 years ago
|
Attachment #8891094 -
Attachment is obsolete: true
Attachment #8891094 -
Flags: review?(surkov.alexander)
Comment 7•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
Opted out of adding another DocAccessible method. Removed the "containers" array that is not used in PutChildrenBack,
Assignee | ||
Updated•7 years ago
|
Attachment #8891543 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
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
Comment 10•7 years ago
|
||
Pushed by eisaacson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3bdb80960d Fix eslint issues in test. r=me
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
Flags: needinfo?(eitan)
Hrm, looks like the followup actually fixed the test failure. Feel free to reland this with the followup folded in.
Assignee | ||
Comment 13•7 years ago
|
||
Test fixed and eslinted. Attaching here for relanding because the tree is currently closed.
Assignee | ||
Updated•7 years ago
|
Attachment #8892153 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(eitan)
Keywords: checkin-needed
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7a7d0f161888
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 16•7 years ago
|
||
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?
status-firefox54:
--- → wontfix
status-firefox55:
--- → wontfix
status-firefox-esr52:
--- → affected
Flags: needinfo?(eitan)
Assignee | ||
Comment 17•7 years ago
|
||
No, I don't think so. Although the assertion fails, we recover and do the right thing in non-debug builds.
Flags: needinfo?(eitan)
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•