Created attachment 145055 [details] [review]
[St] Call clutter_actor_destroy in dispose vfunc

Created attachment 145068 [details] [review]
[St] Call clutter_actor_destroy in dispose vfunc

ClutterGroup calls _destroy, but St was just calling _unparent.
This caused problems because the DESTROY signal was not emitted
for child elements after destroying a toplevel.  Also, in a GC'd
binding it would cause unpredictable lifetime of children.

Review of attachment 145068 [details] [review]:

StTable isn't handled here
StScrollView needs to destroy, not remove, its scrollbars
StLabel needs to destroy the ClutterText (it doesn't even unparent it!)
StTooltip needs to destroy the label (it doesn't even unparent it!)

StScrollBar is probably OK since the steppers are really internal so unparenting will work.
Note that StWidget can't destroy the background since it will cause problems for the StButton background animation hack; since the background is really internal it's probably OK to just unparent it.

You might want to double-check the above by doing a grep for clutter_actor_set_parent() in case I missed something.

::: src/st/st-bin.c
@@ +357,1 @@
       priv->child = NULL;

I think this probably would be better as:
 
 assert (priv->child == NULL);

So we catch the problem not leak if for some reason the destroy doesn't cause the child to be removed.

::: src/st/st-box-layout.c
@@ +405,3 @@
+  g_list_foreach (priv->children, (GFunc) clutter_actor_destroy, NULL);
+  /* We rely on unparenting in _destroy */
+  g_assert (priv->children == NULL);

I think this is better as:

 while (priv->children)
   clutter_actor_destroy (priv->children->data);

More robust against weird side effects.

*** Bug 597844 has been marked as a duplicate of this bug. ***

Created attachment 145144 [details] [review]
[St] Use clutter_actor_destroy in dispose, add _dispose where needed

ClutterGroup calls _destroy, but most of St was just calling _unparent.
This caused problems because the DESTROY signal was not emitted
for child elements after destroying a toplevel.  Also, in a GC'd
binding it would cause unpredictable lifetime of children.

Some St widgets simply didn't have _dispose at all; implement it.

Note because of the usage of the background_image in StButton,
we can't cleanly destroy it inside the StWidget.

Review of attachment 145144 [details] [review]:

Looks good to me with two small fixes noted below.

::: src/st/st-label.c
@@ +188,3 @@
+{
+  StLabelPrivate *priv = ST_LABEL (actor)->priv;
+  clutter_actor_destroy (priv->label);

dispose can be called more than once, so this should be like st-entry.c

 if (priv->label)
   {
      clutter_actor_destroy (priv->label);
      priv->label = NULL';
   }

(Don't worry about the fact that other code in the widget doesn't handle priv->label == NULL -  resurrecting actors would take a ton more work. The most common case for double dispose is that at dispose time the object doesn't get freed because there is a refcount to it and then it will get disposed again as finalized - this happens, e.g., any time you call label.destroy() in JS since JS will have a reference to the label until GC time.)

::: src/st/st-scroll-view.c
@@ +133,1 @@
       priv->vscroll = NULL;

Hmm, little bit weird because StScrollView does have a remove(), it just doesn't handle the hscroll/vscroll, but I think it's OK.

::: src/st/st-tooltip.c
@@ +361,3 @@
+  StTooltipPrivate *priv = ST_TOOLTIP (self)->priv;
+
+  clutter_actor_destroy (priv->label);

Needs the if (priv->label) { ... ; priv->label = NULL } as described for st-label.c

Review of attachment 145144 [details] [review]:

Your patch on bug 597919 points out that StScrollBar should call g_object_run_dispose() on the adjustment to disconnect signals there, since otherwise the innocuous looking:

  let vAdjust = scrollview.vscroll.adjustment;
  vAdjust.connect('changed', Lang.bind(this, function () { this._onAdjustScopeChanged(tabData); }));

creates an uncollectable loop - the callback referenes the scope, the scope references the vAdjust, the vAdjust references the callback.

(In reply to comment #7)
> Review of attachment 145144 [details] [review]:
> 
> Your patch on bug 597919 points out that StScrollBar should call
> g_object_run_dispose() on the adjustment to disconnect signals there, since
> otherwise the innocuous looking:
> 
>   let vAdjust = scrollview.vscroll.adjustment;
>   vAdjust.connect('changed', Lang.bind(this, function () {
> this._onAdjustScopeChanged(tabData); }));
> 
> creates an uncollectable loop - the callback referenes the scope, the scope
> references the vAdjust, the vAdjust references the callback.

Hmm...the ScrollBar doesn't necessarily "own" the adjustments since someone could call st_scroll_bar_set_adjustment.  So is it still correct to dispose there?

(In reply to comment #7)
> Review of attachment 145144 [details] [review]:
> 
> Your patch on bug 597919 points out that StScrollBar should call
> g_object_run_dispose() on the adjustment to disconnect signals there, since
> otherwise the innocuous looking:
> 
>   let vAdjust = scrollview.vscroll.adjustment;
>   vAdjust.connect('changed', Lang.bind(this, function () {
> this._onAdjustScopeChanged(tabData); }));
> 
> creates an uncollectable loop - the callback referenes the scope, the scope
> references the vAdjust, the vAdjust references the callback.

An alternative we can say here is that it should be slaved to the lifecycle of the main actor, in other words if we had:

Signals.connect_bounded ({ source: scrollview.vscroll.adjustment,
                           signal: 'changed',
                           destroyable: scrollview,
                           callback: Lang.bind(this, function () { this._onAdjustScopeChanged(tabData); }))

where "destroyable" is an object that has a "destroy" signal.

(In reply to comment #8)
> (In reply to comment #7)
> > Review of attachment 145144 [details] [review] [details]:
> > 
> > Your patch on bug 597919 points out that StScrollBar should call
> > g_object_run_dispose() on the adjustment to disconnect signals there, since
> > otherwise the innocuous looking:
> > 
> >   let vAdjust = scrollview.vscroll.adjustment;
> >   vAdjust.connect('changed', Lang.bind(this, function () {
> > this._onAdjustScopeChanged(tabData); }));
> > 
> > creates an uncollectable loop - the callback referenes the scope, the scope
> > references the vAdjust, the vAdjust references the callback.
> 
> Hmm...the ScrollBar doesn't necessarily "own" the adjustments since someone
> could call st_scroll_bar_set_adjustment.  So is it still correct to dispose
> there?

I think we should consider st_scroll_bar_set_adjustment() like clutter_container_add_actor() ... as handing ownership of the adjustment to the scrollbar.

Someone has to be the ownership, and i if I look at the scroll view code, the scrollbar is the most natural candidate to "own" the adjustment - the scrollview gets the adjustements from the scrollbars and sets them on the child.

I don't like a "connect specially this way to prevent memory leaks" way of doing things, because then we're restricting not leaking to the domain of programmers who have some idea of what references and garbage collection are.

If we destroy too aggressively, it will lead to obvious bugs, and someone can always pull the adjustment out of the line of fire with scrollbar.set_adjustment(null); it can also be documented in the docs for st_scrollbar_set_adjustment().

(In reply to comment #10)
>
> I think we should consider st_scroll_bar_set_adjustment() like
> clutter_container_add_actor() ... as handing ownership of the adjustment to the
> scrollbar.
> 
> Someone has to be the ownership, and i if I look at the scroll view code, the
> scrollbar is the most natural candidate to "own" the adjustment - the
> scrollview gets the adjustements from the scrollbars and sets them on the
> child.

Fair enough, I'll attach a patch.

> I don't like a "connect specially this way to prevent memory leaks" way of
> doing things, because then we're restricting not leaking to the domain of
> programmers who have some idea of what references and garbage collection are.

Well, there are various different cases in this domain.  Often in the shell we can have four things involved; a ClutterActor subclass, some other GObject (might be a ClutterActor, might not), and a proxy object for each one.  Then we typically create a signal handler on the GObject.  

In the case where the lifecycle of one doesn't have any expected relation to the other (the AppIcon and MetaWindow case), we really do want some way to express that the signal handler should go away if either half does.  Dispose on the GObject will break the connection if the MetaWindow goes away earlier, but nothing by default will if the Actor is destroyed.

Actually rather than in Signals, I could imagine the Gjs-bound .connect having this functionality.

I guess it doesn't come up often, but when it does it's fairly insidious since one is used to just typing .connect everywhere.

There is another issue with using clutter_actor_destroy(). It will run dispose on the actor, which will run the following code:

  /* avoid recursing when called from clutter_actor_destroy() */
  if (priv->parent_actor != NULL)
    {
      ClutterActor *parent = priv->parent_actor;

      if (CLUTTER_IS_CONTAINER (parent))
        clutter_container_remove_actor (CLUTTER_CONTAINER (parent), self);
      else
        priv->parent_actor = NULL;
    }

  /* parent should be gone */
  g_assert (priv->parent_actor == NULL);


Therefore, Widget cannot destroy the background and border image actors, because a sub class that implements ClutterContainer (such as Button) will cause the assertion to be reached, since the background and border image actors cannot be removed using clutter_container_remove_actor().

Well, I think the expected contract here is that for "implementation detail" widgets, you can get away with just _run_dispose () and unparent ().  

But for say a generic Box widget, we really need to call _destroy () on children.

for internal children of actors implementing the Container interface, we have clutter_actor_push_internal()/pop_internal() since Clutter 1.2.

Is there anything left to be done here?

All St actors consistently call destroy on their children inside dispose (or better, have Clutter do it), so I don't think so.