GNOME Bugzilla – Bug 154498
Unnecessary warning on console: signalproxy_connectionnode.cc
Last modified: 2011-07-19 14:08:56 UTC
Martin's fix for a memory leak in libsigc++ means that a lot more objects are actually getting deleted which is good, but it's revealed that glibmm seems to be producing an unnecessary g_warning when the object a signalproxy is watching is destroyed. In SignalProxyConnectionNode::notify the comment says: // If there is no object, this call was triggered from destroy_notify_handler() // because we set conn->object to 0 there: which sounds reasonable enough, but the code then goes on to emit a g_warning when it detects this case, which looks ugly and unprofessional. My particular case is one where both gtk and gtkmm destroy a single object on the same event. The last ref to a widget goes away causing gtk to destroy it, which leads in turn to destroying a GtkAction. destroy_notify_handler is called on the signalproxy for that action, but after this happens, gtkmm is notified that the widget went away causing the gtkmm wrapper to be deleted which in turn notifies the GtkAction's wrapper through sigc::trackable. Thus we hit this particular case. There doesn't seem to be anything wrong with this course of events so the warning is inappropriate. It's a trivial change which I can make myself if this is the correct thing to do.
Does every gtkmm example does this? I'd like to see a test case (or named exampe) so that I can at least investigate in detail later. For now, please go ahead and comment out the g_warning(), but also try to investigate with valgrind/memprof in case we are now leaking other memory. Remember to edit the ChangeLog. Thanks.
Created attachment 32253 [details] Test case Ok, I've narrowed the particular case down and I've attached a test case. The scenario is that I have an action which is connected to a mem_fun of an object but also has the action itself bound as a parameter. As the action is bound as a Glib::RefPtr, this creates a circular reference which would normally keep the action around forever. In this scenario the initial Action RefPtr goes out of scope so that the only ref left is the one in the bound RefPtr. Then the object goes out of scope, causing the mem_fun to be invalidated. This disconnects the action which in turn causes the bound RefPtr to disappear which reduces the action's refcount to zero, and then the action disappears too. I can't fully work out what the exact sequence of events is, but the main point is that I don't see anything wrong with this particular scenario, so the warning seems unjustified. The testcase also illustrates that it is better usage to use a sigc::ref around the bound refptr to avoid increasing the refcount. There's no need for an extra ref because, obviously, if the action goes away, it cannot be subsequently activated.
Sorry, using the sigc::ref isn't going to work. As it's going to refer to a particular Glib::RefPtr that's now long out of scope. In fact, the correct thing to do is dereference the Glib::RefPtr<Action> and bind the raw pointer to the mem_fun, but we're not supposed to dereference RefPtrs, so what's the right thing to do?
> In this scenario the initial Action RefPtr goes out of scope so that the only > ref left is the one in the bound RefPtr No, I think that sigc::bind should take a copy of the RefPtr<>, therefore keeping the object (referred to by the RefPtr) alive. I see no need, or sense, in using this sigc::ref() thing. If that works, and doesn't leak, and you've removed the unnecessary warning, then what's the problem?
I only wanted to confirm with you that the warning was indeed unnecessary, now that we have a specific test case. If you still agree that it's unnecessary, I'll make the change.
And actually, I remain concerned. If notify() is really getting called after destroy_notify_handler(), this means that conn has been deleted so we shouldn't be dereferencing it to check object_. If this doesn't crash, it just means we're lucky. So, I think something is actually wrong.
Yes, for now, please go ahead and comment out the g_warning(), but also try to investigate with valgrind/memprof in case we are now leaking other memory. Remember to edit the ChangeLog. But this bug will remain open until I have investigated your test case properly.
Ok, memprof does not report any memory leaks with libsigc++ 2.0.6-test1 which is good news. I also used valgrind to confirm that my testcase which generates the warning is unsafe, in that it dereferences conn after it's been deleted.
Created attachment 32619 [details] test2.cc
Created attachment 32620 [details] valgrind_log.txt Yes, valgrind does seem to find that problem. Here is a slightly-changed test case and the valgrind output from it when I do valgrind --tool=memcheck --num-callers=20 --log-file=valgrind_log.txt ./a.out (That's a new version of valgrind, with new command-line options.) So this needs some more investigation/fixing.
This is still a problem with the lastest versions.
Interestingly this problem is gone (according to valgrind) when using the alternate visit_each_type() code (now in effect in cvs).
I need to test this with the latest releases.
The valgrind warnings still show up with the latest stuff.
There are still valgrind warnings on Ubuntu 10.10 with gtkmm 2.20.3, glibmm 2.25.5, and sigc++ 2.2.4.2 This bug is similar to the gtkmm bug 564005. The libsigc++ bug 321552 and bug 589202 also show valgrind warnings due to accesses to a deleted object, when a slot is being deleted. Those bugs are slightly different, Glib::SignalProxyConnectionNode is not involved. The reporters of bugs 564005 and 589202 mention that the accesses to freed memory cause a segfault on Windows and Solaris 10, probably because freed memory is (wisely) filled with a non-zero bit pattern. I've suggested solutions in bug 564005 comment 8 and bug 589202 comment 8, but I suspect that I don't fully understand what's the fundamental error, causing all these accesses to deleted objects. Isn't it time to change the title of this bug? It's not about an unnecessary warning from SignalProxyConnectionNode any more, is it? The warning is gone. And perhaps it was not unnecessary after all. It is an error when SignalProxyConnectionNode::notify() is called after destroy_notify_handler(), because that means a deleted SignalProxyConnectionNode object is accessed, as mentioned 6 years ago by Philip in comment 6.
Created attachment 190870 [details] [review] patch: SignalProxyConnectionNode: Avoid access to deleted object. This patch fixes Philip's concern in comment 6. This patch only partly fixes the problem with accesses to a deleted object. There will be no such accesses in SignalProxyConnectionNode::notify(), even if it is called after destroy_notify_handler(), because it will be called with data == 0. But there are still such accesses in sigc::internal::slot_rep::disconnect(). The patch in bug 564005 comment 12 fixes all illegal accesses in the test case. If that patch is accepted, this SignalProxyConnectionNode patch is strictly unnecessary, at least for the test cases in this bug and bug 564005. But there may be other situations where it can be useful.
So, now that bug #564005, we can close this bug too?
(In reply to comment #17) > So, now that bug #564005, we can close this bug too? I mean, now that bug #564005 is fixed, we can close this bug too?
(In reply to comment #18) > now that bug #564005 is fixed, we can close this bug too? Yes, that's right. Closing it. There's really no need to push the patch in comment 16. It's unnecessary, now that the patch in bug 564005 comment 18 has been applied to libsigc++.