Bug 31044

Created attachment 42361 [details] proposed patch

Comment on attachment 42361 [details] proposed patch > +static AtkObject* webkit_accessible_get_parent(AtkObject* object) > +{ > + AccessibilityObject* coreParent = core(object)->parentObjectUnignored(); > + if (!coreParent && core(object)->isWebArea()) > + return atkParentOfWebView(object); > + > + if (!coreParent) > + return NULL; return 0; > + > + return coreParent->wrapper(); > +} > + > static gint webkit_accessible_get_n_children(AtkObject* object) > { > return core(object)->children().size(); > @@ -254,7 +266,16 @@ static gint webkit_accessible_get_index_in_parent(AtkObject* object) > AccessibilityObject* coreObject = core(object); > AccessibilityObject* parent = coreObject->parentObjectUnignored(); > > - g_return_val_if_fail(parent, 0); > + if (!parent && core(object)->isWebArea()) { > + AtkObject* atkParent = atkParentOfWebView(object); > + if (!atkParent) > + return -1; > + > + unsigned count = atk_object_get_n_accessible_children(atkParent); > + for (unsigned i = 0; i < count; ++i) > + if (atk_object_ref_accessible_child(atkParent, i) == object) This is doing a g_object_ref on each child, and then letting it go, so it's leaking all of them basically. You need to use GOwnPtr here, or similar, so that the references go away when you exit the function. > + return i; > + } > > AccessibilityObject::AccessibilityChildrenVector children = parent->children(); > unsigned count = children.size(); > @@ -263,7 +284,7 @@ static gint webkit_accessible_get_index_in_parent(AtkObject* object) > return i; > } > > - return 0; > + return -1; > } > > static AtkAttributeSet* addAttributeToSet(AtkAttributeSet* attributeSet, const char* name, const char* value) > -- > 1.6.3.3 > r- for those two issues, but looks good to me otherwise.

Created attachment 42450 [details] use g_object_unref to free children (?) > > - g_return_val_if_fail(parent, 0); > > + if (!parent && core(object)->isWebArea()) { > > + AtkObject* atkParent = atkParentOfWebView(object); > > + if (!atkParent) > > + return -1; > > + > > + unsigned count = atk_object_get_n_accessible_children(atkParent); > > + for (unsigned i = 0; i < count; ++i) > > + if (atk_object_ref_accessible_child(atkParent, i) == object) > > This is doing a g_object_ref on each child, and then letting it go, so it's > leaking all of them basically. You need to use GOwnPtr here, or similar, so > that the references go away when you exit the function. Oops. Thanks for this. At the moment, I'm leaning towards "similar". :-) (Can I just do a g_object_unref instead??) But I would like to understand this.... I tried: [...] GOwnPtr<AtkObject> child(atk_object_ref_accessible_child(atkParent, i)); if (child == object) return i; This is working, we get a match, and we attempt to return i. At which point, GtkLauncher aborts. :-( *** glibc detected *** /home/jd/WebKit/WebKitBuild/Release/Programs/GtkLauncher: double free or corruption (out): 0x08d1aa28 *** ~~~~~~~~~~~~~ Thread 2 (Thread 0xb7141b70 (LWP 9857)): #0 0x003cf422 in __kernel_vsyscall () #1 0x004cce15 in pthread_cond_wait@@GLIBC_2.3.2 () at ../nptl/sysdeps/unix/sysv/linux/i386/i686/../i486/pthread_cond_wait.S:122 #2 0x007b4d07 in WTF::TCMalloc_PageHeap::scavengerThread() () from /home/jd/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #3 0x007b4d41 in WTF::TCMalloc_PageHeap::runScavengerThread(void*) () from /home/jd/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #4 0x004c880e in start_thread (arg=0xb7141b70) at pthread_create.c:300 #5 0x067f37ee in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130 Thread 1 (Thread 0xb72fa760 (LWP 9851)): #0 0x003cf422 in __kernel_vsyscall () #1 0x067514d1 in *__GI_raise (sig=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:64 #2 0x06754932 in *__GI_abort () at abort.c:92 #3 0x06787ee5 in __libc_message (do_abort=2, fmt=0x684b438 "*** glibc detected *** %s: %s: 0x%s ***\n") at ../sysdeps/unix/sysv/linux/libc_fatal.c:189 #4 0x06791ff1 in malloc_printerr (action=<value optimized out>, str=0x6 <Address 0x6 out of bounds>, ptr=0x8d1aa28) at malloc.c:6217 #5 0x067936f2 in _int_free (av=<value optimized out>, p=<value optimized out>) at malloc.c:4750 #6 0x0679679d in *__GI___libc_free (mem=0x8d1aa28) at malloc.c:3716 #7 0x0856a186 in g_free () from /lib/libglib-2.0.so.0 #8 0x00e29dd2 in webkit_accessible_get_index_in_parent(_AtkObject*) () from /home/jd/WebKit/WebKitBuild/Release/.libs/libwebkit-1.0.so.2 #9 0x001aff72 in atk_object_get_index_in_parent (accessible=0x8d1aa28) at atkobject.c:961 #10 0x08279d26 in impl_accessibility_accessible_get_index_in_parent (servant=0x9087594, ev=0xbfc34d3c) at accessible.c:285 #11 0x08275fa6 in _ORBIT_skel_small_Accessibility_Accessible_getIndexInParent (_o_servant=0x9087594, _o_retval=0xbfc34b90, _o_args=0x0, _o_ctx=0xbfc34c28, _o_ev=0xbfc34d3c, _impl_getIndexInParent=0x8279d00 <impl_accessibility_accessible_get_index_in_parent>) at Accessibility-common.c:124 #12 0x05ccc537 in ?? () from /usr/lib/libORBit-2.so.0 #13 0x05cd2b45 in ORBit_OAObject_invoke () from /usr/lib/libORBit-2.so.0 #14 0x05cbee63 in ORBit_small_invoke_adaptor () from /usr/lib/libORBit-2.so.0 #15 0x05cd0649 in ?? () from /usr/lib/libORBit-2.so.0 #16 0x05cd0d22 in ?? () from /usr/lib/libORBit-2.so.0 #17 0x05cd0ed9 in ?? () from /usr/lib/libORBit-2.so.0 #18 0x05cd2f92 in ORBit_handle_request () from /usr/lib/libORBit-2.so.0 #19 0x05cbb155 in giop_connection_handle_input () from /usr/lib/libORBit-2.so.0 #20 0x05cda743 in ?? () from /usr/lib/libORBit-2.so.0 #21 0x05cdd016 in ?? () from /usr/lib/libORBit-2.so.0 #22 0x08561e78 in g_main_context_dispatch () from /lib/libglib-2.0.so.0 #23 0x08565720 in ?? () from /lib/libglib-2.0.so.0 #24 0x08565b8f in g_main_loop_run () from /lib/libglib-2.0.so.0 #25 0x04c78419 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0 #26 0x08049e6d in main () ~~~~~~~~~~~~~ Any suggestions regarding what I'm doing wrong/missing? Thanks! (Flagging for review in the hopes that the use of g_object_unref is fitting and appropriate.)

(In reply to comment #3) > Created an attachment (id=42450) [details] > use g_object_unref to free children (?) > But I would like to understand this.... I tried: > > [...] > GOwnPtr<AtkObject> child(atk_object_ref_accessible_child(atkParent, i)); > if (child == object) > return i; > > This is working, we get a match, and we attempt to return i. At which point, > GtkLauncher aborts. :-( > Any suggestions regarding what I'm doing wrong/missing? Thanks! The default implementation of freeOwnedGPtr (see JavaScriptCore/wtf/GOwnPtr.h) uses g_free instead of g_object_unref. So to have an AtkObject use GOwnPtr, you would need to "override" freeOwnedGPtr that will accept an AtkObject as a parameter and that calls g_object_unref to free to object. > > (Flagging for review in the hopes that the use of g_object_unref is fitting and > appropriate.) It's probably OK for this patch. But I think it's ideal if we could use GOwnPtr for AtkObjects as well in the future. r=me.

Comment on attachment 42450 [details] use g_object_unref to free children (?) Clearing flags on attachment: 42450 Committed r50509: <http://trac.webkit.org/changeset/50509>

All reviewed patches have been landed. Closing bug.