Created attachment 133460 [details] [review]
Revert "Unparent rather than destroy MutterWindows."

This reverts commit 16d49695ad07555a36fe7ad4f60067fc944689f4.

If we want an actor to go away deterministically, we should call
clutter_actor_destroy(); using unparent instead a) may result
leaks through cyclic ref counts, and b) will delay the deallocation
of the window texture in a garbage collected environment until
the next garbage collection, causing much greater transient use
of memory.

This change was put in place because using clutter_actor_destroy() is generally bad practice, unparenting is the normal way to dispose of actors. Using destroy() means you cannot manage actor life span by simply taking a reference to it, and is particularly problem if it takes place inside signal emission, where it pretty much invariably causes crashes.

Having said that, the last scenario is not likely with the MutterWindow, so reverting this might be possible, but I need to test it extensively first.

If destroy inside signal emission invariably causes crashes, then that would reflect (serious) bugs in Clutter. We did a lot of work in the early days of GTK+ to make the object/signal system absolutely robust against that, and that work was carried over into the design of GObject.

It's worth keeping in mind that destroy() does not immediately cause an object to be freed, but rather it "breaks links" - the object releases references to other objects and other objects are requested to drop references to that object. That will frequently cause the refcount to go to zero and the object to be freed, but explicit reference counts (like the one held by the signal system while emitting a signal on the object) are unaffected.

Consider the following code:

  parent = Clutter.Group();
  child = Clutter.Text({ reactive: true, text: "Hello" });
  parent.add_actor('child');
  child.connect('button-press-event', function() {
     parent.x = parent.y = 50;
  })
  
  grandparent.add_actor(parent);

If we try to get rid of that combination of actors by doing:

  grandparent.remove_actor(parent);

Then we are left with a reference cycle:

  
          parent => child => javascript closure 
            \...........<=.........../

This cycle cannot be collected because it includes both the Javascript garbage collection system and C reference counting. (The problem is general to basically all combinations of C and another language: it's not Javascript specific.)

On the other hand, if you simply do parent.destroy(), then the parent will be removed from the grandparent, the child destroyed recursively, signal connections on the child disconnected, and everything properly freed.

I understand your problem, but my concern is that once the actor is disposed of, there is not much you can do with it, even it if is not finalized, and in the past using clutter_actor_destroy() gave me nothing but grief -- anyway, I am going to apply locally and test for a while.

Ping on this?

Let's do this; if the old problems resurface, we will need to find a better way of fixing them.

Pushed