[webkit-reviews] review requested: [Bug 106903] [GTK] Possible bogus unref of accessible table cell : [Attachment 186349] Add missing extra ref to implementation of atk_table_ref_at()

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has asked  for review:
Bug 106903: [GTK] Possible bogus unref of accessible table cell
https://bugs.webkit.org/show_bug.cgi?id=106903

Attachment 186349: Add missing extra ref to implementation of
atk_table_ref_at()
https://bugs.webkit.org/attachment.cgi?id=186349&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
(In reply to comment #0)
> Created an attachment (id=182758) [details]
> test case
> 
> Steps to reproduce:
> 
> 1. Load the test case in Epiphany
> 2. Start the event listener in a terminal
> 3. Give focus back to Epiphany
> 
> Expected results:
> 
> 1. "Success" would be printed in the terminal window
> 2. Epiphany would not crash
> 
> Actual results:
> 
> 1. "Failure after x calls" is printed in the terminal window
> 2. Epiphany might crash
> 
> Other details:
> 
> * The number of calls before failure is, for me, at least 3
> * This problem does not occur if I prevent the unref of table cells in
at-spi2-atk
> * This problem does not occur in Firefox or gtk-demo
> 
> So it seems that we might be unrefing a table cell somewhere where we
shouldn't be.
> But I so far have been unable to find it. And the fact that the number of
times
> varies makes me wonder if there's not a race condition or at-spi2 registry
timeout
> coming into play. :-/

I was checking this for a while now and I think the problem might be other than
an extra and uneeded unref. More specifically, I think the problem is that
there's a "missing and needed ref" in the implementation of the
atk_table_ref_at() method, which is meant to return a transfer-full reference
to the AtkObject representing the cell, but that it's now returning the
internal pointer held in WebCore's AccessibilityObject::m_wrapper

   In accessibility/atk/WebKitAccessibleInterfaceTable.cpp:
   ========================================================

   static AtkObject* webkitAccessibleTableRefAt(AtkTable* table, gint row, gint
column)
   {
	AccessibilityTableCell* axCell = cell(table, row, column);
	if (!axCell)
	    return 0;

       return axCell->wrapper();
   }

   [...]

   In accessibility/atk/AccessibilityObjectAtk.cpp:
   ================================================
   AccessibilityObjectWrapper* AccessibilityObject::wrapper() const
   {
       return m_wrapper;
   }


Thus, I believe this is what was causing the issue when getting the cell from
the event listener example (I could reproduce the issue in Accerciser too),
since the implementation of "GetAccessibleAt" in the side of the ATK bridge is
as follows:

   static DBusMessage *
   impl_GetAccessibleAt (DBusConnection * bus, DBusMessage * message,
			 void *user_data)
   {
     AtkTable *table = (AtkTable *) user_data;
     [...]

     obj = atk_table_ref_at (table, row, column);
     reply = spi_object_return_reference (message, obj);
     if (obj)
       g_object_unref (obj);

     return reply;
   }


I think that g_object_unref() is the one you (Joanie) commented to check if it
fixed the issue, but that unref is Ok since atk_table_ref_at() is a
transfer-full method, which reinforces my belief that the problem here was a
missing call to g_object_ref(). You can actually check with gdb that ref_count
for "obj" is zero here, too.

Last, regarding to the undeterministic nature of the failure, I do also think
that the at-spi2 registry might have something to do with that. Probably the
cached version of the AT-SPI object representing the still valid version of the
cell (before reaching ref_count zero in WebKit) is still being returned to
pyatspi for some time.

So, attaching a one-liner patch :)