Bug 28659 - Extra g_object_unref shouldn't happen for ATK objects without extra references in spi_object_return_referenc
Summary: Extra g_object_unref shouldn't happen for ATK objects without extra reference...
Alias: None
Product: at-spi2
Classification: Unclassified
Component: atk (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Rob Taylor
QA Contact:
Depends on:
Reported: 2010-06-22 00:11 UTC by Mario Sanchez Prada
Modified: 2010-06-23 06:08 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Patch proposal (6.63 KB, patch)
2010-06-22 00:13 UTC, Mario Sanchez Prada
Details | Splinter Review
Patch proposal (a better one) (8.16 KB, patch)
2010-06-23 01:39 UTC, Mario Sanchez Prada
Details | Splinter Review

Description Mario Sanchez Prada 2010-06-22 00:11:56 UTC
In spi_object_return_reference() inside at-spi2-atk/atk-adaptor/object.c, an extra g_object_unref() done by the end of the function is causing problems when the AtkObject doesn't have an extra reference (that is, it's just the same reference held internally by the implementation of ATK in use: GAIL or a11y layer in WebKitGtk, for instance), specially when a leasing is needed since the wouldn't enter in the cache. An example:

Suppose the AtkObject in use has a GObject ref_count of 1 when calling to spi_object_return_reference() (that is, it doesn't come from an atk_*_ref_*() method, but from a atk_*_get_*() one):

  1. spi_object_append_reference() is called for the object, which calls to spi_object_lease_if_needed().
  2. spi_object_lease_if_needed() determines that a leasing is needed (maybe the AtkObject is about a cell in a table), so it calls to spi_leasing_take().
  3. spi_leasing_take() increments the ref_count to 2 and sets an expiry function to be called after the timeout (expiry_func()).
  4. When the control returns to spi_object_return_reference(), g_object_unref() is called over the AtkObject, as it's not NULL, decreasing its ref_count to 1.
  5. After some time, the expiry_func function gets called, decreasing the ref_count to 0 and therefore getting the AtkObject destroyed, when that's something that shouldn't happen since that object would be property of the ATK implementation being used, such as GAIL for Gtk or the a11y implementation for WebKitGtk+.

Hence, it would be needed to take into account whether an AtkObject comes from an atk_*_ref_*() or an atk_*_get_*() method when calling to spi_object_return_reference(), to know when the extra g_object_unref() would be needed or not. This way, we wouldn't run into problems further, specially when leasing is involved, as you can see in the example above.
Comment 1 Mario Sanchez Prada 2010-06-22 00:13:34 UTC
Created attachment 36414 [details] [review]
Patch proposal

Implemented patch proposal as suggested by Mike Gorse: adding an extra parameter to decide whether to unref in spi_object_return_reference()
Comment 2 Mike Gorse 2010-06-22 01:15:55 UTC
Committed.  Thanks for the patch.
Comment 3 Mario Sanchez Prada 2010-06-23 01:38:27 UTC
Reopening after thinking that perhaps the implementation of the attached patch was not the best one:

I think that, instead of adding an extra parameter to know when to unref the AtkObject inside spi_object_return_reference(), it would be better just to completely remove the g_object_unref() from that function and only do it, when needed, after calling it, that is, from the callers.

I should have noticed before sending the former patch, sorry for the annoyance. I'm attaching a new patch right now...
Comment 4 Mario Sanchez Prada 2010-06-23 01:39:28 UTC
Created attachment 36433 [details] [review]
Patch proposal (a better one)

Here is the new patch, which applies cleanly right on top of the last one, that is, on top of current master :-)
Comment 5 Mike Gorse 2010-06-23 06:08:40 UTC

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.