Launchpad has imported 21 comments from the remote bug at https://bugzilla.mozilla.org/show_bug.cgi?id=733712.
If you reply to an imported comment from within Launchpad, your comment will be sent to the remote bug automatically. Read more about Launchpad's inter-bugtracker facilities at https://help.launchpad.net/InterBugTracking. ------------------------------------------------------------------------ On 2012-03-07T11:20:56+00:00 Chris Coulson wrote: 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 Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/5 ------------------------------------------------------------------------ On 2012-03-07T11:37:03+00:00 Alexander Surkov wrote: Do you have stack crash? Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/6 ------------------------------------------------------------------------ On 2012-03-07T11:39:12+00:00 Chris Coulson wrote: >From the bug in Launchpad: https://launchpadlibrarian.net/95732761/gdb- thunderbird.txt Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/7 ------------------------------------------------------------------------ On 2012-03-07T11:53:39+00:00 Alexander Surkov wrote: 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? Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/9 ------------------------------------------------------------------------ On 2012-03-07T12:12:11+00:00 Alejandro Piñeiro wrote: (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). Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/10 ------------------------------------------------------------------------ On 2012-03-07T12:27:26+00:00 Alexander Surkov wrote: (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 Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/12 ------------------------------------------------------------------------ On 2012-03-07T12:49:26+00:00 Alejandro Piñeiro wrote: 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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/16 ------------------------------------------------------------------------ On 2012-03-07T13:29:59+00:00 Alexander Surkov wrote: 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 Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/17 ------------------------------------------------------------------------ On 2012-03-07T13:48:05+00:00 Alexander Surkov wrote: just remove atk_object_set_name instances Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/18 ------------------------------------------------------------------------ On 2012-03-07T14:55:45+00:00 Ginn-chen-r wrote: 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... Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/19 ------------------------------------------------------------------------ On 2012-03-07T15:01:46+00:00 Alexander Surkov wrote: (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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/20 ------------------------------------------------------------------------ On 2012-03-07T15:12:00+00:00 Ginn-chen-r wrote: (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? Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/21 ------------------------------------------------------------------------ On 2012-03-07T16:29:52+00:00 Alexander Surkov wrote: (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). Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/22 ------------------------------------------------------------------------ On 2012-03-07T17:45:47+00:00 Trevor Saunders wrote: (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? Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/23 ------------------------------------------------------------------------ On 2012-03-07T18:09:48+00:00 Alexander Surkov wrote: (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 Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/24 ------------------------------------------------------------------------ On 2012-03-08T06:15:20+00:00 Ginn-chen-r wrote: 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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/25 ------------------------------------------------------------------------ On 2012-03-09T14:24:43+00:00 Alexander Surkov wrote: (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) Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/27 ------------------------------------------------------------------------ On 2012-03-09T15:34:44+00:00 Trevor Saunders wrote: (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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/28 ------------------------------------------------------------------------ On 2012-03-12T01:52:44+00:00 Ginn-chen-r wrote: (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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/29 ------------------------------------------------------------------------ On 2012-03-12T02:10:18+00:00 Alexander Surkov wrote: (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? Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/30 ------------------------------------------------------------------------ On 2012-03-12T07:31:49+00:00 Ginn-chen-r wrote: (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. Reply at: https://bugs.launchpad.net/firefox/+bug/948788/comments/31 ** Changed in: firefox Status: Unknown => Confirmed ** Changed in: firefox Importance: Unknown => Critical -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/948788 Title: thunderbird crashed on launch To manage notifications about this bug go to: https://bugs.launchpad.net/firefox/+bug/948788/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
