GNOME Bugzilla – Bug 670647
workspace: avoid crash when selecting a zoomed clone
Last modified: 2012-03-09 22:27:19 UTC
See patch. It worked before because lower() and raise() were just reordering in a list, but in post-apocalyptic Clutter they are two-step operations i.e. "remove child" and "add child at position". Clearly the second step fails for actors in destruction.
Created attachment 208222 [details] [review] workspace: avoid crash when selecting a zoomed clone The lightbox destroying method tries to call lower() for the highlighted actor. Since the actor is currently in destruction, the call to lower() fails and leaves the actor in an unstable state which in the end causes a crash. Fix this by retarding the lightbox destruction.
Review of attachment 208222 [details] [review]: Looks good. (I'd s/retard/delay/ in the commit message, but up to you)
Review of attachment 208222 [details] [review]: No, I don't think this is the right solution. Calling lower() shouldn't put the actor into an unstable state if it's in destruction. The promise from ebassi was that there would be no regressions from before the apocalypse, so I think it's safe to ask him to hold his promise. (Leaving it at ACN, though... if we do eventually decide this is the right solution, please change 'retard')
what is the problem, here? the bug report is not really clear on that. I agree that there shouldn't be regressions (except when depending on undefined behaviour), but I'd like to understand what did break before I try and fix it.
Created attachment 208596 [details] [review] 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:
I'd like somebody to test the change in attachment 208596 [details] [review] and see if the assertion failure goes away; if it doesn't, I start making every method that changes the stacking order silently bail out if the child is inside its destruction sequence.
(In reply to comment #6) > I'd like somebody to test the change in attachment 208596 [details] [review] and see if the > assertion failure goes away; This works for me.
attachment 208596 [details] [review] has been pushed, closing as FIXED
*** Bug 671102 has been marked as a duplicate of this bug. ***
*** Bug 671733 has been marked as a duplicate of this bug. ***