I have hit exactly the same problem with my own application program which is based on the same example code.

The problem only appeared after a recent upgrade from gtk+-2.16.6 to gtk+-2.18.7 and from gtkmm-2.16.0 to gtkmm-2.18.2

It is not obvious whether this problem is a bug in gtk+/gtkmm or whether it is a bug in the example code (and thence my application program).

If the example code is buggy then my question is where is the *authoritative* documentation for GTK/Cairo integration at the API level?

I found a workaround to the bug. Let's assume that the example code is correct. That means that the changes made to Glib::RefPtr<> contain a regression. Specifically it misses one reference-count of the underlying of object or at least this is implied by the Gtk-Warnings.

So a simple solution would be:
MyWidget::~MyWidget()
{
m_refGdkWindow->reference();
m_refGdkWindow.reset();
}

Note that m_refGdkWindow.reset(); is equivalent to m_refGdkWindow.clear();(in MyWidget::on_unrealize) which is deprecated.


I'll create a bug about Glib::RefPtr and report it here too.

I filed a bug here-->https://bugzilla.gnome.org/show_bug.cgi?id=614501 about RefPtr.

That example code has changed recently to avoid using deprecated API. Does it help to use an older version of the example code?

I used the code available at the site. Is that the newer code? If, not where can I get the newer example code?

Moreover, do the Gtk-Warnings pop up for you too?

Maybe something changed on how you create/manage a custom widget and it's Gdk::Window, in the recent releases?

Here is the gdb backtrace, breaking on g_log:

Breakpoint 1, 0x00d70056 in g_log () from /lib/libglib-2.0.so.0
(gdb) bt

+ Trace 221240

Created attachment 158048 [details] [review]
investigation.patch

This patch to gtkmm-documentation shows some investigation that I tried, trying to use more C API, to find out where the problem is. Even using the C API (inside a C++ class), I saw that same warning until I used gdk_window_destroy(). However, I am not sure that we should really have to use gdk_window_destroy().

I was not able to do the same thing with the C++ code without introducing other problems. This is what I tried in the destructor:

if(window)
{
  window->reference
  gdk_window_destroy(window->gobj());
  window.reset();
}

Created attachment 159506 [details] [review]
Patch to fix custom widget example

I was able to fix my own application by deleting m_refGdkWindow completely and calling get_window() in on_expose_event() instead. I can't remember where I first saw this approach.

I've modified the custom widget example similarly; simplified.patch is attached.

I'm not claiming this is an authoritative fix for the example, but it works for me.

(In reply to comment #8)
> I was able to fix my own application by deleting m_refGdkWindow completely and
> calling get_window() in on_expose_event() instead.

Instead of creating the GdkWindow, you are then using the parent's GdkWindow, I think. We do call Gtk::Widget::set_has_window(false) so that feels like it makes sense. I'd like to know when we are meant to call that with true or false.

I noticed that the MyWidget::on_unrealized() method is never called on this system.  Maybe that has something to do with this.

A work-around for the problem is to add
   m_refGdkWindow->reference();
in MyWidget::on_realize. I tried to dig into the source code of Gtk::Widget
and gtk_widget_* a bit, and I think the problem is this:

Gtk::Widget::set_window calls gtk_widget_set_window(GtkWidget* widget,
GdkWindow* window), which stores the pointer 'window' without calling
g_object_ref(window). Now the created GdkWindow has a reference count of 1,
but there are 2 pointers to it, one in Gtk::Widget, and one in Glib::RefPtr.
When MyWidget is destroyed, first the destructor
Glib::RefPtr<Gdk::Window>::~RefPtr unreferences it, and then the destructor
Widget::~Widget (base class of MyWidget) indirectly calls gtk_widget_dispose,
which indirectly calls gtk_widget_real_unrealize, which calls
g_object_unref(widget->window), either directly or via gdk_window_destroy.
(I have a backtrace from gdb that shows some functions called from
Widget::~Widget.)

The cause of the problem seems to be that gtk_widget_set_window takes over the
ownership of the GdkWindow object without increasing its ref count, and the
gtkmm code does not expect that behavior.

A possible solution would be to have
Gtk::Widget::set_window(const Glib::RefPtr<Gdk::Window>& window) do
   window->reference();
Or is the fault in gtk_widget_set_window? Should it call g_object_ref? I guess
that would break a lot of existing C code that depends on the present behavior
of gtk_widget_set_window.

I can confirm this bug. I tried applying the changes in the second patch to my own widget. It got rid of the warning/error messages but the expose event callback isn't drawing the widget, even though it's being called. Could it be because the EXPOSURE_MASK is no longer set on the window? My widget is here:

http://polygnome.svn.sourceforge.net/viewvc/polygnome/branches/no_glade/src/MTBeatWidget.cpp?view=markup&pathrev=249

We've hit something similar in java-gnome. We get the 

Gdk-WARNING "losing last reference to undestroyed window\n"

then a

Gtk-CRITICAL "IA__gdk_window_get_effective_parent: assertion `GDK_IS_WINDOW (window)' failed"

or varition thereof. Always GDK_IS_WINDOW() failed; I only just noticed the warning before that.

The fact that GTKmm is hitting this makes me suddenly suspicious that it's not something we're doing wrong after all. Both java-gnome and GTMmm make similar demands on GTK that are a bit outside the norm, and our code was all working until not too long ago. Once I get enough of a stack trace I'll be openning our own bug, but I'm having a hard time replicating this quickly and on demand. Nevertheless I thought I should get in touch.

AfC

For reference, I opened bug #637132 with the possibly related problem.

AfC

In gtk+ 3 the following sentence has been added to the documentation of
gtk_widget_set_window: "This function does not add any reference to @window."
I take that as an indication that the gtk+ developers have no intention to
change the behaviour of gtk_widget_set_window. Then it remains for
Gtk::Widget::set_window to add the missing reference. It should look like:

  void Widget::set_window(const Glib::RefPtr<Gdk::Window>& window)
  {
    gtk_widget_set_window(gobj(), Glib::unwrap(window));
    if (window)
      window->reference(); // gtk_widget_set_window does not add a ref.
  }

I assume that set_window is used only as specified. ("This function should only
be used in a widget's #GtkWidget::realize implementation.") I.e. no GdkWindow
has been previously set in the widget. If there is an old GdkWindow that should
be unreferenced or destroyed, things get more complicated.


I've compared the custom widget example to what some of the gtk+ widgets do in
their realize functions (e.g. gtk_drawing_area_realize). If MyWidget in the
custom widget example shall follow the same procedure, some changes are
necessary:

In the constructor: set_has_window(true);
In MyWidget::on_realize:
  Do not call the base class Gtk::Widget::on_realize(). It's appropriate only
  for widgets without a GdkWindow of their own.
  Instead, call set_realized().
  Change
    m_refGdkWindow = Gdk::Window::create(get_window() /*parent*/, &attributes,
            GDK_WA_X | GDK_WA_Y);
    set_has_window();
    set_window(m_refGdkWindow);
  to
    m_refGdkWindow = Gdk::Window::create(get_parent_window(), &attributes,
            GDK_WA_X | GDK_WA_Y);
    set_window(m_refGdkWindow);

    //Attach this widget's style to its Gdk::Window. (Not necessary in gtkmm 3)
    style_attach();

In the present implementation of MyWidget::on_realize, there is another ref
count error. Gtk::Widget::on_realize() calls gtk_widget_real_realize which
increases the ref count of the parent GdkWindow. That extra reference is never
unreferenced. That's one reason not to call Gtk::Widget::on_realize().

I can make patches on request.

(In reply to comment #15)
> In gtk+ 3 the following sentence has been added to the documentation of
> gtk_widget_set_window: "This function does not add any reference to @window."

Ah. That's very interesting. Do you know what commit added that?

> I take that as an indication that the gtk+ developers have no intention to
> change the behaviour of gtk_widget_set_window. Then it remains for
> Gtk::Widget::set_window to add the missing reference. It should look like:
> 
>   void Widget::set_window(const Glib::RefPtr<Gdk::Window>& window)
>   {
>     gtk_widget_set_window(gobj(), Glib::unwrap(window));
>     if (window)
>       window->reference(); // gtk_widget_set_window does not add a ref.
>   }

Yes, could you submit a patch for that, please? Otherwise I or someone will get to it when we have a chance.

> I assume that set_window is used only as specified. ("This function should only
> be used in a widget's #GtkWidget::realize implementation.") I.e. no GdkWindow
> has been previously set in the widget. If there is an old GdkWindow that should
> be unreferenced or destroyed, things get more complicated.
> 
> 
> I've compared the custom widget example to what some of the gtk+ widgets do in
> their realize functions (e.g. gtk_drawing_area_realize). If MyWidget in the
> custom widget example shall follow the same procedure, some changes are
> necessary:

Could you submit a patch for that too, please?
 
> In the constructor: set_has_window(true);
> In MyWidget::on_realize:
>   Do not call the base class Gtk::Widget::on_realize(). It's appropriate only
>   for widgets without a GdkWindow of their own.
>   Instead, call set_realized().
>   Change
>     m_refGdkWindow = Gdk::Window::create(get_window() /*parent*/, &attributes,
>             GDK_WA_X | GDK_WA_Y);
>     set_has_window();
>     set_window(m_refGdkWindow);
>   to
>     m_refGdkWindow = Gdk::Window::create(get_parent_window(), &attributes,
>             GDK_WA_X | GDK_WA_Y);
>     set_window(m_refGdkWindow);
> 
>     //Attach this widget's style to its Gdk::Window. (Not necessary in gtkmm 3)
>     style_attach();
> 
> In the present implementation of MyWidget::on_realize, there is another ref
> count error. Gtk::Widget::on_realize() calls gtk_widget_real_realize which
> increases the ref count of the parent GdkWindow. That extra reference is never
> unreferenced. That's one reason not to call Gtk::Widget::on_realize().

Well, can we fix on_realize()? Obviously I need to look at this more, but I'm in a rush right now.

Created attachment 178081 [details] [review]
Patch with modified Gtk::Widget::set_window()

I've attached a patch for Gtk::Widget::set_window(). It's made on the master
branch in Git. Is that ok? I guess you want to apply the patch both to the
master branch and some of the gtkmm-2-xx branches.

I added the documentation of set_window() in widget.hg and made some changes
to it. I first thought of using _WRAP_METHOD_DOCS_ONLY, but then I guess the
new info "This function does not add any reference to @window." would be
included the next time gtk_docs.xml is regenerated, and that's not true for
the gtkmm method.

I don't know what commit added the new info to the description of
gtk_widget_set_window. And I don't know of any reasonably easy method to find
out. (I'm very far from being a Git expert.) If it's important I can try.

I will make a patch for the custom_widget example later, hopefully tomorrow.
I'll probably work on the gtkmm-2-22 branch then, if you don't mind. The
custom_widget example is very different in gtkmm 3. As explained in bug 639073,
many other changes are necessary there.

Created attachment 178116 [details] [review]
gtkmm-documentation patch: custom_widget example

Here's the patch that modifies the custom_widget example, mainly
MyWidget::on_realize(). It's made on the gtkmm-2-22 branch.

I found that 'git blame gtk/widget.c' reasonably easily found the commit that
added the comment. It's "Adding note to docs of gtk_widget_set_window()"
http://git.gnome.org/browse/gtk+/commit/?id=a6a036ce22c2fa21b47a405e625360c02a140c91
committed by Tristan Van Berkom on 2010-09-09.

I forgot to include the bug number in ChangeLog in the patch in comment 17.
Is it important? Can you add it yourself? Shall I submit a modified patch?

Review of attachment 178081 [details] [review]:

Thanks. Applied to the master and gtkmm-2-24 branches.

I guess that set_window() should also be protected, right? Or is it called on widgets other than "this", such as child widgets?

Review of attachment 178116 [details] [review]:

Applied to the gtkmm-2-22 and master branches (I'll create a gtkmm-2-24 branch soon, from gtkmm-2-22). I had to resolve some conflicts when applying it to master.

(In reply to comment #19)
> Review of attachment 178081 [details] [review]:
> 
> Thanks. Applied to the master and gtkmm-2-24 branches.

Actually, will this extra ref ever be unrefed by GtkWidget? Maybe the patch creates a leak.

(In reply to comment #20)
> I guess that set_window() should also be protected, right? Or is it called on
> widgets other than "this", such as child widgets?

I'm not absolutely sure, but I think you're right. Probably both set_window()
and set_has_window() should be protected. (But the corresponding 'get' methods
can still be public.) Making the 'set' methods protected will not make all
misuse impossible, but it's still a good idea. I didn't think of that when I
made the patch.

(In reply to comment #22)
> Actually, will this extra ref ever be unrefed by GtkWidget? Maybe the patch
> creates a leak.

Yes, it will be unrefed when the widget is unrealized.
gtk_widget_real_unrealize calls gdk_window_destroy, which unrefs the window.
If you look at the error messages in the description of this bug,
the last one is
   GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT
   (object)' failed
It shows, I believe, that there has been one more unref than ref on the
GdkWindow.

(In reply to comment #20)
> I guess that set_window() should also be protected, right? Or is it called on
> widgets other than "this", such as child widgets?

My previous reply to comment 20 was too cautious. Yes, set_has_window() and
set_window() should definitely be protected. set_has_window() shall only be
called in a constructor, and set_window() only in on_realize(), which is
protected.

(In reply to comment #24)
> Yes, set_has_window() and
> set_window() should definitely be protected. set_has_window() shall only be
> called in a constructor, and set_window() only in on_realize(), which is
> protected.

Thanks. Done in git master.

(In reply to comment #23)
> Yes, it will be unrefed when the widget is unrealized.

Thanks. That's OK then.


We don't yet have an unset_window() equivalent for gdk_widget_set_window(NULL), but when we do add one, it should the unref too. However, it 
a) should unref anything that was added with set_window(),
b) not unref anything that was added via the C API instead.

I fear that we must store a RefPtr in a member variable to do this.

(In reply to comment #26)
> We don't yet have an unset_window() equivalent for gdk_widget_set_window(NULL),

Do we need an unset_window()? Has anyone asked for it?

Once again I searched some of the gtk+ code. As far as I can see, no one ever
calls gtk_widget_set_window(NULL). GtkWidget::priv.window is nullified only
when a widget is unrealized, and that's done with a call to
gtk_widget_unrealize() or gtk_widget_real_unrealize().

Unless someone urgently needs an unset_window() method, let's at least wait
until we know what will happen with bug 639419. If gtk_widget_attach_window()
is implemented, and gtk_widget_set_window() is deprecated, we will definitely
need to do no more than wrap gtk_widget_attach_window() with _WRAP_METHOD,
and update the custom_widget example to use attach_window().

(In reply to comment #27)
> (In reply to comment #26)
> > We don't yet have an unset_window() equivalent for gdk_widget_set_window(NULL),
> 
> Do we need an unset_window()? Has anyone asked for it?

No, but I guessed that it can take NULL, so there should be one.

Well, more obviously, we need to handle someone re-setting the GdkWindow - we must deref the previous GdkWindow. That's regardless of whether we have an unset_window() method.

> Once again I searched some of the gtk+ code. As far as I can see, no one ever
> calls gtk_widget_set_window(NULL). GtkWidget::priv.window is nullified only
> when a widget is unrealized, and that's done with a call to
> gtk_widget_unrealize() or gtk_widget_real_unrealize().

OK. Thanks.

[snip]
> let's at least wait
> until we know what will happen with bug 639419.

Yes.

We are not getting anywhere with bug #639419 even though we have a patch. I would like to just ignore this function if possible because it would be so awkard to work around its strangeness. I thought I had seen a patch removing its use from our custom widget example, or do we really still need to call it at all?

I tested the custom_widget example with set_has_window(false), i.e. it uses
its parent's Gdk::Window to draw on. The GUI looks exactly the same, but I
can't say if this is something that can be recommended for all custom widgets.
Perhaps it would fail in a more complicated widget. I think you have to ask
a gtk+ developer, if you want to know. The gtk+ widgets behave very
differently. Some use their parent's GdkWindow, some create their own GdkWindow
and store it with gtk_widget_set_window(), some create their own GdkWindow but
do not store it with gtk_widget_set_window().

(In reply to comment #28)
> Well, more obviously, we need to handle someone re-setting the GdkWindow - we
> must deref the previous GdkWindow. That's regardless of whether we have an
> unset_window() method.

Is this really necessary? The description of gtk_widget_set_window() says quite
clearly: "This function should only be used in a widget's #GtkWidget::realize
implementation." And when the GtkWidget::realize signal is sent,
widget->priv->window == NULL. Or at least that's what I think and hope.
Perhaps you can find a gtk+ developer who can confirm.

If I'm right, we can either leave Gtk::Widget::set_window() as it is, or add
a test that it's not called in the wrong situation. Perhaps

void Widget::set_window(const Glib::RefPtr<Gdk::Window>& window)
{
  g_return_if_fail(!gtk_widget_get_window(gobj())); // New test

  gtk_widget_set_window(gobj(), Glib::unwrap(window));
  if (window)
    window->reference(); // gtk_widget_set_window does not add a ref.
}

The gtk+ bug 639419 has been closed. gtk_widget_set_window() won't be changed.
Shouldn't we close this bug, too? The original problem with error messages from
the custom widget example in the gtkmm tutorial was fixed 5 years ago by adding
a reference in Gtk::Widget::set_window().

Sure. Thanks for giving it thought.