1369185 - Assertion failure: aChild == mListAccessible, at src/accessible/html/HTMLSelectAccessible.cpp:366

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

This test requires a11y, please make sure it is enabled. 

One method to do so is by using:
https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension
Assertion failure: aChild == mListAccessible, at src/accessible/html/HTMLSelectAccessible.cpp:366

#0 0x7fce48d0b45a in mozilla::a11y::HTMLComboboxAccessible::RemoveChild(mozilla::a11y::Accessible*) src/accessible/html/HTMLSelectAccessible.cpp:366:3
#1 0x7fce48cee324 in mozilla::a11y::DocAccessible::MoveChild(mozilla::a11y::Accessible*, mozilla::a11y::Accessible*, int) src/accessible/generic/DocAccessible.cpp:2242:14
#2 0x7fce48ceda2b in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2127:9
#3 0x7fce48c95cf2 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:810:18
#4 0x7fce46b2335f in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1790:12
#5 0x7fce46b2c14e in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:300:7
#6 0x7fce46b2bf1d in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:321:5
#7 0x7fce46b2fd65 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:753:5
#8 0x7fce46b2e886 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:666:35
#9 0x7fce46b2a537 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:512:20
#10 0x7fce4156cff1 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1302:14
#11 0x7fce415771a0 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:393:10
#12 0x7fce420a1235 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:96:21
#13 0x7fce41fee1d7 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:238:10
#14 0x7fce41fee069 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:211:3
#15 0x7fce4665207a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
#16 0x7fce49232471 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:283:30
#17 0x7fce4938ba52 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4568:22
#18 0x7fce4938d68b in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4748:8
#19 0x7fce4938e578 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4843:21
#20 0x4eca88 in do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:236:22
#21 0x4ec3a0 in main src/browser/app/nsBrowserApp.cpp:309:16
#22 0x7fce5e02c82f in __libc_start_main /build/glibc-9tT8Do/glibc-2.23/csu/../csu/libc-start.c:291
#23 0x41e0d4 in _start (m-c-1495643560-asan-debug/firefox+0x41e0d4)

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Don't allow <select> to aria-own, or <option> to be owned. r?surkov

I checked with Chrome, and they don't allow <select> to aria own outside elements of any kind. Neither do
 they allow other containers to own option/optgroup elements.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 8904745 [details] [diff] [review]
Don't allow <select> to aria-own, or <option> to be owned. r?surkov

This is failing some tests. I'll upload a new patch later today.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Don't allow <select> to aria-own, or <option> to be owned. r?surkov

IsAcceptableChild() needed to be implemented correctly in certain Accessible classes to get this right..

Comment 4

[alexander :surkov (:asurkov)]

Comment on attachment 8906136 [details] [diff] [review]
Don't allow <select> to aria-own, or <option> to be owned. r?surkov

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

::: accessible/generic/Accessible.h
@@ +497,5 @@
>    /**
>     * Return true if the accessible is an acceptable child.
>     */
>    virtual bool IsAcceptableChild(nsIContent* aEl) const
> +    { return aEl && !aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup); }

thinking aloud: one day we probably should have a map for this

::: accessible/generic/DocAccessible.cpp
@@ +1609,5 @@
>  
> +      nsIContent* dependentContent = iter.GetElem(id);
> +      if (relAttr == nsGkAtoms::aria_owns &&
> +          !aRelProvider->IsAcceptableChild(dependentContent))
> +        continue;

thinking aloud: I agree that tossing it out early is a good thing, despite all validation happens later.

oh, that's a hack for HTML selects, and not obvious one:  you ignore aria-owns on HTML:select entirely. Is that what we really want to do? For example, do we want to deny aria-owns on selects pointing to own children?

::: accessible/html/HTMLSelectAccessible.cpp
@@ +313,5 @@
> +bool
> +HTMLSelectOptGroupAccessible::IsAcceptableChild(nsIContent* aEl) const
> +{
> +  return aEl->IsNodeOfType(nsINode::eDATA_NODE) ||
> +    aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup);

do we allow nested optgroups? are they flatted out?

Comment 5

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

Eitan is this still P1? (Please feel free to make a call on the priority here)

Comment 6

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] (NeedInfo me for attention) from comment #5)
> Eitan is this still P1? (Please feel free to make a call on the priority
> here)

oh yeah, either p1 or p2 I believe, it's a fix for some bad things that may happen with a11y tree.

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #4)
> Comment on attachment 8906136 [details] [diff] [review]
> Don't allow <select> to aria-own, or <option> to be owned. r?surkov
> 
> Review of attachment 8906136 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/Accessible.h
> @@ +497,5 @@
> >    /**
> >     * Return true if the accessible is an acceptable child.
> >     */
> >    virtual bool IsAcceptableChild(nsIContent* aEl) const
> > +    { return aEl && !aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup); }
> 
> thinking aloud: one day we probably should have a map for this
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +1609,5 @@
> >  
> > +      nsIContent* dependentContent = iter.GetElem(id);
> > +      if (relAttr == nsGkAtoms::aria_owns &&
> > +          !aRelProvider->IsAcceptableChild(dependentContent))
> > +        continue;
> 
> thinking aloud: I agree that tossing it out early is a good thing, despite
> all validation happens later.

It's crucial we catch this early. Otherwise the subtree that is not "owned" does not get constructed because it is marked as a dependent ID to be constructed later in DoARIAOwnsRelocation (where it is ignored).
 
> 
> oh, that's a hack for HTML selects, and not obvious one:  you ignore
> aria-owns on HTML:select entirely. Is that what we really want to do? For
> example, do we want to deny aria-owns on selects pointing to own children?

Pretty sure we don't want to do that either. I tried to generalize this with IsAcceptableChild and not make it specific to HTMLSelect, in case there are other accessible types that by definition never accept children or children of a certain type.

As for aria-owns reordering in a select element, I could go back to check Chrome. But my hunch is that they don't allow that either. It makes sense, <select> is implemented as a native widget, you can't modify the layout or the focus order.

> 
> ::: accessible/html/HTMLSelectAccessible.cpp
> @@ +313,5 @@
> > +bool
> > +HTMLSelectOptGroupAccessible::IsAcceptableChild(nsIContent* aEl) const
> > +{
> > +  return aEl->IsNodeOfType(nsINode::eDATA_NODE) ||
> > +    aEl->IsAnyOfHTMLElements(nsGkAtoms::option, nsGkAtoms::optgroup);
> 
> do we allow nested optgroups? are they flatted out?

Good catch! Need to add that..

Comment 8

[Eitan Isaacson [:eeejay]]

Making this a P2 because it is not a crasher.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #7)

> > thinking aloud: I agree that tossing it out early is a good thing, despite
> > all validation happens later.
> 
> It's crucial we catch this early. Otherwise the subtree that is not "owned"
> does not get constructed because it is marked as a dependent ID to be
> constructed later in DoARIAOwnsRelocation (where it is ignored).

then you don't need to check IsAcceptableChild in DoARIAOwnsRelocation?

> > 
> > oh, that's a hack for HTML selects, and not obvious one:  you ignore
> > aria-owns on HTML:select entirely. Is that what we really want to do? For
> > example, do we want to deny aria-owns on selects pointing to own children?
> 
> Pretty sure we don't want to do that either. I tried to generalize this with
> IsAcceptableChild and not make it specific to HTMLSelect, in case there are
> other accessible types that by definition never accept children or children
> of a certain type.
> 
> As for aria-owns reordering in a select element, I could go back to check
> Chrome. But my hunch is that they don't allow that either. It makes sense,
> <select> is implemented as a native widget, you can't modify the layout or
> the focus order.

it sounds reasonable, but could you add a test case for this case?

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 8906136 [details] [diff] [review]
Don't allow <select> to aria-own, or <option> to be owned. r?surkov

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

cancelling review; Eitan, is new patch is coming, right?

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #9)
> (In reply to Eitan Isaacson [:eeejay] from comment #7)
> 
> > > thinking aloud: I agree that tossing it out early is a good thing, despite
> > > all validation happens later.
> > 
> > It's crucial we catch this early. Otherwise the subtree that is not "owned"
> > does not get constructed because it is marked as a dependent ID to be
> > constructed later in DoARIAOwnsRelocation (where it is ignored).
> 
> then you don't need to check IsAcceptableChild in DoARIAOwnsRelocation?

We still need to because
1. We schedule relocations for many reasons (initial tree/attribute change/content inserted).
2. A container can aria-own one acceptable child and one not acceptable child. DoARIAOwnsRelocation will iterate over all those children IDs.

It's important that the ignore policy be consistent, hence the use of IsAcceptableChild everywhere.

> 
> > > 
> > > oh, that's a hack for HTML selects, and not obvious one:  you ignore
> > > aria-owns on HTML:select entirely. Is that what we really want to do? For
> > > example, do we want to deny aria-owns on selects pointing to own children?
> > 
> > Pretty sure we don't want to do that either. I tried to generalize this with
> > IsAcceptableChild and not make it specific to HTMLSelect, in case there are
> > other accessible types that by definition never accept children or children
> > of a certain type.
> > 
> > As for aria-owns reordering in a select element, I could go back to check
> > Chrome. But my hunch is that they don't allow that either. It makes sense,
> > <select> is implemented as a native widget, you can't modify the layout or
> > the focus order.
> 
> it sounds reasonable, but could you add a test case for this case?

Sure.

Comment 12

[Eitan Isaacson [:eeejay]]

Attachment: Don't allow <select> to aria-own, or <option> to be owned. r?surkov

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 8907818 [details] [diff] [review]
Don't allow <select> to aria-own, or <option> to be owned. r?surkov

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

r=me, thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e766e76dba
Don't allow <select> to aria-own, or <option> to be owned. r=surkov

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2e766e76dba