Re: GdkWindow ref counting

[Owen Taylor <otaylor redhat com>]

Caveat: it's been a long time since I've dealt with this code; I've
 looked through it again, but there may be some aspects that I'm 
 forgetting.

On Thu, 2006-11-02 at 11:52 +0100, Tim Janik wrote:
> hey Owen.
> 
> i've just chased GdkWindow reference counts with the help of Mitch and Tko
> and figured there's either a leak in Gdk or missing API docs.
> 
> basically, what i would have expected to work and not leak are:
> 1) w = gdk_window_new();
>     gdk_window_destroy (w);
> 2) w = gdk_window_foreign_new (xid);
>     gdk_window_destroy (w);
> 3) w = gdk_window_foreign_new();
>     gdk_window_unref (w);
> 
> what can be observed is that (1) and (2) both yield new gdkwindow
> objects with ref_count==2. also, (2) and (3) are apparently leaking
> a GdkWindow with a single reference.
> 
> for the uninitiated,
>    http://developer.gnome.org/doc/API/2.0/gdk/gdk-Windows.html#gdk-window-destroy
> has the docs on why
>     w = gdk_window_new();
>     gdk_window_unref (w);
> can NOT be expected to work leak-free.
> 
> (1) appears to work, because gdk_window_destroy() does an unref and
> an XDestroyWindow. when the corresponding DestroyNotify is released,
> the second unref occours and the window is removed from the xid table
> (gdk/x11/gdkwindow-x11.c:gdk_window_destroy_notify).

Yes, this is exactly as intended; there is one reference held by
GTK+ and released on destroy (much like GtkWindow), and one reference
held by the XID table.

> (2) however appears to leak one reference count, because we don't
> issue XDestroyWindow for foreign gdk windows, and thus never trigger
> the gdk_window_destroy_notify code.

 gdk_window_destroy() on a foreign window:
   - For windows in our hierarchy, hides them, reparents them to the
     root window, and sends them a WM_DELETE event
   - For windows not in our heirarchy, issues XDestroyWindow()

The first case is meant to handle the case of GtkPlug and similar
embedding situations.

I don't know what your test cases looked like, but perhaps you
used XCreateWindow() to create a window inside your hierarchy,
and then called gdk_window_foreign_new() on it? That case is
certainly a bit problematical now:

 - The reparent-to-the-root behavior isn't right for that case
 - If you call XDestroyWindow() yourself the window won't be
   removed from parent->children().

So, to make it work, you'd need to gdk_window_destroy() first,
then call XDestroyWindow()

The other possibility is that you didn't select for StructureNotify
on the foreign window, then GTK+ will never get notification 
and not remove the window from the XID table.

> also, note that (2) isn't always applicable for applications, because
> gdk_window_foreign_new() may simply hand out a new reference to an
> existing window if there already is a GdkWindow for this xid (and it
> might even be a valid internal, i.e. "non-foreign" one).
>
> that's why (3) should work for foreigns when it's not sure that the
> xid is really a foreign one. however, this case is leaking for the
> same reasons as (2).

This probably is a bug, or something that could be improved; in
the case above, it would be good if you could simply call 
XDestroyWindow() then g_object_unref().

I don't think the XID table should be holding a reference for foreign
windows, because we aren't guaranteed to be selecting for
StructureNotify, though it's normal to do so.

(There is, actually, never that much of a reason for the XID table
to hold a reference ... we used to warn about receiving events
for unknown windows, and to do that without races you need the
reference, but that warning was removed.)

> now we're wondering whether there's really a leak here (basically
> removing the additional g_object_ref near the end of foreign_new
> could fix this, since it doesn't pair with a future DestroyNotify
> to be received).

You can't just do that; because the code elsewhere will still
assume that the XID table is holding a reference:

 - You need to not unref when receiving a DestroyNotify for a 
   foreign window.
 - You need to make finalization of a foreign window remove it 
   from the XID table.

Also, see my note above about parent->children as a related
issue; finalization of a foreign window needs to make sure that
it is removed from the parent's list of children.

> or, if that "leak" is established/promoted API practise already,
> and users are actually expected to treat foreigns like:
> 
> 4) w = gdk_window_foreign_new();
>     if (GDK_WINDOW_IS_FOREIGN (w) &&
>         !GDK_WINDOW_IS_DESTROYED (w))
>       gdk_window_destroy (w);
>     gdk_window_unref (w);
> 
> to avoid leaks.

I have no idea what that is supposed to accomplish. It doesn't look
particularly useful to me. Certainly if you are trying to unref
the refcount that the XID table holds, that's a really bad idea!

(I'm actually a bit worried that if we remove the XID table reference
we'll find some apps that are doing exactly the above and then
crash! For that reason, I think it's probably not suitable as
a minor release change.)

> in any case, the intended usage should be made clear in the docs
> (it's prolly foreign_new that needs amending here), and properly
> documented in the code (i.e. we should add comments next to
> go_bject_ref in gdk_window_new and foreign_new which state what
> the paired unref is).
> 
> as an aside, we wondered whether there's a reason that
> gdk_window_destroy does *not* guard its unref call into
> if (!GDK_WINDOW_IS_DESTROYED (w)):
> 
> void
> gdk_window_destroy (GdkWindow *window)
> {
>    _gdk_window_destroy_hierarchy (window, FALSE, FALSE); // this function DOES guard it's body with !GDK_WINDOW_IS_DESTROYED()
>    g_object_unref (window);
> }

GDK_WINDOW_DESTROYED() doesn't mean "gdk_window_destroy() has been
called", it means "the X resources for the window have been destroyed"
That could happen:

 - Because a parent window was destroyed
 - Because some foreign agency destroyed the window (For a GtkPlug,
   maybe the app it was embedded in crashed)

So, if you guarded the g_object_unref() there, you would cause leaks;
the rule now is that if you call gdk_window_new(), you should call
gdk_window_destroy() exactly once.


					- Owen