758072 - Pivot fails to traverse backwards to previous sibling if current node does not match rule

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Attachment: Test case

Here is a test case that fails in AccessFu. When traversing forwards you go 'WTF', 'Hello', 'World'. When traversing backwards you go 'World', 'WTF'.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Fix backward sibling traversal in nsIAccessiblePivot.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 626641 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot.

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

it seems right but I had some hard time to read that code. Some things:

1) searchCurrent and rv are arguments names
2) 
while (idxInParent > 0) {
  if (!(accessible = parent->GetChildAt(--idxInParent)))
    continue;

is never null since --idxInParent >= 0 (at least after the patch). If I miss something and it can be null and that's expected then it's better to rework this code.

3) assigning under if is really hard to read like in example above or
if (!(accessible = parent))

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> Comment on attachment 626641 [details] [diff] [review]
> Fix backward sibling traversal in nsIAccessiblePivot.
> 
> Review of attachment 626641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> it seems right but I had some hard time to read that code. Some things:
> 
> 1) searchCurrent and rv are arguments names
> 2) 
> while (idxInParent > 0) {
>   if (!(accessible = parent->GetChildAt(--idxInParent)))
>     continue;
> 
> is never null since --idxInParent >= 0 (at least after the patch). If I miss
> something and it can be null and that's expected then it's better to rework
> this code.
> 
> 3) assigning under if is really hard to read like in example above or
> if (!(accessible = parent))

I don't think if (!(foo = br)) is any different from while (foo = bar), but its seems like that code should probably use PrevSibling() and of course its not clear to me what the null check that is never true does.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> > Comment on attachment 626641 [details] [diff] [review]
> > Fix backward sibling traversal in nsIAccessiblePivot.
> > 
> > Review of attachment 626641 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > it seems right but I had some hard time to read that code. Some things:
> > 
> > 1) searchCurrent and rv are arguments names
> > 2) 
> > while (idxInParent > 0) {
> >   if (!(accessible = parent->GetChildAt(--idxInParent)))
> >     continue;
> > 
> > is never null since --idxInParent >= 0 (at least after the patch). If I miss
> > something and it can be null and that's expected then it's better to rework
> > this code.
> > 
> > 3) assigning under if is really hard to read like in example above or
> > if (!(accessible = parent))
> 
> I don't think if (!(foo = br)) is any different from while (foo = bar), but
> its seems like that code should probably use PrevSibling() and of course its
> not clear to me what the null check that is never true does.

From https://bugzilla.mozilla.org/show_bug.cgi?id=698823#c106

@@ +307,5 @@
> +      return aNode;
> +  }
> +
> +  while (aNode != mRootNode) {
> +    while (nsAccessible* previousSibling = aNode->PrevSibling()) {

accessible tree is array oriented, it's preferable to walk by indices

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> 2) 
> while (idxInParent > 0) {
>   if (!(accessible = parent->GetChildAt(--idxInParent)))
>     continue;
> 
> is never null since --idxInParent >= 0 (at least after the patch). If I miss
> something and it can be null and that's expected then it's better to rework
> this code.

This is inspired by the equally redundant line here (it should be in the debug block):
http://dxr.lanedo.com/mozilla-central/accessible/src/base/nsAccessible.cpp.html#l2665

If a function I call shows "return nsnull", I null check. Even if it is an impossible code path. The fact is that whoever wrote that function entertained the possibility of it returning null is enough.

Not checking it, and hoping that the index in parent is never broken seems wrong to me. Especially since this is a potential crasher.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Fix backward sibling traversal in nsIAccessiblePivot. r=surkov

Renamed searchCurrent to aSearchCurrent.
Renamed rv to aResult.

Comment 7

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f387bc4ee25e

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> > 3) assigning under if is really hard to read like in example above or
> > if (!(accessible = parent))
> 
> I don't think if (!(foo = br)) is any different from while (foo = bar)

I'm ok with while ((assigning_statement)), we use it from time to time but we don't use if ((assignment_statement)) and I don't like it, I don't consider that as a good style. If you wish then for example, you don't like ++idx which is pretty fine with me.

Comment 9

[Ed Morley [:emorley]]

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