Last Comment Bug 758072 - Pivot fails to traverse backwards to previous sibling if current node does not match rule
: Pivot fails to traverse backwards to previous sibling if current node does no...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
: alexander :surkov
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 17:11 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-25 08:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (231 bytes, text/html)
2012-05-23 17:11 PDT, Eitan Isaacson [:eeejay]
no flags Details
Fix backward sibling traversal in nsIAccessiblePivot. (2.72 KB, patch)
2012-05-23 17:13 PDT, Eitan Isaacson [:eeejay]
surkov.alexander: review+
Details | Diff | Splinter Review
Fix backward sibling traversal in nsIAccessiblePivot. r=surkov (7.96 KB, patch)
2012-05-24 11:38 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-05-23 17:11:30 PDT
Created attachment 626638 [details]
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] 2012-05-23 17:13:25 PDT
Created attachment 626641 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot.
Comment 2 alexander :surkov 2012-05-24 05:19:05 PDT
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) 2012-05-24 06:19:04 PDT
(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] 2012-05-24 10:29:54 PDT
(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] 2012-05-24 11:02:28 PDT
(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] 2012-05-24 11:38:41 PDT
Created attachment 626899 [details] [diff] [review]
Fix backward sibling traversal in nsIAccessiblePivot. r=surkov

Renamed searchCurrent to aSearchCurrent.
Renamed rv to aResult.
Comment 7 Eitan Isaacson [:eeejay] 2012-05-24 11:48:46 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f387bc4ee25e
Comment 8 alexander :surkov 2012-05-25 01:32:53 PDT
(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] 2012-05-25 08:32:58 PDT
https://hg.mozilla.org/mozilla-central/rev/f387bc4ee25e

Note You need to log in before you can comment on or make changes to this bug.