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
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 on attachment 728261 set_name( ) which mimics the behavior without calling atk_object_ get_name( )
Implement a replacement of atk_object_
>diff --git i/accessible/ src/atk/ AccessibleWrap. cpp w/accessible/ src/atk/ AccessibleWrap. cpp src/atk/ AccessibleWrap. cpp src/atk/ AccessibleWrap. cpp object_ signals [LAST_SIGNAL] = { 0, }; Change( AtkObject *aAtkObj, const nsAutoString& aNewNameUTF16);
>index e35da5d..208e955 100644
>--- i/accessible/
>+++ w/accessible/
>@@ -142,16 +142,20 @@ struct MaiAtkObjectClass
> static guint mai_atk_
>
> #ifdef MAI_LOGGING
> int32_t sMaiAtkObjCreated = 0;
> int32_t sMaiAtkObjDeleted = 0;
> #endif
>
> G_BEGIN_DECLS
>+
>+static void
>+MaybeFireName
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; >Name(uniName) ; oUTF16 objName( aAtkObj- >name); Equals( objName) ) set_name( aAtkObj, NS_ConvertUTF16 toUTF8( uniName) .get()) ; ange(aAtkObj, uniName);
> accWrap-
>
>- NS_ConvertUTF8t
>- if (!uniName.
>- atk_object_
>+ // XXX Firing an event from here does not seem right
>+ MaybeFireNameCh
nit, might be nice if you renamed the var to just name since we don't convert it here
>+MaybeFireName Change( AtkObject* aAtkObj, const nsAutoString& aNewNameUTF16)
nit, utf16 is implied by the type being nsFooString not nsFooCString so kind of redundant
>+{ oUTF16 curNameUTF16( aAtkObj- >name);
>+ NS_ConvertUTF8t
wouldn't it be more efficient to just convert the new name to utf8?
>+ Equals( curNameUTF16) ) {
>+ if (aNewNameUTF16.
>+ return;
>+ }
nit, no braces
>+ const gchar* name = NS_ConvertUTF16 toUTF8( 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());
>+ set_name( ), get_name( ). Instead of get_name( ) we directly access aAtkObj->name. This is because get_name( ) would call getNameCB() which would call ange() (or atk_object_ set_name( ) before this problem was bugzilla. mozilla. org/733712 if_fail( ATK_IS_ OBJECT( aAtkObj) ); if_fail( name != NULL); GET_CLASS( aAtkObj) ; bugzilla. gnome.org/ 665870 >set_name) (aAtkObj, name); notify( G_OBJECT( aAtkObj) , atk_object_ name_property_ name);
>+ // Below we duplicate the functionality of atk_object_
>+ // but without calling atk_object_
>+ // atk_object_
>+ // atk_object_
>+ // MaybeFireNameCh
>+ // fixed) and we would get an infinite recursion.
>+ // See http://
>+
>+ AtkObjectClass *klass;
>+ gboolean notify = FALSE;
>+
>+ g_return_
>+ g_return_
>+
>+ klass = ATK_OBJECT_
>+ if (klass->set_name) {
>+ // Do not notify for initial name setting.
>+ // See bug http://
>+ notify = (aAtkObj->name != NULL);
>+
>+ (klass-
>+ if (notify) {
>+ g_object_
>+ }
>+ }
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.