733712 - Don't call atk_object_set_name

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Chris Coulson]

Not sure whose bug this is, but atk 2.3.91 contains this commit which triggers a stack exhaustion crash in Firefox:

http://git.gnome.org/browse/atk/commit/?id=7ebaa51b17fbca385d9d1f3dd026bd4770852d9b

Firefox overloads AtkObject->get_name() with getNameCB() in nsAccessibleWrap.cpp, and getNameCB() calls atk_object_set_name().

See https://launchpad.net/bugs/948788. We've reverted the atk change in Ubuntu for the time being, but I guess this is going to hit other distro's too

Comment 1

[alexander :surkov (:asurkov)]

Do you have stack crash?

Comment 2

[Chris Coulson]

From the bug in Launchpad: https://launchpadlibrarian.net/95732761/gdb-thunderbird.txt

Comment 3

[alexander :surkov (:asurkov)]

Oh, I see. We do lazy name setting so we do atk_object_set_name whenever we were asked for accessible name (atk_object_get_name). I'm not sure whether we have another option to do this lazily.

In general I think ATK code shouldn't allow the crash even if the server does something wrong. So simple reentrance guard on atk side should help. Alejandro?

Comment 4

[Alejandro Piñeiro]

(In reply to alexander :surkov from comment #3)
> Oh, I see. We do lazy name setting so we do atk_object_set_name whenever we
> were asked for accessible name (atk_object_get_name). I'm not sure whether
> we have another option to do this lazily.

So AFAIU, your implementation of _get_name is calling _set_name, and ATK implementation of _set_name is calling _get_name, right?


> In general I think ATK code shouldn't allow the crash even if the server
> does something wrong. So simple reentrance guard on atk side should help.
> Alejandro?

What kind of reentrance guard? Do you have any example on Firefox code?

Anyway, as this week is a GNOME release week, I think that it would be safer to just revert that change, and make a new release, and we could think on solve this issue later (probably after GNOME 3.4).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Alejandro Piñeiro from comment #4)
> (In reply to alexander :surkov from comment #3)
> > Oh, I see. We do lazy name setting so we do atk_object_set_name whenever we
> > were asked for accessible name (atk_object_get_name). I'm not sure whether
> > we have another option to do this lazily.
> 
> So AFAIU, your implementation of _get_name is calling _set_name, and ATK
> implementation of _set_name is calling _get_name, right?

yes

> > In general I think ATK code shouldn't allow the crash even if the server
> > does something wrong. So simple reentrance guard on atk side should help.
> > Alejandro?
> 
> What kind of reentrance guard? Do you have any example on Firefox code?

sorry for terms. I meant something like this

1015,7 +1015,7 @@ atk_object_set_name (AtkObject *accessible,
if (klass->set_name)
{
  static bool isReentrance = false;
  if (!isReentrance) {
    /* Do not notify for initial name setting. See bug 665870 */
    isReentrance = true;
    notify = (atk_object_get_name (accessible) != NULL);
    isReentrance = false;
  }
}

> Anyway, as this week is a GNOME release week, I think that it would be safer
> to just revert that change, and make a new release, and we could think on
> solve this issue later (probably after GNOME 3.4).

unfortunately that will discover Thunderbird crash

Comment 6

[Alejandro Piñeiro]

FWIW, I have just made a new ATK release:

https://mail.gnome.org/archives/gnome-accessibility-devel/2012-March/msg00004.html

Thanks for pinging me with this issue.

Comment 7

[alexander :surkov (:asurkov)]

after irc chat with Alejandro it seems we don't need to call atk_object_set_name at all. We override atkobject->get_name so we are guaranteed that we always deliver correct accessible name, in other words it doesn't make sense to call atk_object_set_name to change atkobject->accessible->name. Also calling the set_atk_object_name inside of atk_object_get_name could lead to weird things like described by this bug. Also it makes name_changed signal to fire what makes name_changed signal irrelevant (AT asks accessible name and additionally gets name_changed signal).

atkobject->accessible->name is used only by default implementation of get_name/set_name so we shouldn't care to keep accessible->name updated. Also we shouldn't worry to override atkobject->set_name because no one will call it.

changing bug summary

Comment 8

[alexander :surkov (:asurkov)]

just remove atk_object_set_name instances

Comment 9

[Ginn Chen]

We used to use aAtkObj->name as a cache of accessible name, it was long long ago.

Since you said calling atk_object_set_name inside of atk_object_get_name could lead to werid things, I'm wondering whether we should do atk_object_set_parent in getParentCB(), hmm...

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #9)
> Since you said calling atk_object_set_name inside of atk_object_get_name
> could lead to werid things, I'm wondering whether we should do
> atk_object_set_parent in getParentCB(), hmm...

yeah, I thought about the same. But iirc we have a bug for that.

Comment 11

[Ginn Chen]

(In reply to alexander :surkov from comment #10
> yeah, I thought about the same. But iirc we have a bug for that.

You mean bug 730841?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #11)
> (In reply to alexander :surkov from comment #10
> > yeah, I thought about the same. But iirc we have a bug for that.
> 
> You mean bug 730841?

nah, bug 716865, at least it has some related discussion (btw, bug 730841 was a follow up from that bug).

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #8)
> just remove atk_object_set_name instances

I suspect we can do a little more cleanup while here, but I'm not sure atm.

you should also explicitly fire name change events right?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to alexander :surkov from comment #8)
> > just remove atk_object_set_name instances
> 
> I suspect we can do a little more cleanup while here, but I'm not sure atm.

we have only two instances, it sounds like there's nothing to clean up

> you should also explicitly fire name change events right?

oh, right, we should emit a signal under EVENT_NAME_CHANGE case in nsAccessibleWrap::FirePlatformEvent

Comment 15

[Ginn Chen]

I think the solution is
1) We implement both getNameCB and setNameCB, make sure setNameCB won't get into getNameCB.
Same for parent, role, description, etc.
or
2) We never call atk_object_set_*(), we use object->xxx = blahblahblah, in our code.

However, IMHO,
atk_object_set_name should just use accessible->name.
If an implementation need to call abstract atk_object_get_name instead, it must have implemented atk_object_set_name too.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #15)
> I think the solution is
> 1) We implement both getNameCB and setNameCB, make sure setNameCB won't get
> into getNameCB.

makes sense to be on safe side. I'm fine with that.

> Same for parent, role, description, etc.

I'd deal with them in follow ups. Otherwise it might be not a good first bug at all.

> or
> 2) We never call atk_object_set_*(), we use object->xxx = blahblahblah, in
> our code.

API said that object->xxx shouldn't be public (atk_object->accessible).

> However, IMHO,
> atk_object_set_name should just use accessible->name.

It doesn't sound like it's worth to cache accessible name since we always calculate the name on demand (also it's extra strings copy). In gecko we don't fire name_change event for every changed name. So we get hard times to update accessible->name. It sounds all we can do what we do now when we were asked for a name then update accessible->name.

So I think we should go with 1)

Comment 17

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Ginn Chen from comment #15)
> > I think the solution is
> > 1) We implement both getNameCB and setNameCB, make sure setNameCB won't get
> > into getNameCB.
> 
> makes sense to be on safe side. I'm fine with that.

seems good to me.

> > Same for parent, role, description, etc.
> 
> I'd deal with them in follow ups. Otherwise it might be not a good first bug
> at all.

yeah, but each of them can probably be there own good first bug.

Comment 18

[Ginn Chen]

(In reply to alexander :surkov from comment #16)
> > 2) We never call atk_object_set_*(), we use object->xxx = blahblahblah, in
> > our code.
> 
> API said that object->xxx shouldn't be public (atk_object->accessible).
>

I don't think setting object->xxx in getXXXCB is using it as public.
 
> > However, IMHO,
> > atk_object_set_name should just use accessible->name.
> 
> It doesn't sound like it's worth to cache accessible name since we always
> calculate the name on demand (also it's extra strings copy). In gecko we
> don't fire name_change event for every changed name. So we get hard times to
> update accessible->name. It sounds all we can do what we do now when we were
> asked for a name then update accessible->name.
> 
> So I think we should go with 1)

I don't think we should cache it, but I think we should keep accessible->name updated.
We need to copy the string to gchar* anyway.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #18)
> (In reply to alexander :surkov from comment #16)
> > > 2) We never call atk_object_set_*(), we use object->xxx = blahblahblah, in
> > > our code.
> > 
> > API said that object->xxx shouldn't be public (atk_object->accessible).
> >
> 
> I don't think setting object->xxx in getXXXCB is using it as public.

now - it doesn't since we use atk_object_set_name

> > So I think we should go with 1)
> 
> I don't think we should cache it, but I think we should keep
> accessible->name updated.

this means a caching, no? Internally we don't keep calculated name, we always calculated it on demand. Why would we need to keep accessible->name updated and how we are going to do that if we don't use atk_object_set_name?

> We need to copy the string to gchar* anyway.

yep, just copy the string every time when we were asked. Doesn't sound good?

Comment 20

[Ginn Chen]

(In reply to alexander :surkov from comment #19)
> this means a caching, no? Internally we don't keep calculated name, we
> always calculated it on demand. Why would we need to keep accessible->name
> updated and how we are going to do that if we don't use atk_object_set_name?

Just use obj->name = xxx
And we saved g_strdup() here.

> 
> > We need to copy the string to gchar* anyway.
> 
> yep, just copy the string every time when we were asked. Doesn't sound good?

Actually it's not possible.
atk_object_get_name() returns const gchar*, caller will not free the return value.
We have to keep it somewhere in AtkObject, otherwise we leak.

Another benefit is we can easier see what the atk object was in gdb/dbx.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Ginn Chen from comment #20)

> > > We need to copy the string to gchar* anyway.
> > 
> > yep, just copy the string every time when we were asked. Doesn't sound good?
> 
> Actually it's not possible.
> atk_object_get_name() returns const gchar*, caller will not free the return
> value.
> We have to keep it somewhere in AtkObject, otherwise we leak.

I see

So if provide getName/setName implementation that will use accessible->name to store a value then would it be acceptable solution?

Comment 22

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #21)
> (In reply to Ginn Chen from comment #20)
> 
> > > > We need to copy the string to gchar* anyway.
> > > 
> > > yep, just copy the string every time when we were asked. Doesn't sound good?
> > 
> > Actually it's not possible.
> > atk_object_get_name() returns const gchar*, caller will not free the return
> > value.
> > We have to keep it somewhere in AtkObject, otherwise we leak.
> 
> I see
> 
> So if provide getName/setName implementation that will use accessible->name
> to store a value then would it be acceptable solution?

or we could do what we do for a bunch of other atk methods reutrning const gchar* nd use nsAccessibleWrap::ReturnString() which keeps a static nsCString and returns a pointer into its buffer, and so requires caller of atk method to copy returned string before calling another atk function, which I think its very likely the bridge will always do.

Comment 23

[alexander :surkov (:asurkov)]

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

> or we could do what we do for a bunch of other atk methods reutrning const
> gchar* nd use nsAccessibleWrap::ReturnString() which keeps a static
> nsCString and returns a pointer into its buffer, and so requires caller of
> atk method to copy returned string before calling another atk function,
> which I think its very likely the bridge will always do.

I think I would try to avoid assumptions like this. Ginn?

Comment 24

[JeffreyDecker]

I would like to take on this bug if that's alright. Just reading through the comments though it looks like it's a little unclear as to how we want to refactor to fix the bug. Has there been any decision made? I think this would be an iterating bug to work on as I am still a relative newbie to making fixes to Firefox but have a solid programming background. 

Thanks,
Jeffrey

Comment 25

[alexander :surkov (:asurkov)]

(In reply to JeffreyDecker from comment #24)
> I would like to take on this bug if that's alright.

done, thank you btw.

> Just reading through the
> comments though it looks like it's a little unclear as to how we want to
> refactor to fix the bug.

I think go with solution from comment #21.

> Has there been any decision made? 

Ginn, Trevor, does comment #21 approach sounds acceptable for you?

Comment 26

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #25)
> (In reply to JeffreyDecker from comment #24)
> > I would like to take on this bug if that's alright.
> 
> done, thank you btw.
> 
> > Just reading through the
> > comments though it looks like it's a little unclear as to how we want to
> > refactor to fix the bug.
> 
> I think go with solution from comment #21.
> 
> > Has there been any decision made? 
> 
> Ginn, Trevor, does comment #21 approach sounds acceptable for you?

seems fine.

Comment 27

[JeffreyDecker]

Just as clarification I need to first write getName and setName and then replace the instances of atk_object_get_name and atk_object_set_name with the new function. Just want to make sure before I get started.

Thank you.

Comment 28

[Ginn Chen]

(In reply to JeffreyDecker from comment #27)
> Just as clarification I need to first write getName and setName and then
> replace the instances of atk_object_get_name and atk_object_set_name with
> the new function. Just want to make sure before I get started.
> 
> Thank you.


No, I don't think you need to write getName and setName.
You can write setNameCB just like we have getNameCB.
And use obj->name directly in getNameCB and setNameCB.

Comment 29

[Vasil Dimov [:vd]]

Hello,

First of all, this bug does not exist currently because the following commit was reverted from ATK source: https://git.gnome.org/browse/atk/commit/?id=7ebaa51b17fbca385d9d1f3dd026bd4770852d9b

So in order to close this bug, either

1. Leave the code as is and put a comment in ATK source not to call atk_object_get_name() from atk_object_set_name() to avoid future reappearance of this bug, or

2. Change Mozilla's accessible/src/atk/AccessibleWrap.cpp to avoid the problem. I am going to attach a patch for that here shortly. After that the above commit to ATK source may be replayed.

Comment 30

[Vasil Dimov [:vd]]

Attachment: Implement setNameCB() in AccessibleWrap.cpp and use it in getNameCB() instead of atk_object_set_name()

Beware - I am not very familiar with this code, I may have screwed it up!

Comment 31

[Trevor Saunders (:tbsaunde)]

Comment on attachment 725350 [details] [diff] [review]
Implement setNameCB() in AccessibleWrap.cpp and use it in getNameCB() instead of atk_object_set_name()

>@@ -644,11 +644,40 @@ getNameCB(AtkObject* aAtkObj)
> 
>   NS_ConvertUTF8toUTF16 objName(aAtkObj->name);
>   if (!uniName.Equals(objName))
>-    atk_object_set_name(aAtkObj, NS_ConvertUTF16toUTF8(uniName).get());
>+    setNameCB(aAtkObj, NS_ConvertUTF16toUTF8(uniName).get());

you don't need to implement SetNameCB() or deal with atkobj->name at all you should be able to just do g_object_notify(blah);

>@@ -1011,6 +1040,7 @@ AccessibleWrap::FirePlatformEvent(AccEvent* aEvent)
>         accessible->Name(newName);
>         NS_ConvertUTF16toUTF8 utf8Name(newName);
>         if (!atkObj->name || !utf8Name.Equals(atkObj->name))
>+          /* XXX also use setNameCB() here? */
>           atk_object_set_name(atkObj, utf8Name.get());

just change here too.  Note we should do the same thing for atk_object_set_parent() and atk_object_set_description()

btw I don't really care but most people around here like 8 lines of context in diffs.

Comment 32

[Vasil Dimov [:vd]]

Attachment: Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

Do not call our replacement setNameCB() and do not register it as a callback by assigning it to AtkObjectClass::set_name. Also use it in the other place where we use atk_object_set_name(), as suggested by Trevor Saunders (:tbsaunde).

Our function AtkObjectSetName() does the same as what atk_object_set_name() does, except that it does not call atk_object_get_name() but accesses aAtkObj->name directly.

For reference, atk_object_set_name() is implemented here:
https://git.gnome.org/browse/atk/tree/atk/atkobject.c#n999
If the set_name() member has not been overriden, then it will point to atk_object_real_set_name(), which is implemented here:
https://git.gnome.org/browse/atk/tree/atk/atkobject.c#n1420

Practically this patch is a non-functional change since the _current_ version of atk_object_set_name() does not call atk_object_get_name() either but accesses the name directly. The point is that after this patch hits the tree, then this ATK commit could be replayed: https://git.gnome.org/browse/atk/commit/?id=7ebaa51b17fbca385d9d1f3dd026bd4770852d9b

Comment 33

[alexander :surkov (:asurkov)]

Comment on attachment 726580 [details] [diff] [review]
Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

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

::: i/accessible/src/atk/AccessibleWrap.cpp
@@ +147,5 @@
>  #endif
>  
>  G_BEGIN_DECLS
> +
> +static void AtkObjectSetName(AtkObject *aAtkObj, const gchar *name);

nit: type* name (here and below)

@@ +647,4 @@
>  
>    NS_ConvertUTF8toUTF16 objName(aAtkObj->name);
>    if (!uniName.Equals(objName))
> +    AtkObjectSetName(aAtkObj, NS_ConvertUTF16toUTF8(uniName).get());

nit: you can change the code to use one conversion

@@ +654,5 @@
>  
> +static void
> +AtkObjectSetName(AtkObject *aAtkObj, const gchar *name)
> +{
> +  /* This function duplicates the functionality of atk_object_set_name(),

nit: We use '//' comment style in function body

Comment 34

[Trevor Saunders (:tbsaunde)]

Comment on attachment 726580 [details] [diff] [review]
Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

>+static void
>+AtkObjectSetName(AtkObject *aAtkObj, const gchar *name)

so, both of the times this is called we have to check if we actually want to fire an event first.  So why don't we refactor this stuff like this

make this function
static void
MaybeFireNameChange(AtkObject* aObj, nsString& aNewName)

then it can incapsilate the stuff about comparing against the old name.

>+    size_t name_len = strlen(name);
>+
>+    if (strlen(aAtkObj->name) >= name_len) {
>+      /* If the new name is shorter, then just use the old memory chunk
>+       * to minimize memory fragmentation. */
>+      memcpy(aAtkObj->name, name, name_len + 1);
>+    } else {
>+      g_free(aAtkObj->name);
>+      aAtkObj->name = g_strdup(name);

I'd be suprised if this isn't a win and possibly a loss.  Especially since the best you can do is strlen while the allocator actually knows how big the block is.

otherwise this seems like the correct approach so f=me but I'd like to see another version before r+ :)

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> Comment on attachment 726580 [details] [diff] [review]
> Implement a replacement of atk_object_set_name() which mimics the behavior
> without calling atk_object_get_name()
> 
> >+static void
> >+AtkObjectSetName(AtkObject *aAtkObj, const gchar *name)
> 
> so, both of the times this is called we have to check if we actually want to
> fire an event first.

we don't want an event in the former case

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #35)
> (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > Comment on attachment 726580 [details] [diff] [review]
> > Implement a replacement of atk_object_set_name() which mimics the behavior
> > without calling atk_object_get_name()
> > 
> > >+static void
> > >+AtkObjectSetName(AtkObject *aAtkObj, const gchar *name)
> > 
> > so, both of the times this is called we have to check if we actually want to
> > fire an event first.
> 
> we don't want an event in the former case

not sure what you mean.  both places we call atk_object_set_name() we guard it with something that is more or less oldName != newName

now, the first of those shouldn't be needed in an ideal world because we'd always fire name change event, but we don't do that because its between hard and impossible to do that because name algorithm is complicated and depends on all sorts of stuff.

Comment 37

[alexander :surkov (:asurkov)]

atk_object_set_name() called inside get_name is a wrong thing we try to remove here. It made us fire name change events which is ridiculous I think. If I get right then ATK implementation internals don't need that event as long as we override get_name. On the another hand I don't understand why the consumer might need name change event when it asks for the name.

Comment 38

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #37)
> atk_object_set_name() called inside get_name is a wrong thing we try to

I think the only thing we try to fix here is the infinite recurssion.

> remove here. It made us fire name change events which is ridiculous I think.
> If I get right then ATK implementation internals don't need that event as
> long as we override get_name. On the another hand I don't understand why the
> consumer might need name change event when it asks for the name.

I absolutely agree what we do is crazy, but I know there is caching involved and I'm atleast somewhat concerned not fireing an event in getName() could break that even more than it is now.  If your offering to fix name change events so they're  fired whenever a name changes and never when it doesn't then of course I'm happy to remove this madness.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #38)
> (In reply to alexander :surkov from comment #37)
> > atk_object_set_name() called inside get_name is a wrong thing we try to
> 
> I think the only thing we try to fix here is the infinite recurssion.

and code madnness :)

> > remove here. It made us fire name change events which is ridiculous I think.
> > If I get right then ATK implementation internals don't need that event as
> > long as we override get_name. On the another hand I don't understand why the
> > consumer might need name change event when it asks for the name.
> 
> I absolutely agree what we do is crazy, but I know there is caching involved
> and I'm atleast somewhat concerned not fireing an event in getName() could
> break that even more than it is now.  

what kind of caching? And how does this caching is supposed to work if somebody asks us to calculate the name (bypassing that cache)? Or alternatively who uses that cache and why all consumers don't want to use it?

> If your offering to fix name change
> events so they're  fired whenever a name changes and never when it doesn't
> then of course I'm happy to remove this madness.

iirc AT needs this event every time when name is changed. In this sense our name change event might never work for this purpose.

Comment 40

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #39)
> (In reply to Trevor Saunders (:tbsaunde) from comment #38)
> > (In reply to alexander :surkov from comment #37)
> > > atk_object_set_name() called inside get_name is a wrong thing we try to
> > 
> > I think the only thing we try to fix here is the infinite recurssion.
> 
> and code madnness :)

why? the reason for the bug was crashes with a change to atk, so just fixing the way we interact with atk should be fine.

> > > remove here. It made us fire name change events which is ridiculous I think.
> > > If I get right then ATK implementation internals don't need that event as
> > > long as we override get_name. On the another hand I don't understand why the

I believe it needs event to keep cache in sync with reality.

> > > consumer might need name change event when it asks for the name.

what if there is more than one consumer, then it may be arguably less bad for other consumers to get the event late rather than never.

> > I absolutely agree what we do is crazy, but I know there is caching involved
> > and I'm atleast somewhat concerned not fireing an event in getName() could
> > break that even more than it is now.  
> 
> what kind of caching? And how does this caching is supposed to work if
> somebody asks us to calculate the name (bypassing that cache)? Or
> alternatively who uses that cache and why all consumers don't want to use it?

I think the way it works is that consumer processes have a local repreentation of atkobject in their process which keeps the name and atk updates the name based on name change event.  So each consumer can choose for itself to use or not use cache as it likes, and might well want to not use the cache because it can easily become out of date due to us not always firing name change events.

> > If your offering to fix name change
> > events so they're  fired whenever a name changes and never when it doesn't
> > then of course I'm happy to remove this madness.
> 
> iirc AT needs this event every time when name is changed. In this sense our
> name change event might never work for this purpose.

its possible


in any case I'm not really willing to remove the madness we have now atleast until we talk to atk people about it, and I don't see a reason to make vd block on that.

Comment 41

[alexander :surkov (:asurkov)]

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

> > > I think the only thing we try to fix here is the infinite recurssion.
> > 
> > and code madnness :)
> 
> why? the reason for the bug was crashes with a change to atk, so just fixing
> the way we interact with atk should be fine.

the crash was fixed on atk side, we need to make our side robust.

> > > > consumer might need name change event when it asks for the name.
> 
> what if there is more than one consumer, then it may be arguably less bad
> for other consumers to get the event late rather than never.

it's intermittent failures, it's not the way the software is supposed to work

> I think the way it works is that consumer processes have a local
> repreentation of atkobject in their process which keeps the name and atk
> updates the name based on name change event.  So each consumer can choose
> for itself to use or not use cache as it likes, and might well want to not
> use the cache because it can easily become out of date due to us not always
> firing name change events.

> in any case I'm not really willing to remove the madness we have now atleast
> until we talk to atk people about it, and I don't see a reason to make vd
> block on that.

see comment #7:

"after irc chat with Alejandro it seems we don't need to call atk_object_set_name at all. We override atkobject->get_name so we are guaranteed that we always deliver correct accessible name, in other words it doesn't make sense to call atk_object_set_name to change atkobject->accessible->name."

If you are really sure than some cache exists that we must update then could you please run a testcase with orca or check it with Alejandro?

Comment 42

[Vasil Dimov [:vd]]

Attachment: Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

Comment 43

[Trevor Saunders (:tbsaunde)]

Comment on attachment 728261 [details] [diff] [review]
Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

>diff --git i/accessible/src/atk/AccessibleWrap.cpp w/accessible/src/atk/AccessibleWrap.cpp
>index e35da5d..208e955 100644
>--- i/accessible/src/atk/AccessibleWrap.cpp
>+++ w/accessible/src/atk/AccessibleWrap.cpp
>@@ -142,16 +142,20 @@ struct MaiAtkObjectClass
> static guint mai_atk_object_signals [LAST_SIGNAL] = { 0, };
> 
> #ifdef MAI_LOGGING
> int32_t sMaiAtkObjCreated = 0;
> int32_t sMaiAtkObjDeleted = 0;
> #endif
> 
> G_BEGIN_DECLS
>+
>+static void
>+MaybeFireNameChange(AtkObject *aAtkObj, const nsAutoString& aNewNameUTF16);

there's no reason this needs to be extern "C" which is all the G_DECL thing is hiding is there?

btw type* name

>   nsAutoString uniName;
>   accWrap->Name(uniName);
> 
>-  NS_ConvertUTF8toUTF16 objName(aAtkObj->name);
>-  if (!uniName.Equals(objName))
>-    atk_object_set_name(aAtkObj, NS_ConvertUTF16toUTF8(uniName).get());
>+  // XXX Firing an event from here does not seem right
>+  MaybeFireNameChange(aAtkObj, uniName);

nit, might be nice if you renamed the var to just name since we don't convert it here

>+MaybeFireNameChange(AtkObject* aAtkObj, const nsAutoString& aNewNameUTF16)

nit, utf16 is implied by the type being nsFooString not nsFooCString so kind of redundant

>+{
>+  NS_ConvertUTF8toUTF16 curNameUTF16(aAtkObj->name);

wouldn't it be more efficient to just convert the new name to utf8?

>+
>+  if (aNewNameUTF16.Equals(curNameUTF16)) {
>+    return;
>+  }

nit, no braces

>+  const gchar* name = NS_ConvertUTF16toUTF8(aNewNameUTF16).get();

so, that creates an object that only lives for the statement which is going to mean accessing atkObj->name after this statement is a use after free.

if you convert newName to utf8 at the start you can just do free(atkOjb->name); atkObj->name = strdup(newNameUTF8.get());

>+
>+  // Below we duplicate the functionality of atk_object_set_name(),
>+  // but without calling atk_object_get_name(). Instead of
>+  // atk_object_get_name() we directly access aAtkObj->name. This is because
>+  // atk_object_get_name() would call getNameCB() which would call
>+  // MaybeFireNameChange() (or atk_object_set_name() before this problem was
>+  // fixed) and we would get an infinite recursion.
>+  // See http://bugzilla.mozilla.org/733712
>+
>+  AtkObjectClass *klass;
>+  gboolean notify = FALSE;
>+
>+  g_return_if_fail(ATK_IS_OBJECT(aAtkObj));
>+  g_return_if_fail(name != NULL);
>+
>+  klass = ATK_OBJECT_GET_CLASS(aAtkObj);
>+  if (klass->set_name) {
>+    // Do not notify for initial name setting.
>+    // See bug http://bugzilla.gnome.org/665870
>+    notify = (aAtkObj->name != NULL);
>+
>+    (klass->set_name)(aAtkObj, name);
>+    if (notify) {
>+      g_object_notify(G_OBJECT(aAtkObj), atk_object_name_property_name);
>+    }
>+  }

the only part of this you need is the if notify g_object_notify() bit unless I'm missing something

thanks for helping to clean this up it looks good other than that.

Comment 44

[Vasil Dimov [:vd]]

(In reply to Trevor Saunders (:tbsaunde) from comment #43)
> Comment on attachment 728261 [details] [diff] [review]
> Implement a replacement of atk_object_set_name() which mimics the behavior
> without calling atk_object_get_name()
> 
> >diff --git i/accessible/src/atk/AccessibleWrap.cpp w/accessible/src/atk/AccessibleWrap.cpp
> >index e35da5d..208e955 100644
> >--- i/accessible/src/atk/AccessibleWrap.cpp
> >+++ w/accessible/src/atk/AccessibleWrap.cpp
> >@@ -142,16 +142,20 @@ struct MaiAtkObjectClass
> > static guint mai_atk_object_signals [LAST_SIGNAL] = { 0, };
> > 
> > #ifdef MAI_LOGGING
> > int32_t sMaiAtkObjCreated = 0;
> > int32_t sMaiAtkObjDeleted = 0;
> > #endif
> > 
> > G_BEGIN_DECLS
> >+
> >+static void
> >+MaybeFireNameChange(AtkObject *aAtkObj, const nsAutoString& aNewNameUTF16);
> 
> there's no reason this needs to be extern "C" which is all the G_DECL thing
> is hiding is there?

Yes, G_BEGIN_DECLS does ``extern "C" {''. There's no special reason why
I put the function prototype inside G_BEGIN_DECLS. I have now moved it
outside.

> btw type* name

Fixed.

> >   nsAutoString uniName;
> >   accWrap->Name(uniName);
> > 
> >-  NS_ConvertUTF8toUTF16 objName(aAtkObj->name);
> >-  if (!uniName.Equals(objName))
> >-    atk_object_set_name(aAtkObj, NS_ConvertUTF16toUTF8(uniName).get());
> >+  // XXX Firing an event from here does not seem right
> >+  MaybeFireNameChange(aAtkObj, uniName);
> 
> nit, might be nice if you renamed the var to just name since we don't
> convert it here

Done.

> >+MaybeFireNameChange(AtkObject* aAtkObj, const nsAutoString& aNewNameUTF16)
> 
> nit, utf16 is implied by the type being nsFooString not nsFooCString so kind
> of redundant

Ok, renamed it to aNewName.

> >+{
> >+  NS_ConvertUTF8toUTF16 curNameUTF16(aAtkObj->name);
> 
> wouldn't it be more efficient to just convert the new name to utf8?

Right, done that way in the simplified version (my original intention was
to preserve the way the body of atk_object_set_name() looks).

> >+  if (aNewNameUTF16.Equals(curNameUTF16)) {
> >+    return;
> >+  }
> 
> nit, no braces

Done.

> >+  const gchar* name = NS_ConvertUTF16toUTF8(aNewNameUTF16).get();
> 
> so, that creates an object that only lives for the statement which is going
> to mean accessing atkObj->name after this statement is a use after free.

Fixed.

> if you convert newName to utf8 at the start you can just do
> free(atkOjb->name); atkObj->name = strdup(newNameUTF8.get());

Done.

> >+  // Below we duplicate the functionality of atk_object_set_name(),
> >+  // but without calling atk_object_get_name(). Instead of
> >+  // atk_object_get_name() we directly access aAtkObj->name. This is because
> >+  // atk_object_get_name() would call getNameCB() which would call
> >+  // MaybeFireNameChange() (or atk_object_set_name() before this problem was
> >+  // fixed) and we would get an infinite recursion.
> >+  // See http://bugzilla.mozilla.org/733712
> >+
> >+  AtkObjectClass *klass;
> >+  gboolean notify = FALSE;
> >+
> >+  g_return_if_fail(ATK_IS_OBJECT(aAtkObj));
> >+  g_return_if_fail(name != NULL);
> >+
> >+  klass = ATK_OBJECT_GET_CLASS(aAtkObj);
> >+  if (klass->set_name) {
> >+    // Do not notify for initial name setting.
> >+    // See bug http://bugzilla.gnome.org/665870
> >+    notify = (aAtkObj->name != NULL);
> >+
> >+    (klass->set_name)(aAtkObj, name);
> >+    if (notify) {
> >+      g_object_notify(G_OBJECT(aAtkObj), atk_object_name_property_name);
> >+    }
> >+  }
> 
> the only part of this you need is the if notify g_object_notify() bit unless
> I'm missing something

Plus the free/strdup calls. I have now simplified the above (it does
not look like the original atk_object_set_name() anymore).

> thanks for helping to clean this up it looks good other than that.

You are welcome!

Comment 45

[Vasil Dimov [:vd]]

Attachment: Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

Comment 46

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd327272240

Comment 47

[Trevor Saunders (:tbsaunde)]

Comment on attachment 729066 [details] [diff] [review]
Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

sorry it took me so long, but I finally got to test your patch today.  It seems good so I checked it in for you ask you just saw :-)

Comment 48

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/369df88bbdbb for assertions like https://tbpl.mozilla.org/php/getParsedLog.php?id=21826076&tree=Mozilla-Inbound

Comment 49

[Vasil Dimov [:vd]]

Attachment: Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

This is an updated patch that should fix the assertion failure. The only change compared to the pushed patch [1] is this:

-+  if (newNameUTF8.Equals(aAtkObj->name))
++  if (aAtkObj->name && newNameUTF8.Equals(aAtkObj->name))

For some reason I was not able to reproduce the assertion failure myself with the "broken" patch, so I cannot verify that this one fixes the problem, but it *should* fix it.

I will followup here if I manage to reproduce the failure and then confirm that the above change fixes is.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd327272240

Comment 50

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5aa0a654611

Comment 51

[Trevor Saunders (:tbsaunde)]

Comment on attachment 738132 [details] [diff] [review]
Implement a replacement of atk_object_set_name() which mimics the behavior without calling atk_object_get_name()

landed again, but forgot to qref -u sorry  :(

Comment 52

[Vasil Dimov [:vd]]

No problem, thank you!

Comment 53

[Ryan VanderMeulen [:RyanVM]]

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