mozAccessible shouldn't keep references to expired children

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lsocks, Assigned: lsocks)

Tracking

unspecified
mozilla50
Unspecified
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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.
OS X has overloaded InsertChildAt/RemoveChild method, which are supposed to do this job I guess, but apparently they fail
(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().
(Assignee)

Comment 3

2 years ago
Created attachment 8754252 [details] [diff] [review]
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.
Attachment #8754252 - Flags: review?(surkov.alexander)
(Assignee)

Comment 4

2 years ago
Created attachment 8754253 [details] [diff] [review]
p2 Remove now unneccessary methods
Attachment #8754253 - Flags: review?(surkov.alexander)
(Assignee)

Comment 5

2 years ago
Created attachment 8754255 [details] [diff] [review]
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
Attachment #8754255 - Flags: review?(surkov.alexander)
(Assignee)

Comment 6

2 years ago
Created attachment 8754256 [details] [diff] [review]
p4 Get rid of checks for unignored
Attachment #8754256 - Flags: review?(surkov.alexander)
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
Attachment #8754253 - Flags: review?(surkov.alexander) → review+
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
Attachment #8754252 - Flags: review?(surkov.alexander) → review+
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?
Attachment #8754255 - Flags: review?(surkov.alexander) → review+
Attachment #8754256 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 10

2 years ago
> 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.
(Assignee)

Comment 11

2 years ago
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.
Flags: needinfo?(mzehe)
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.
Flags: needinfo?(mzehe)

Comment 13

2 years ago
Pushed by lorien@lorienhu.com:
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

2 years ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee: nobody → lorien
You need to log in before you can comment on or make changes to this bug.