Created attachment 210551 [details] [review]
shell-embedded-window: Remove hacks for old and fixed Clutter bug

We used to use realize/unrealize instead of map/unmap in ShellEmbeddedWindow
because there originally was no map/unmap. The days of this are long gone...

Created attachment 210552 [details] [review]
shell-tray-manager: Remove bogus gtk_widget_destroy

Since ShellTrayIcon/ShellGtkEmbed keeps its own reference to the
child GTK+ window, we shouldn't destroy the widget but instead
rely on correct and proper memory management to get us there.

Created attachment 210553 [details] [review]
shell-gtk-embed: Fix NULL pointer dereference

Clutter will try to unmap during a dispose if we have a parent, so if we
set our own actor to NULL before the chain up, we're going to attempt to
unmap our own NULL actor. Fix that by swapping the order in which we
chain up.

Review of attachment 210551 [details] [review]:

Makes sense.

Review of attachment 210552 [details] [review]:

Uhm... as the docs for gtk_widget_destroy() say, widgets deriving from GtkWindow (like ShellGtkEmbed) are inserted in Gtk's toplevel list, so I guess you need both destroy and unref.

Review of attachment 210553 [details] [review]:

I understand the change in dispose(), but why map and unmap too?

Review of attachment 210552 [details] [review]:

Look at the source for gtk_widget_destroy -- it just calls run_dispose. The dispose vfunc runs, which sends out a destroy signal, and the gtk_window_real_destroy vfunc runs, which removes the window from the toplevel list.

Review of attachment 210553 [details] [review]:

For consistency more than anything.

(In reply to comment #7)
> Review of attachment 210552 [details] [review]:
> 
> Look at the source for gtk_widget_destroy -- it just calls run_dispose. The
> dispose vfunc runs, which sends out a destroy signal, and the
> gtk_window_real_destroy vfunc runs, which removes the window from the toplevel
> list.

[ Just reading the comments, haven't dug into the code in question ]

In general, we strongly want to destroy all GtkWidget derivatives and not let them get destroyed implicitly at zero refcount. Several reasons here:

 A) Cycles are not reliably collected when they go through language bindings and signal connection
 B) We want the minimum possible happening inside GC - it's much better if we're just finalizing objects that have already been cleaned up.

But in particular for GtkWindow, a window is *refcounted* by toplevel_list until gtk_widget_destroy() is called on it once. So, if the refcount on a GtkWindow hits zero without destroy() ever being called on it, that means there is a bug on it. Somebody called unref on a reference count that they didn't own.

(In reply to comment #9)
> (In reply to comment #7)
> > Review of attachment 210552 [details] [review] [details]:
> > 
> > Look at the source for gtk_widget_destroy -- it just calls run_dispose. The
> > dispose vfunc runs, which sends out a destroy signal, and the
> > gtk_window_real_destroy vfunc runs, which removes the window from the toplevel
> > list.
> 
> But in particular for GtkWindow, a window is *refcounted* by toplevel_list
> until gtk_widget_destroy() is called on it once. So, if the refcount on a
> GtkWindow hits zero without destroy() ever being called on it, that means there
> is a bug on it. Somebody called unref on a reference count that they didn't
> own.

Ah, OK. The case I was trying to fix was that we pass the window to ShellGtkEmbed, where we ref it. After we destroy the window, we destroy the actor, which will try to unref the stale pointer there. Should we just not ref/unref in the actor?

Review of attachment 210552 [details] [review]:

(rejecting)

Created attachment 210969 [details] [review]
shell-gtk-embed: Fix NULL pointer dereference

Clutter will try to unmap during a dispose if we have a parent, so if we
set our own actor to NULL before the chain up, we're going to attempt to
unmap our own NULL actor. Fix that by swapping the order in which we
chain up.

Patch status has been "accepted-commit_after_freeze" for more than 3 months now.
Which exact freeze does this refer to? Can this get committed, or should the patch status be updated?

Comment on attachment 210551 [details] [review]
shell-embedded-window: Remove hacks for old and fixed Clutter bug

Attachment 210551 [details] pushed as d212d57 - shell-embedded-window: Remove hacks for old and fixed Clutter bug

Attachment 210969 [details] pushed as f563fb1 - shell-gtk-embed: Fix NULL pointer dereference