(In reply to Trevor Saunders (:tbsaunde) from comment #43)
> Comment on attachment 728261
> 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.
(In reply to Trevor Saunders (:tbsaunde) from comment #43) set_name( ) which mimics the behavior get_name( ) 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);
> Comment on attachment 728261
> Implement a replacement of atk_object_
> without calling atk_object_
>
> >diff --git i/accessible/
> >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?
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; >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
Done.
> >+MaybeFireName Change( 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.
> >+{ oUTF16 curNameUTF16( aAtkObj- >name);
> >+ NS_ConvertUTF8t
>
> 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 set_name( ) looks).
to preserve the way the body of atk_object_
> >+ if (aNewNameUTF16. Equals( curNameUTF16) ) {
> >+ return;
> >+ }
>
> nit, no braces
Done.
> >+ 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.
Fixed.
> if you convert newName to utf8 at the start you can just do newNameUTF8. get());
> free(atkOjb->name); atkObj->name = strdup(
Done.
> >+ // Below we duplicate the functionality of atk_object_ 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);
> >+ // 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
Plus the free/strdup calls. I have now simplified the above (it does set_name( ) anymore).
not look like the original atk_object_
> thanks for helping to clean this up it looks good other than that.
You are welcome!