716865 - atk's nsAccessibleWrap::FireShowHideEvent() makes bad assumptions about the event target

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

in nsDocAccessible::UpdateTreeInternal we fire delayed event for show or hide, them remove the accessible from its parent.  Then later we go to fire the event, where FireShowHideEvent() wants to get the parent atk object and its index in the parent, but of course these are no longer available.  I can think of a couple  fixes.

The simplest thing would be to put the index in parent in the event (the parent is already) and provide accessor functions to get at them.

An alternative is to fire the events at a different time, maybe by over riding BindToParent() and UnbindFromParent()? I believe this wouldn't break the is from user input detection that atk also assumes works here.

Note this causes a bunch of assertions when running mochitest-a11y

Comment 1

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> in nsDocAccessible::UpdateTreeInternal we fire delayed event for show or
> hide, them remove the accessible from its parent.  Then later we go to fire
> the event, where FireShowHideEvent() wants to get the parent atk object and
> its index in the parent, but of course these are no longer available.  I can
> think of a couple  fixes.
> 
> The simplest thing would be to put the index in parent in the event (the
> parent is already) and provide accessor functions to get at them.

that's the way to go hoping the coalescence works fine (while it's doesn't) but that's better than we have now.

it appears there's no consumers of correct behavior?

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

Comment on attachment 600621 [details] [diff] [review]
patch

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +1414,5 @@
> +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> +  g_signal_emit_by_name(parent, signal_name, indexInParent, aObject, NULL);
> +  g_free(signal_name);
> +  atk_object_set_parent(aObject, parent);

this doesn't look equivalent to removed set_parent call because we deal here with trees, not single object. So say you appended a subtree then you will set a parent for root of subtree only.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> Comment on attachment 600621 [details] [diff] [review]
> patch
> 
> Review of attachment 600621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/atk/nsAccessibleWrap.cpp
> @@ +1414,5 @@
> > +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> > +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> > +  g_signal_emit_by_name(parent, signal_name, indexInParent, aObject, NULL);
> > +  g_free(signal_name);
> > +  atk_object_set_parent(aObject, parent);
> 
> this doesn't look equivalent to removed set_parent call because we deal here
> with trees, not single object. So say you appended a subtree then you will
> set a parent for root of subtree only.

yeah, forgot to see what coalescing of mutation events we did.  Doesn't this mean that we don't fire children_changed when we should?

Comment 5

[alexander :surkov (:asurkov)]

Does atk (maybe atk2) really want this? iirc they deal on subtrees, no?

btw, not because of coalescence, we just deal with subtrees and always dealt on subtrees iirc.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander :surkov from comment #3)
> > Comment on attachment 600621 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 600621 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/atk/nsAccessibleWrap.cpp
> > @@ +1414,5 @@
> > > +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> > > +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> > > +  g_signal_emit_by_name(parent, signal_name, indexInParent, aObject, NULL);
> > > +  g_free(signal_name);
> > > +  atk_object_set_parent(aObject, parent);
> > 
> > this doesn't look equivalent to removed set_parent call because we deal here
> > with trees, not single object. So say you appended a subtree then you will
> > set a parent for root of subtree only.
> 
> yeah, forgot to see what coalescing of mutation events we did.  Doesn't this
> mean that we don't fire children_changed when we should?

on the other hand I guess I could add back the set_parent() call with a check that atkObj->parent was previously null

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> Does atk (maybe atk2) really want this? iirc they deal on subtrees, no?

I'm not sure http://developer.gnome.org/atk/stable/AtkObject.html#AtkObject-children-changed would seem to suggest you should fire an event per accessible added to the tree.

> btw, not because of coalescence, we just deal with subtrees and always dealt
> on subtrees iirc.

ok,  though at a quick look it appears we coalesce changes to the same subtree which would certainly cause problems.

Comment 8

[alexander :surkov (:asurkov)]

If the idea to build atk tree lazily then yes, we should set it up whenever it's null (or when different?). 

So the question is can we communicate to atk like call set_parent at unsafe time (for example when layout is not done). If we can't the question is when and how we can update atk tree.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)
> (In reply to alexander :surkov from comment #5)
> > Does atk (maybe atk2) really want this? iirc they deal on subtrees, no?
> 
> I'm not sure
> http://developer.gnome.org/atk/stable/AtkObject.html#AtkObject-children-
> changed would seem to suggest you should fire an event per accessible added
> to the tree.

iirc, that's something that ginn talked about, maybe I'm wrong

> > btw, not because of coalescence, we just deal with subtrees and always dealt
> > on subtrees iirc.
> 
> ok,  though at a quick look it appears we coalesce changes to the same
> subtree which would certainly cause problems.

sure it is, I meant you don't need to catch coalescence (which is not super frequent) to get a problem

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> > (In reply to alexander :surkov from comment #5)
> > > Does atk (maybe atk2) really want this? iirc they deal on subtrees, no?
> > 
> > I'm not sure
> > http://developer.gnome.org/atk/stable/AtkObject.html#AtkObject-children-
> > changed would seem to suggest you should fire an event per accessible added
> > to the tree.
> 
> iirc, that's something that ginn talked about, maybe I'm wrong

I'm not really sure either, I might be wrong too.

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> If the idea to build atk tree lazily then yes, we should set it up whenever
> it's null (or when different?). 

well, we never reparent accessibles so different shouldn't be a case, or do I forget something else?

> So the question is can we communicate to atk like call set_parent at unsafe
> time (for example when layout is not done). If we can't the question is when
> and how we can update atk tree.

I think the main thing we'd want to prevent here is being called into during  time when we don't want to be, so if we can totally prevent atk calling us at times then maybe we should do that otherwise if atk could already call into us then calling atk doesn't save us anything since whatever we do its possible we are called at bad time.

I think it also depends what you mean by "unsafe" if unsafe is accessibles are defunct but not marked as such or only halfway initialized I think we should try and disallow that if we can, but maybe for different definitions of unsafe allowing atk to do things is ok.

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: patch v2

making this simpler would be nice, but this will atleast prevent us fromfiring ccessible-parent events when they clearly aren't needed, and should fix the assertion issue.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> (In reply to alexander :surkov from comment #8)
> > If the idea to build atk tree lazily then yes, we should set it up whenever
> > it's null (or when different?). 
> 
> well, we never reparent accessibles so different shouldn't be a case, or do
> I forget something else?

we do, at least I see assertions. but that's something I want to do, see nsDocAccessible::RecreateAccessible

> > So the question is can we communicate to atk like call set_parent at unsafe
> > time (for example when layout is not done). If we can't the question is when
> > and how we can update atk tree.
> 
> I think the main thing we'd want to prevent here is being called into during
> time when we don't want to be, so if we can totally prevent atk calling us
> at times then maybe we should do that otherwise if atk could already call
> into us then calling atk doesn't save us anything since whatever we do its
> possible we are called at bad time.
>
> I think it also depends what you mean by "unsafe" if unsafe is accessibles
> are defunct but not marked as such or only halfway initialized I think we
> should try and disallow that if we can, but maybe for different definitions
> of unsafe allowing atk to do things is ok.

I meant halfway initialized, when we started update but didn't finished yet. I'd like to avoid when we call into atk during our update and atk calls into us.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 600676 [details] [diff] [review]
patch v2

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

::: accessible/src/atk/nsAccessibleWrap.cpp
@@ +874,3 @@
>      g_object_ref(childAtkObj);
> +    if (childAtkObj->accessible_parent != aAtkObj->accessible_parent)
> +        atk_object_set_parent(childAtkObj, aAtkObj);

nit: not nice but two space indentation please

@@ +1400,5 @@
> +  AtkObject* parent = nsnull;
> +  if (aIsAdded) {
> +    MAI_LOG_DEBUG(("\n\nReceived: Show event\n"));
> +    indexInParent = getIndexInParentCB(aObject);
> +    parent = getParentCB(aObject);

I'm still uncertain about whether we do lazy atk tree creation. I'd like to be consistent if possible

@@ +1414,4 @@
>  
> +  bool isFromUserInput = aEvent->IsFromUserInput();
> +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);

nit: char*, not char *

@@ +1416,5 @@
> +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> +  g_signal_emit_by_name(parent, signal_name, indexInParent, aObject, NULL);
> +  g_free(signal_name);
> +  atk_object_set_parent(aObject, parent);

for show event it dupes the logic since getParentCB did this trick already
for hide event does somebody remove the parent or you just make sure it's set up? I guess atk needs a valid parent for hide event processing, correct?

btw, what do you try to fix here? I mean all you can do is a guess so who relies on this guess and what happens if the guess wasn't correct?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #13)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > (In reply to alexander :surkov from comment #8)
> > > If the idea to build atk tree lazily then yes, we should set it up whenever
> > > it's null (or when different?). 
> > 
> > well, we never reparent accessibles so different shouldn't be a case, or do
> > I forget something else?
> 
> we do, at least I see assertions. but that's something I want to do, see
> nsDocAccessible::RecreateAccessible

ok, I don't really count assertions since designing around brokenness seems like it will lead to pain, but wanting to support reparenting is a good enough reason for me.

> > > So the question is can we communicate to atk like call set_parent at unsafe
> > > time (for example when layout is not done). If we can't the question is when
> > > and how we can update atk tree.
> > 
> > I think the main thing we'd want to prevent here is being called into during
> > time when we don't want to be, so if we can totally prevent atk calling us
> > at times then maybe we should do that otherwise if atk could already call
> > into us then calling atk doesn't save us anything since whatever we do its
> > possible we are called at bad time.
> >
> > I think it also depends what you mean by "unsafe" if unsafe is accessibles
> > are defunct but not marked as such or only halfway initialized I think we
> > should try and disallow that if we can, but maybe for different definitions
> > of unsafe allowing atk to do things is ok.
> 
> I meant halfway initialized, when we started update but didn't finished yet.
> I'd like to avoid when we call into atk during our update and atk calls into
> us.

sure

> @@ +1400,5 @@
> > +  AtkObject* parent = nsnull;
> > +  if (aIsAdded) {
> > +    MAI_LOG_DEBUG(("\n\nReceived: Show event\n"));
> > +    indexInParent = getIndexInParentCB(aObject);
> > +    parent = getParentCB(aObject);
> 
> I'm still uncertain about whether we do lazy atk tree creation. I'd like to
> be consistent if possible

I'm not completely sure, but my feeling is its just a mess

> @@ +1414,4 @@
> >  
> > +  bool isFromUserInput = aEvent->IsFromUserInput();
> > +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> > +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> 
> nit: char*, not char *

yeah, didn't notice since I was just moving stuff around and it sounds the same way either way.

> @@ +1416,5 @@
> > +  char *signal_name = g_strconcat(aIsAdded ? "children_changed::add" :  "children_changed::remove",
> > +                                  isFromUserInput ? "" : kNonUserInputEvent, NULL);
> > +  g_signal_emit_by_name(parent, signal_name, indexInParent, aObject, NULL);
> > +  g_free(signal_name);
> > +  atk_object_set_parent(aObject, parent);
> 
> for show event it dupes the logic since getParentCB did this trick already

I'd sort of like to get rid of that logic in GetParent()

> for hide event does somebody remove the parent or you just make sure it's
> set up? I guess atk needs a valid parent for hide event processing, correct?

well, it wants a parent, not sure what happens if it doesn't have one though we've been giving it null for a while and I'm not aware of it causing any pain.

> btw, what do you try to fix here? I mean all you can do is a guess so who
> relies on this guess and what happens if the guess wasn't correct?

the assertions mentioned in the description, and while I was here I tried to reduce the number of times we cause accessible-parent signals to be emmitted when atk_object_set_parent() is called, though I think this second issue may be better handled on the atk side, since it will help more apps than just us if atk_object_set_parent() ensures the parent has changed before firing an event.

I'm not sure what exactly atk does with this information, Mike can you explain?

Comment 16

[Ginn Chen]

Even if it is going to be fixed in atk, I think we'd better still do a check in Mozilla side, for users who're using old atk libraries.

Comment 17

[alexander :surkov (:asurkov)]

Ginn, so we need to fire "children_add"/"children_remove" for each container accessible in the changed subtree? If so why we don't do that when accessible tree is created initially (all we do is fire lazily the parent_changed signal by atk_object_set_parent call)?

Btw, why do we need to do atk_object_set_parent at all? That sounds like inconsistency because atk doesn't provide similar method to set up children. In Gecko (current state) parent is never changed but children can be appended or removed to/from parent.

Comment 18

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #17)
> In Gecko (current state) parent is never changed but children can be
> appended or removed to/from parent.

well, that's how I used to treat the tree changes in Gecko :) Anyway ATK dupes a tree change by parent_changed and children_changed signals. What's a reason?

Comment 19

[Ginn Chen]

(In reply to alexander :surkov from comment #17)
> Ginn, so we need to fire "children_add"/"children_remove" for each container
> accessible in the changed subtree? If so why we don't do that when
> accessible tree is created initially (all we do is fire lazily the
> parent_changed signal by atk_object_set_parent call)?

In old days, we do atk a11y in a lazily way.
We don't create atk object until it is being used.
So we don't set parent until someone is asking or we use the parent atk object.

> 
> Btw, why do we need to do atk_object_set_parent at all? That sounds like
> inconsistency because atk doesn't provide similar method to set up children.
> In Gecko (current state) parent is never changed but children can be
> appended or removed to/from parent.

I don't have the answer.

BTW: When I drag a tab out for a new window, do we recreate every a11y objects for the tab?