(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.

You are welcome!

-- 
You received this bug notification because you are a member of Desktop
Packages, which is subscribed to firefox in Ubuntu.
https://bugs.launchpad.net/bugs/948788

Title:
  thunderbird crashed on launch

Status in The Mozilla Firefox Browser:
  In Progress
Status in “atk1.0” package in Ubuntu:
  Fix Released
Status in “firefox” package in Ubuntu:
  Fix Released
Status in “atk1.0” source package in Precise:
  Fix Released
Status in “firefox” source package in Precise:
  Fix Released

Bug description:
  Lauching thunderbird directly results in a core-dump

  ProblemType: Bug
  DistroRelease: Ubuntu 12.04
  Package: thunderbird 11.0~b4+build1-0ubuntu1
  ProcVersionSignature: Ubuntu 3.2.0-18.28-generic 3.2.9
  Uname: Linux 3.2.0-18-generic x86_64
  AddonCompatCheckDisabled: False
  AlsaVersion: Advanced Linux Sound Architecture Driver Version 1.0.24.
  ApportVersion: 1.94-0ubuntu2
  Architecture: amd64
  ArecordDevices:
   **** List of CAPTURE Hardware Devices ****
   card 0: Intel [HDA Intel], device 0: CONEXANT Analog [CONEXANT Analog]
     Subdevices: 1/1
     Subdevice #0: subdevice #0
  AudioDevicesInUse:
   USER        PID ACCESS COMMAND
   /dev/snd/controlC0:  nbarcet    2605 F.... pulseaudio
  BuildID: 20120302135656
  CRDA: Error: [Errno 2] No such file or directory
  Card0.Amixer.info:
   Card hw:0 'Intel'/'HDA Intel at 0xf2620000 irq 45'
     Mixer name : 'Intel IbexPeak HDMI'
     Components : 'HDA:14f15069,17aa214c,00100302 
HDA:80862804,17aa21b5,00100000'
     Controls      : 27
     Simple ctrls  : 9
  Card29.Amixer.info:
   Card hw:29 'ThinkPadEC'/'ThinkPad Console Audio Control at EC reg 0x30, fw 
6IHT39WW-1.14'
     Mixer name : 'ThinkPad EC 6IHT39WW-1.14'
     Components : ''
     Controls      : 1
     Simple ctrls  : 1
  Card29.Amixer.values:
   Simple mixer control 'Console',0
     Capabilities: pswitch pswitch-joined penum
     Playback channels: Mono
     Mono: Playback [on]
  Channel: beta
  Date: Wed Mar  7 09:51:39 2012
  EcryptfsInUse: Yes
  ForcedLayersAccel: False
  IncompatibleExtensions:
   EDS Contact Integration - [email protected], Version=0.3.9, 
minVersion=7.0, maxVersion=11.0a1, Location=app-global, Type=extension, 
Active=Yes
   Dictionnaire français «Classique & Réforme 1990» - 
[email protected], Version=4.3, 
minVersion=5.0, maxVersion=10.*, Location=app-profile, Type=extension, 
Active=Yes
   Auto Select Latest Message (restartless) - ID=autoselectlatestmessage@vano, 
Version=1.0, minVersion=3.3a1pre, maxVersion=10.*, Location=app-profile, 
Type=extension, Active=Yes
   Quicktext - ID={8845E3B3-E8FB-40E2-95E9-EC40294818C4}, Version=0.9.11.1, 
minVersion=5.0b2pre, maxVersion=10.*, Location=app-profile, Type=extension, 
Active=Yes
   Google Contacts - ID={BDD92442-0534-4D6F-A966-BAB7D561D781}, Version=0.6.40, 
minVersion=3.1, maxVersion=10.*, Location=app-profile, Type=extension, 
Active=Yes
  InstallationMedia: Ubuntu 12.04 LTS "Precise Pangolin" - Alpha amd64 
(20120201.2)
  ProcEnviron:
   LANGUAGE=en_US:en
   TERM=xterm
   PATH=(custom, user)
   LANG=en_US.UTF-8
   SHELL=/bin/bash
  Profiles: Profile0 (Default) - LastVersion=11.0/20120302135656
  RunningIncompatibleAddons: True
  SourcePackage: thunderbird
  UpgradeStatus: Upgraded to precise on 2012-02-16 (19 days ago)
  dmi.bios.date: 02/01/2011
  dmi.bios.vendor: LENOVO
  dmi.bios.version: 6IET75WW (1.35 )
  dmi.board.name: 2516CTO
  dmi.board.vendor: LENOVO
  dmi.board.version: Not Available
  dmi.chassis.asset.tag: No Asset Information
  dmi.chassis.type: 10
  dmi.chassis.vendor: LENOVO
  dmi.chassis.version: Not Available
  dmi.modalias: 
dmi:bvnLENOVO:bvr6IET75WW(1.35):bd02/01/2011:svnLENOVO:pn2516CTO:pvrThinkPadT410:rvnLENOVO:rn2516CTO:rvrNotAvailable:cvnLENOVO:ct10:cvrNotAvailable:
  dmi.product.name: 2516CTO
  dmi.product.version: ThinkPad T410
  dmi.sys.vendor: LENOVO

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/948788/+subscriptions

-- 
Mailing list: https://launchpad.net/~desktop-packages
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~desktop-packages
More help   : https://help.launchpad.net/ListHelp

Reply via email to