actor: Do not check for child destruction in add_child_internal()
authorEmmanuele Bassi <ebassi@linux.intel.com>
Tue, 28 Feb 2012 15:45:24 +0000 (15:45 +0000)
committerEmmanuele Bassi <ebassi@linux.intel.com>
Wed, 29 Feb 2012 15:20:57 +0000 (15:20 +0000)
commit80626e75842a24c3d0a45068e241ba309f6ec138
treeba40330d3b7ae6589f580f6cbe3ed553e2d5d3fc
parent6e78ebca957fe838508e6a02f6c9c312edd40f21
actor: Do not check for child destruction in add_child_internal()

We currently check for the IN_DESTRUCTION flag inside the
add_child_internal() function.

This check disallows calling methods that change the stacking order
within the destruction sequence, by triggering a critical warning first,
and leaving the actor in an undefined state, which then ends up being
caught by an assertion.

The reproducible sequence is:

  - actor gets destroyed;
  - another actor, linked to the first, will try to change the
    stacking order of the first actor;
  - changing the stacking order is a composite operation composed
    by the following steps:
    1. ref() the child;
    2. remove_child_internal(), which removes the reference;
    3. add_child_internal(), which adds a reference;
  - the state of the actor is not changed between (2) and (3), as
    it could be an expensive recomputation;
  - if (3) bails out, then the actor is in an undefined state, but
    still alive;
  - the destruction sequence terminates, but the actor is unparented
    while its state indicates being parented instead.
  - assertion failure.

The obvious fix would be to decompose each set_child_*_sibling() method
into proper remove_child()/add_child(), with state validation; this may
cause excessive work, though, and trigger a cascade of other bugs in
code that assumes that a change in the stacking order is an atomic
operation.

Another potential fix is to just remove this check here, and let code
doing stacking order changes inside the destruction sequence of an actor
continue doing the work.

The third fix is to silently bail out early from every
set_child_*_sibling() and set_child_at_index() method, and avoid doing
work.

I have a preference for the second solution, since it involves the least
amount of work, and the least amount of code duplication.

See bug: https://bugzilla.gnome.org/show_bug.cgi?id=670647
clutter/clutter-actor.c