Bug 68142 – AccessibilityRenderObject::previousSibling is not consistent with nextSibling

• Status: UNCONFIRMED
• Component: Accessibility

[Dominic Mazzoni]

I replaced the last line of AccessibilityRenderObject::nextSibling with a check that the previous sibling of the new "next" sibling is equal to this, as follows:

    AccessibilityObject* axNextSibling = axObjectCache()->getOrCreate(nextSibling);
    ASSERT(axNextSibling->previousSibling() == this);
    return axNextSibling;

This resulted in assertion failures in 7 different tests:

Regressions: Unexpected DumpRenderTree crashes : (7)
  accessibility/adjacent-continuations-cause-assertion-failure.html = CRASH
  accessibility/aria-hidden-updates-alldescendants.html = CRASH
  accessibility/div-within-anchors-causes-crash.html = CRASH
  accessibility/duplicate-child-nodes.html = CRASH
  accessibility/image-link-inline-cont.html = CRASH
  accessibility/inline-continuations.html = CRASH
  accessibility/removed-anonymous-block-child-causes-crash.html = CRASH

If previousSibling and nextSibling agreed, this assertion should not fail.

The severity of this is quite low, since most ports do not appear to use previousSibling at all - but it's surely worth fixing.

[chris fleizach]

do you think these errors where here before your changes and you just exposed them?

agreed severity is low, but it's odd they don't match up

[Dominic Mazzoni]

(In reply to comment #1)
> do you think these errors where here before your changes and you just exposed them?
> 
> agreed severity is low, but it's odd they don't match up

Yes, I confirmed that this assertion would have failed before any of my recent patches. It's possible that these patches made things worse - i.e. it's possible that my patch to previousSibling wasn't correct - but it wasn't 100% correct before.

It actually makes me wonder whether it's worth having a previousSibling method at all. What if instead of having public nextSibling and previousSibling methods, we just compute and store the children of each AccessibilityRenderObject in addChildren?

Searching through the codebase, it looks like the almost all of the times nextSibling is called, it's actually just used to iterate over the children of a parent node - so we could replace that with a loop called on the parent node rather than a loop from the first child over its siblings.

The exception seems to be in GTK, where we need to expose a previousSibling method - but maybe it'd be okay to just replace that one with a slightly more cumbersome implementation (jump to parent, iterate over children to find the one prior to this node).

What do you think?

On the other hand, we can just fix previousSibling and this assertion will make sure it won't break again. :)

[chris fleizach]

(In reply to comment #2)
> (In reply to comment #1)
> > do you think these errors where here before your changes and you just exposed them?
> > 
> > agreed severity is low, but it's odd they don't match up
> 
> Yes, I confirmed that this assertion would have failed before any of my recent patches. It's possible that these patches made things worse - i.e. it's possible that my patch to previousSibling wasn't correct - but it wasn't 100% correct before.
> 
> It actually makes me wonder whether it's worth having a previousSibling method at all. What if instead of having public nextSibling and previousSibling methods, we just compute and store the children of each AccessibilityRenderObject in addChildren?
> 

This seems reasonable enough, it certainly buggier to have two methods (next/previous) trying to deal with continuations.

> The exception seems to be in GTK, where we need to expose a previousSibling method - but maybe it'd be okay to just replace that one with a slightly more cumbersome implementation (jump to parent, iterate over children to find the one prior to this node).
>

It sounds more accurate to check the parent's cached children for this case for sure.

[Radar WebKit Bug Importer]

<rdar://problem/15709598>