Bug 31044 - [Gtk] assertion when webkit_accessible_get_index_in_parent attempts to get parent of the web view
: [Gtk] assertion when webkit_accessible_get_index_in_parent attempts to get pa...
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
:
: 25531
  Show dependency treegraph
 
Reported: 2009-11-02 22:02 PST by
Modified: 2009-11-04 02:36 PST (History)


Attachments
proposed patch (3.60 KB, patch)
2009-11-02 22:16 PST, Joanmarie Diggs (irc: joanie)
xan.lopez: review-
Review Patch | Details | Formatted Diff | Diff
use g_object_unref to free children (?) (3.67 KB, patch)
2009-11-03 21:37 PST, Joanmarie Diggs (irc: joanie)
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-11-02 22:02:23 PST
+++ This bug was initially created as a clone of Bug #27011 +++

The web view claims to be an orphan. Orphans don't have an index in their supposedly non-existent parent. Attempting to get the index under these circumstances triggers an assertion.
------- Comment #1 From 2009-11-02 22:16:19 PST -------
Created an attachment (id=42361) [details]
proposed patch
------- Comment #2 From 2009-11-02 23:29:50 PST -------
(From update of attachment 42361 [details])
> +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.
------- Comment #3 From 2009-11-03 21:37:21 PST -------
Created an attachment (id=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.)
------- Comment #4 From 2009-11-04 02:26:03 PST -------
(In reply to comment #3)
> Created an attachment (id=42450) [details] [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 #5 From 2009-11-04 02:36:07 PST -------
(From update of attachment 42450 [details])
Clearing flags on attachment: 42450

Committed r50509: <http://trac.webkit.org/changeset/50509>
------- Comment #6 From 2009-11-04 02:36:12 PST -------
All reviewed patches have been landed.  Closing bug.