1254824 - mozAccessible shouldn't keep references to expired children

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Lorien Hu (:lsocks)]

Found a couple of natives that didn't have references back to their parents and showed up as (null) while I was running sanity checks on the tree.

I think the problem is caused by this:

mozAccessibles cache their unignored children in mChildren, which includes their ignored children's unignored children (indirect descendants). When an accessible that has an ignored parent is shut down, invalidateChildren is only run on the direct parent's native. Any other ancestors that have it in their native children will keep references to the expired native object when they should also be invalidated.

Comment 1

[alexander :surkov (:asurkov)]

OS X has overloaded InsertChildAt/RemoveChild method, which are supposed to do this job I guess, but apparently they fail

Comment 2

[Trevor Saunders (:tbsaunde)]

(In reply to Lorien Hu (:lsocks) from comment #0)
> Found a couple of natives that didn't have references back to their parents
> and showed up as (null) while I was running sanity checks on the tree.
> 
> I think the problem is caused by this:
> 
> mozAccessibles cache their unignored children in mChildren, which includes
> their ignored children's unignored children (indirect descendants). When an
> accessible that has an ignored parent is shut down, invalidateChildren is
> only run on the direct parent's native. Any other ancestors that have it in
> their native children will keep references to the expired native object when
> they should also be invalidated.

sounds reasonable, I guess invalidateChildren should be called on the result of GetUnignoredParent().

Comment 3

[Lorien Hu (:lsocks)]

Attachment: p1 Get regular parent/children instead of unignored

After fixing this bug differently and finding several more wrong with the unignored hierarchy, I think the best approach is to just get rid of the concept altogether to eliminate unnecessary complexity. VoiceOver seems to handle this just fine and we don't do this for any other platform. NSAccessibility provides native objects with an ignore flag already.

Comment 4

[Lorien Hu (:lsocks)]

Attachment: p2 Remove now unneccessary methods

Comment 5

[Lorien Hu (:lsocks)]

Attachment: p3 No longer need GetClosestInterestingAccessible

some of these don't call GetObjectOrRepresentedView where it doesn't make sense for the mozAccessible to be a doc anyway

Comment 6

[Lorien Hu (:lsocks)]

Attachment: p4 Get rid of checks for unignored

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 8754253 [details] [diff] [review]
p2 Remove now unneccessary methods

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

cool, it makes sense to get Marco's feedback for possible regressions

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 8754252 [details] [diff] [review]
p1 Get regular parent/children instead of unignored

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

::: accessible/mac/mozAccessible.mm
@@ +686,5 @@
> +    for (uint32_t childIdx = 0; childIdx < childCount; childIdx++) {
> +        mozAccessible* nativeChild = GetNativeFromGeckoAccessible(accWrap->GetChildAt(childIdx));
> +        if (nativeChild)
> +          [mChildren addObject:GetObjectOrRepresentedView(nativeChild)];
> +    }

please fix identation

@@ +694,5 @@
> +      for (uint32_t childIdx = 0; childIdx < childCount; childIdx++) {
> +          mozAccessible* nativeChild = GetNativeFromProxy(proxy->ChildAt(childIdx));
> +          if (nativeChild)
> +            [mChildren addObject:GetObjectOrRepresentedView(nativeChild)];
> +      }

same here

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 8754255 [details] [diff] [review]
p3 No longer need GetClosestInterestingAccessible

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

r=me with comments addressed/answered

::: accessible/mac/mozAccessible.mm
@@ -68,5 @@
> -  }
> -
> -  // if it's a mozAccessible, we need to take care to maybe return the view we
> -  // represent, to the AT.
> -  if ([unignoredObject respondsToSelector:@selector(hasRepresentedView)])

not familiar much with object-c, curious whether this is equivalent to [ unignoredObject hasRepresentedView ]

@@ +498,5 @@
>        nativeChild = GetNativeFromProxy(child);
>    }
>  
>    if (nativeChild)
> +    return nativeChild;

were you supposed to call GetObjectOrRepresentedView or how do you know it doesn't have a view?

@@ +615,5 @@
>      uint32_t childCount = accWrap->ChildCount();
>      for (uint32_t childIdx = 0; childIdx < childCount; childIdx++) {
>          mozAccessible* nativeChild = GetNativeFromGeckoAccessible(accWrap->GetChildAt(childIdx));
>          if (nativeChild)
> +          [mChildren addObject:nativeChild];

can you describe the change please, why no view accessible is expected only?

Comment 10

[Lorien Hu (:lsocks)]

> not familiar much with object-c, curious whether this is equivalent to [
> unignoredObject hasRepresentedView ]

The selector is checking for if the object has that method. This shouldn't be necessary because all mozAccessibles (and ChildView) support hasRepresentedView. Since we don't need to look for 1st unignored parent we don't go up the tree past 1 level so we wouldn't be calling hasRepresentedView on a non-mozAccessible.


> @@ +498,5 @@
> >        nativeChild = GetNativeFromProxy(child);
> >    }
> >  
> >    if (nativeChild)
> > +    return nativeChild;
> 
> were you supposed to call GetObjectOrRepresentedView or how do you know it
> doesn't have a view?

I'll fix this, it was intentional but I just noticed GetChildAtPoint can return its parameter. I originally thought it'd just be a child which couldn't have a view.


> @@ +615,5 @@
> >      uint32_t childCount = accWrap->ChildCount();
> >      for (uint32_t childIdx = 0; childIdx < childCount; childIdx++) {
> >          mozAccessible* nativeChild = GetNativeFromGeckoAccessible(accWrap->GetChildAt(childIdx));
> >          if (nativeChild)
> > +          [mChildren addObject:nativeChild];
> 
> can you describe the change please, why no view accessible is expected only?

The mozRootAccessible, which is the only one that has a represented view, would not be the child of a mozAccessible so any child we get has to return itself and not a view.

Comment 11

[Lorien Hu (:lsocks)]

Marco, since you're much more knowledgeable about OS X, do you think this change will cause any unforeseen issues? Tests are fine but I'm hesitant to push in case there are regressions.

Comment 12

[Marco Zehe (:MarcoZ)]

Sorry for not getting back to you earlier. I think this is safe to land. I tested the try build you gave me in chat, and while the test wasn't extensive, it didn't reveal anything bad.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d86947f6a1ca
Stop filtering ignored mozAccessibles when creating native objects p1 r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/feedbccaf5f2
Stop filtering ignored mozAccessibles when creating native objects p2 r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b419ce5690c
Stop filtering ignored mozAccessibles when creating native objects p3 r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf6a184a26a
Stop filtering ignored mozAccessibles when creating native objects p4 r=surkov

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d86947f6a1ca
https://hg.mozilla.org/mozilla-central/rev/feedbccaf5f2
https://hg.mozilla.org/mozilla-central/rev/3b419ce5690c
https://hg.mozilla.org/mozilla-central/rev/6bf6a184a26a