1378257 - Assertion failure: aChild->IndexInParent() != aIdxInParent (No move case), at /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2224

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

To reproduce launch the browser GNOME_ACCESSIBILITY=1 set and open the test case.

Assertion failure: aChild->IndexInParent() != aIdxInParent (No move case), at /home/worker/workspace/build/src/accessible/generic/DocAccessible.cpp:2224

#0 0x7f86df369f1a in mozilla::a11y::DocAccessible::MoveChild(mozilla::a11y::Accessible*, mozilla::a11y::Accessible*, int) accessible/generic/DocAccessible.cpp:2248:26
#1 0x7f86df36a1e1 in mozilla::a11y::DocAccessible::PutChildrenBack(nsTArray<RefPtr<mozilla::a11y::Accessible> >*, unsigned int) accessible/generic/DocAccessible.cpp:2186:5
#2 0x7f86df3691e0 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) accessible/generic/DocAccessible.cpp:2136:3
#3 0x7f86df311452 in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) accessible/base/NotificationController.cpp:811:18
#4 0x7f86dcc3280e in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1854:12
#5 0x7f86dcc3bbde in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) layout/base/nsRefreshDriver.cpp:298:7
#6 0x7f86dcc3b9ad in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:319:5
#7 0x7f86dcc3f0c5 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:761:5
#8 0x7f86dcc3e056 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:674:35
#9 0x7f86dcc39fb7 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() layout/base/nsRefreshDriver.cpp:520:20
#10 0x7f86d745e159 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1422:14
#11 0x7f86d7463940 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:489:10
#12 0x7f86d7fbe325 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:97:21
#13 0x7f86d7f0ab77 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:320:10
#14 0x7f86d7f0aa09 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:293:3
#15 0x7f86dc74fe2a in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27
#16 0x7f86df8cc5a1 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:287:30
#17 0x7f86dfa278f2 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4590:22
#18 0x7f86dfa2954f in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4773:8
#19 0x7f86dfa2a438 in XRE_main(int, char**, mozilla::BootstrapConfig const&) toolkit/xre/nsAppRunner.cpp:4868:21
#20 0x4ecaf8 in do_main(int, char**, char**) browser/app/nsBrowserApp.cpp:237:22
#21 0x4ec410 in main browser/app/nsBrowserApp.cpp:310:16
#22 0x7f86f525082f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#23 0x41e144 in _start (firefox+0x41e144)

Comment 1

[Eitan Isaacson [:eeejay]]

Looks like a dup of 1376214.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #1)
> Looks like a dup of 1376214.

it might be not that obvious. I assume, we catch 'Not in sync' assertion from 1376214, when the attached test case is running, and then this one, when children are pushed back. Is this correct?

Comment 4

[Eitan Isaacson [:eeejay]]

The stack suggests that they both happen at the PutChildrenBack stage. Here the assertion is from DocAccessible::MoveChild, and in bug 1376214 it is Accessible::MoveChild, which is past this assertion. Both check for the same thing (IndexInParent() != aIdxInParent)

(not sure how you would get past this one if you are crashing on assertions)

(In reply to alexander :surkov from comment #3)
> (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > Looks like a dup of 1376214.
> 
> it might be not that obvious. I assume, we catch 'Not in sync' assertion
> from 1376214, when the attached test case is running, and then this one,
> when children are pushed back. Is this correct?

No, we don't catch "Not in sync", that block is never entered in that test case (after my patch it does).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> The stack suggests that they both happen at the PutChildrenBack stage. Here
> the assertion is from DocAccessible::MoveChild, and in bug 1376214 it is
> Accessible::MoveChild, which is past this assertion. Both check for the same
> thing (IndexInParent() != aIdxInParent)
> 
> (not sure how you would get past this one if you are crashing on assertions)

right :) for some reason I treated them as warnings in the console :)

> (In reply to alexander :surkov from comment #3)
> > (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > > Looks like a dup of 1376214.
> > 
> > it might be not that obvious. I assume, we catch 'Not in sync' assertion
> > from 1376214, when the attached test case is running, and then this one,
> > when children are pushed back. Is this correct?
> 
> No, we don't catch "Not in sync", that block is never entered in that test
> case (after my patch it does).

so isn't it rather a separate issue and your patch is also a fix for this one?

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #5)
> (In reply to Eitan Isaacson [:eeejay] from comment #4)
> > The stack suggests that they both happen at the PutChildrenBack stage. Here
> > the assertion is from DocAccessible::MoveChild, and in bug 1376214 it is
> > Accessible::MoveChild, which is past this assertion. Both check for the same
> > thing (IndexInParent() != aIdxInParent)
> > 
> > (not sure how you would get past this one if you are crashing on assertions)
> 
> right :) for some reason I treated them as warnings in the console :)
> 
> > (In reply to alexander :surkov from comment #3)
> > > (In reply to Eitan Isaacson [:eeejay] from comment #1)
> > > > Looks like a dup of 1376214.
> > > 
> > > it might be not that obvious. I assume, we catch 'Not in sync' assertion
> > > from 1376214, when the attached test case is running, and then this one,
> > > when children are pushed back. Is this correct?
> > 
> > No, we don't catch "Not in sync", that block is never entered in that test
> > case (after my patch it does).
> 
> so isn't it rather a separate issue and your patch is also a fix for this
> one?

I believe it is the same issue. We are trying to "put the child back" to where it already is (ie. the initial aria-owns "move" was a no-op and shouldn't have taken place). I'll know for sure if this assertion goes away with the fix for bug 1376214 applied. I had difficulty reproducing this. I'll try again now.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Check if we are re-aria-owning a child into its current index. r?surkov

This is the same patch I misplaced in bug 1376214. I added some comments explaining it better.

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8886292 [details] [diff] [review]
Check if we are re-aria-owning a child into its current index. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2113,5 @@
> +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'
> +      // ...or directly after its current index, which is a no-op.
> +      // eg. a parent aria-owns its last ordinal child:
> +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> +      if (indexInParent == static_cast<int32_t>(insertIdx) ||

can this condition be true when child->IsRelocated() returns false? If not, then I'd nice to assert
MOZ_ASSERT(child->IsRelocated());

@@ +2115,5 @@
> +      // eg. a parent aria-owns its last ordinal child:
> +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> +      if (indexInParent == static_cast<int32_t>(insertIdx) ||
> +          indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> +        if (child->IsRelocated()) {

if a child is not yet relocated in your example above, then it has to be also 'no move' case, no?

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #8)
> Comment on attachment 8886292 [details] [diff] [review]
> Check if we are re-aria-owning a child into its current index. r?surkov
> 
> Review of attachment 8886292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2113,5 @@
> > +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'
> > +      // ...or directly after its current index, which is a no-op.
> > +      // eg. a parent aria-owns its last ordinal child:
> > +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> > +      if (indexInParent == static_cast<int32_t>(insertIdx) ||
> 
> can this condition be true when child->IsRelocated() returns false? If not,
> then I'd nice to assert
> MOZ_ASSERT(child->IsRelocated());

Do you mean only in the 'indexInParent == insertIdx' case? Yeah I guess IsRelocated should always be true.

I can re-do that and add an assert.

> 
> @@ +2115,5 @@
> > +      // eg. a parent aria-owns its last ordinal child:
> > +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> > +      if (indexInParent == static_cast<int32_t>(insertIdx) ||
> > +          indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> > +        if (child->IsRelocated()) {
> 
> if a child is not yet relocated in your example above, then it has to be
> also 'no move' case, no?

Correct. Are you suggesting a change here?

Comment 10

[alexander :surkov (:asurkov)]

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

> > can this condition be true when child->IsRelocated() returns false? If not,
> > then I'd nice to assert
> > MOZ_ASSERT(child->IsRelocated());
> 
> Do you mean only in the 'indexInParent == insertIdx' case? Yeah I guess
> IsRelocated should always be true.
> 
> I can re-do that and add an assert.

please, that should be cleaner

> > @@ +2115,5 @@
> > > +      // eg. a parent aria-owns its last ordinal child:
> > > +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> > > +      if (indexInParent == static_cast<int32_t>(insertIdx) ||
> > > +          indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> > > +        if (child->IsRelocated()) {
> > 
> > if a child is not yet relocated in your example above, then it has to be
> > also 'no move' case, no?
> 
> Correct. Are you suggesting a change here?

I probably was unclear. I meant, if 
(indexInParent == static_cast<int32_t>(insertIdx) - 1 && !child->IsRelocated()) is true
then you still get that assertion? If so, then the bug is not fixed?

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #10)
> (In reply to Eitan Isaacson [:eeejay] from comment #9)
> 
> > > can this condition be true when child->IsRelocated() returns false? If not,
> > > then I'd nice to assert
> > > MOZ_ASSERT(child->IsRelocated());
> > 
> > Do you mean only in the 'indexInParent == insertIdx' case? Yeah I guess
> > IsRelocated should always be true.
> > 
> > I can re-do that and add an assert.
> 
> please, that should be cleaner
> 
> > > @@ +2115,5 @@
> > > > +      // eg. a parent aria-owns its last ordinal child:
> > > > +      // <ul aria-owns='id1'><li id='id1'>Foo</li></ul>
> > > > +      if (indexInParent == static_cast<int32_t>(insertIdx) ||
> > > > +          indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> > > > +        if (child->IsRelocated()) {
> > > 
> > > if a child is not yet relocated in your example above, then it has to be
> > > also 'no move' case, no?
> > 
> > Correct. Are you suggesting a change here?
> 
> I probably was unclear. I meant, if 
> (indexInParent == static_cast<int32_t>(insertIdx) - 1 &&
> !child->IsRelocated()) is true
> then you still get that assertion? If so, then the bug is not fixed?

No, you don't get the assertion in that case with this patch. This patch assures that we don't move the child in that case.

Comment 12

[alexander :surkov (:asurkov)]

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

> > I probably was unclear. I meant, if 
> > (indexInParent == static_cast<int32_t>(insertIdx) - 1 &&
> > !child->IsRelocated()) is true
> > then you still get that assertion? If so, then the bug is not fixed?
> 
> No, you don't get the assertion in that case with this patch. This patch
> assures that we don't move the child in that case.

sorry I don't follow, I hope the code complexity will be my excuse :)

so your patch handles the condition, when it's true:
indexInParent == static_cast<int32_t>(insertIdx) - 1 && child->IsRelocated()

I miss why
indexInParent == static_cast<int32_t>(insertIdx) - 1 && !child->IsRelocated()) is never true

or if it's ever true, then why it doesn't assert for 'no move case' too.

Comment 13

[Eitan Isaacson [:eeejay]]

Attachment: Check if we are re-aria-owning a child into its current index. r?surkov

Comment 14

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #12)
> (In reply to Eitan Isaacson [:eeejay] from comment #11)
> 
> > > I probably was unclear. I meant, if 
> > > (indexInParent == static_cast<int32_t>(insertIdx) - 1 &&
> > > !child->IsRelocated()) is true
> > > then you still get that assertion? If so, then the bug is not fixed?
> > 
> > No, you don't get the assertion in that case with this patch. This patch
> > assures that we don't move the child in that case.
> 
> sorry I don't follow, I hope the code complexity will be my excuse :)
> 
> so your patch handles the condition, when it's true:
> indexInParent == static_cast<int32_t>(insertIdx) - 1 && child->IsRelocated()
> 
> I miss why
> indexInParent == static_cast<int32_t>(insertIdx) - 1 &&
> !child->IsRelocated()) is never true
> 
> or if it's ever true, then why it doesn't assert for 'no move case' too.

ah, here's why you don't get 'no move case' assertion in MoveChild() method:
MOZ_ASSERT(aChild->IndexInParent() != aIdxInParent, "No move case");
for the case <div aria-owns='child'><div id='child'></div></div>

It is because aChild->IndexInParent() is 0, while aIdxInParent == 1. I think we should have an assertion in Accessible::MoveChild :
MOZ_ASSERT(aChild->IndexInParent() != aIdxInParent - 1 || aIdxInParent == mChildren.Length(), '')
to catch this case.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 8887124 [details] [diff] [review]
Check if we are re-aria-owning a child into its current index. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2114,5 @@
> +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'.
> +      // The child was previously relocated, and it should be in the 'owned'
> +      // array at this index.
> +      if (indexInParent == static_cast<int32_t>(insertIdx)) {
> +        MOZ_ASSERT(child->IsRelocated());

it'd be good to move (or copy) the comment above to MOZ_ASSERT, it will be more easier to understand the assertion, for example:
MOZ_ASSERT(child->IsRelocated(), 'A child having an index in parent from aria ownded indices range, has to be aria owned ');

Also while we are here: SafeElementAt looks safer than ElementAt for assertions :) and the assertion message may benefit from its tweaking:
MOZ_ASSERT(owned->SafeElementAt(idx) == child, 'Unexpected child in ARIA ownned array');

@@ +2124,5 @@
> +      // The child is being inserted directly after its current index.
> +      // This happens when a parent aria-owns its last ordinal child:
> +      // <ul aria-owns='id2'><li id='id1'></li><li id='id2'></li></ul>
> +      if (indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> +        MOZ_ASSERT(!child->IsRelocated());

same here, an assertion message is wanted

::: accessible/tests/browser/tree/browser_test_aria_owns.js
@@ +6,5 @@
> +
> +function childrenIds(acc) {
> +  let ids = [];
> +
> +  for (let i=0; i < acc.childCount; i++) {

nit: spaces around =

@@ +15,5 @@
> +}
> +
> +function testChildren(acc, expectedIds) {
> +  is(JSON.stringify(childrenIds(acc)), JSON.stringify(expectedIds),
> +    `Expected children for "${getAccessibleDOMNodeID(acc)}" was wrong`);

I find an explicit style more appealing:

is(acc.childCount, expectedIdx.length, 'Blabla');
for (let i = 0; i < expectedIds.length, i++) {
  is(getAccessibleDOMNodeID(acc.getChildAt(i)), expectedIdx[i], 'Blabla');
}

which is a bit shorter. If you want to remove extra noise from testing logs, then is() may be replaced on if (!) { ok(false); }

Comment 16

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #15)
> @@ +15,5 @@
> > +}
> > +
> > +function testChildren(acc, expectedIds) {
> > +  is(JSON.stringify(childrenIds(acc)), JSON.stringify(expectedIds),
> > +    `Expected children for "${getAccessibleDOMNodeID(acc)}" was wrong`);
> 
> I find an explicit style more appealing:
> 
> is(acc.childCount, expectedIdx.length, 'Blabla');
> for (let i = 0; i < expectedIds.length, i++) {
>   is(getAccessibleDOMNodeID(acc.getChildAt(i)), expectedIdx[i], 'Blabla');
> }
> 
> which is a bit shorter. If you want to remove extra noise from testing logs,
> then is() may be replaced on if (!) { ok(false); }

I like the JSONified output in the logs for clarity, eg. "expected ["a", "b", "c"], but got ["a", "c"].
I can put that in the output though, and not use JSON to compare.

Comment 17

[alexander :surkov (:asurkov)]

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

> > which is a bit shorter. If you want to remove extra noise from testing logs,
> > then is() may be replaced on if (!) { ok(false); }
> 
> I like the JSONified output in the logs for clarity, eg. "expected ["a",
> "b", "c"], but got ["a", "c"].
> I can put that in the output though, and not use JSON to compare.

It is nice for sure as long as arrays are short and readable. Otherwise you might need something like
expected "b" in ["a", "b", "c"] at 1 index, but got "c" in ["a", "c"].

Also you don't have to compare JSONified values, but you are free to provide any error message you like. For example,

for (let i = 0; i < expectedIds.length, i++) {
  let got = getAccessibleDOMNodeID(acc.getChildAt(i));
  let expected = expectedIdx[i];
  if (got != excpected) {
    ok(false, `Expected "${expected}" in ["a", "b", "c"] at ${i} index, but got "${got}" in ["a", "c"]`);
  }
}

Comment 18

[Eitan Isaacson [:eeejay]]

Attachment: Don't move child with aria-own right after itself. r?surkov

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8887124 [details] [diff] [review]
Check if we are re-aria-owning a child into its current index. r?surkov

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

Alex asked for some feedback on the tests in hopes to further improve our testing framework, hope that's alright.

::: accessible/tests/browser/tree/browser_test_aria_owns.js
@@ +23,5 @@
> +  let one = findAccessibleChildByID(accDoc, "one");
> +  let two = findAccessibleChildByID(accDoc, "two");
> +  let three = findAccessibleChildByID(accDoc, "three");
> +
> +  testChildren(one, ["a"]);

Do you think we can reuse (expand if necessary) testAccessibleTree function that we use in tree update tests. so we just call it on doc or something and match ids?

@@ +27,5 @@
> +  testChildren(one, ["a"]);
> +  testChildren(two, ["b", "c", "d"]);
> +  testChildren(three, []);
> +
> +  let onReorders = waitForEvents(

the more I see this syntax, the more it feels like having a JSON structure here (with named properties) would make it more readable (we would at least not need to annotate things like // expected, // unexpected).

@@ +43,5 @@
> +  });
> +
> +  await onReorders;
> +
> +  testChildren(one, ["a"]);

It would be the same here (testAccessibleTree)

@@ +62,5 @@
> +  });
> +
> +  await onReorders;
> +
> +  testChildren(one, ["a"]);

and here It would be the same here (testAccessibleTree)

Comment 20

[alexander :surkov (:asurkov)]

Comment on attachment 8887580 [details] [diff] [review]
Don't move child with aria-own right after itself. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2112,5 @@
> +
> +      // The child is being placed in its current index,
> +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'.
> +      if (indexInParent == static_cast<int32_t>(insertIdx)) {
> +        MOZ_ASSERT(child->IsRelocated(), "Child was previously relocated");

It will assert with 'child was previously relocated' message when child->IsRelocated()==false. If you didn't like to take my message as a base, then at least 'has to be' instead 'was' seems more correct

@@ +2124,5 @@
> +      // This happens when a parent aria-owns its last ordinal child:
> +      // <ul aria-owns='id2'><li id='id1'></li><li id='id2'></li></ul>
> +      if (indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> +        MOZ_ASSERT(!child->IsRelocated(), "Child should be in its ordinal position");
> +        continue;

We should put it into aria-owned cache for consistence and mark it as IsRelocated. Otherwise, when you do

ul.appendChild(document.createEleemnt('li')) then

children ordering will be wrong.

It'd be great to have a test case for this case as well.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Yura Zenevich [:yzen] from comment #19)
> Alex asked for some feedback on the tests in hopes to further improve our
> testing framework, hope that's alright.

yeah, I would definitely delegate the test part to Yura for this one and focus on c++ part if that's ok ;)

Comment 22

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #20)
> Comment on attachment 8887580 [details] [diff] [review]
> Don't move child with aria-own right after itself. r?surkov
> 
> Review of attachment 8887580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2112,5 @@
> > +
> > +      // The child is being placed in its current index,
> > +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'.
> > +      if (indexInParent == static_cast<int32_t>(insertIdx)) {
> > +        MOZ_ASSERT(child->IsRelocated(), "Child was previously relocated");
> 
> It will assert with 'child was previously relocated' message when
> child->IsRelocated()==false. If you didn't like to take my message as a
> base, then at least 'has to be' instead 'was' seems more correct
> 

Oops, I missed your suggested language.

> @@ +2124,5 @@
> > +      // This happens when a parent aria-owns its last ordinal child:
> > +      // <ul aria-owns='id2'><li id='id1'></li><li id='id2'></li></ul>
> > +      if (indexInParent == static_cast<int32_t>(insertIdx) - 1) {
> > +        MOZ_ASSERT(!child->IsRelocated(), "Child should be in its ordinal position");
> > +        continue;
> 
> We should put it into aria-owned cache for consistence and mark it as
> IsRelocated. Otherwise, when you do
> 
> ul.appendChild(document.createEleemnt('li')) then
> 
> children ordering will be wrong.
> 
> It'd be great to have a test case for this case as well.

big 'doh!

This requires fixing bits in the PutChildrenBack end as well, so I will add that to this patch.

Comment 23

[Eitan Isaacson [:eeejay]]

Attachment: Don't move/reclaim aria-owned children to their current position. r?surkov r?yzen

Comment 24

[Eitan Isaacson [:eeejay]]

This patch fixes bug 1376214 as well.

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8887710 [details] [diff] [review]
Don't move/reclaim aria-owned children to their current position. r?surkov r?yzen

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

This is nice.
I still am not 100% sure on [EVENT, id] format for listing events (I would prefer { type: EVENT, target or criteria: id } but we can continue this conversation outside of this bug.

::: accessible/tests/browser/shared-head.js
@@ +359,5 @@
>    return accessible;
>  }
> +
> +function arrayFromChildren(accessible) {
> +  return Array.from(Array(accessible.childCount)).map(

I believe we can do:

return Array.from({ length: accessible.childCount }, (c, i) =>
  accessible.getChildAt(i));

Map is a second arg to 'from' and first arg can be an object with length

Comment 26

[alexander :surkov (:asurkov)]

Comment on attachment 8887710 [details] [diff] [review]
Don't move/reclaim aria-owned children to their current position. r?surkov r?yzen

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

::: accessible/generic/DocAccessible.cpp
@@ +2113,5 @@
> +      // The child is being placed in its current index,
> +      // eg. aria-owns='id1 id2 id3' is changed to aria-owns='id3 id2 id1'.
> +      if (indexInParent == static_cast<int32_t>(insertIdx)) {
> +        MOZ_ASSERT(child->IsRelocated(),
> +          "A child having an index in parent from aria ownded indices range, has to be aria owned");

nit: comma is missed after 'child'

@@ +2115,5 @@
> +      if (indexInParent == static_cast<int32_t>(insertIdx)) {
> +        MOZ_ASSERT(child->IsRelocated(),
> +          "A child having an index in parent from aria ownded indices range, has to be aria owned");
> +        MOZ_ASSERT(owned->ElementAt(idx) == child,
> +          "Unexpected child in ARIA owned array");

nit: aligning

@@ +2120,5 @@
> +        idx++;
> +        continue;
> +      }
> +
> +      // The child is being inserted directly after its current index.

worth to add 'No move case' or more extended comment pointing that we don't want to move a child

@@ +2121,5 @@
> +        continue;
> +      }
> +
> +      // The child is being inserted directly after its current index.
> +      // This happens when a parent aria-owns its last ordinal child:

might want to split this sentence with previous one, like
, what happens if the child is a last ordinal child.

@@ +2217,5 @@
> +    // ... or receding adopted children were just reclaimed, eg:
> +    //    given:      <ul id="list"><li id="b"></li></ul>
> +    //    after load: $("list").setAttribute("aria-owns", "a b");
> +    //    later:      $("list").setAttribute("aria-owns", "");
> +    if (origContainer != owner || child->IndexInParent() != idxInParent) {

not sure I follow it, are these examples not covered by above checks?

Comment 27

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #26)
> might want to split this sentence with previous one, like
> , what happens if the child is a last ordinal child.
> 

Not sure what you are suggesting here. I reworded it a bit, and used simple language.

> @@ +2217,5 @@
> > +    // ... or receding adopted children were just reclaimed, eg:
> > +    //    given:      <ul id="list"><li id="b"></li></ul>
> > +    //    after load: $("list").setAttribute("aria-owns", "a b");
> > +    //    later:      $("list").setAttribute("aria-owns", "");
> > +    if (origContainer != owner || child->IndexInParent() != idxInParent) {
> 
> not sure I follow it, are these examples not covered by above checks?

These are the two cases where we may have ended up with the child already where it is supposed to be in the "put back" phase.

Comment 28

[Eitan Isaacson [:eeejay]]

Attachment: Don't move/reclaim aria-owned children to their current position. r?surkov

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #27)
> (In reply to alexander :surkov from comment #26)
> > might want to split this sentence with previous one, like
> > , what happens if the child is a last ordinal child.
> > 
> 
> Not sure what you are suggesting here. I reworded it a bit, and used simple
> language.

thanks :) still I'd love to make things more easier for understanding like 'when a parent aria-owns its last ordinal child'. Maybe 'when a last referred ID of an aria-owns element is a last child of the aria-owns element'?

> > @@ +2217,5 @@
> > > +    // ... or receding adopted children were just reclaimed, eg:
> > > +    //    given:      <ul id="list"><li id="b"></li></ul>
> > > +    //    after load: $("list").setAttribute("aria-owns", "a b");
> > > +    //    later:      $("list").setAttribute("aria-owns", "");
> > > +    if (origContainer != owner || child->IndexInParent() != idxInParent) {
> > 
> > not sure I follow it, are these examples not covered by above checks?
> 
> These are the two cases where we may have ended up with the child already
> where it is supposed to be in the "put back" phase.

right, I missed this is for PutChildrenBack aprt

Comment 30

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #29)

> thanks :) still I'd love to make things more easier for understanding like
> 'when a parent aria-owns its last ordinal child'. Maybe 'when a last
> referred ID of an aria-owns element is a last child of the aria-owns
> element'?

first referred ID of course

Comment 31

[alexander :surkov (:asurkov)]

Comment on attachment 8888555 [details] [diff] [review]
Don't move/reclaim aria-owned children to their current position. r?surkov

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

r=me for c++ part, thanks for digging it through, it was tough, at least for me :)

::: accessible/generic/DocAccessible.cpp
@@ +2213,5 @@
>        }
>      }
> +
> +    // The child may have already be in its ordinal place for 2 reasons:
> +    // 1. It was in the first aria-owns child, and the last ordinal.

maybe: a last child is a first aria-owned child? btw, I sort of like aria-owned child over aria-owns child

@@ +2226,5 @@
> +    } else {
> +      MOZ_DIAGNOSTIC_ASSERT(!child->PrevSibling() || !child->PrevSibling()->IsRelocated(),
> +        "No relocated child should appear before this one");
> +      MOZ_DIAGNOSTIC_ASSERT(!child->NextSibling() || child->NextSibling()->IsRelocated(),
> +        "No ordinal child should appear after this one");

I would go with MOZ_ASSERT until you are catching some real problem here

Comment 32

[Eitan Isaacson [:eeejay]]

Attachment: Don't move/reclaim aria-owned children to their current position. r=surkov

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d14aaceb2f6
Don't move/reclaim aria-owned children to their current position. r=surkov, r=yzen

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d14aaceb2f6

Comment 35

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this goes back a ways. Is there a user impact that warrants backport consideration here or can it ride the 56 train?

Comment 36

[Eitan Isaacson [:eeejay]]

I don't think it is worth backporting. This addresses edge cases that got discovered with a random fuzz test that doesn't have real world use.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #36)
> I don't think it is worth backporting. This addresses edge cases that got
> discovered with a random fuzz test that doesn't have real world use.

you'll probably need to backport it for the sec crashes patches that lies on top of it like bug 1363723