GNOME Bugzilla – Bug 708130
signal handler spuriously runs when signal is emitted by object not connected to once every 98 emissions
Last modified: 2014-08-17 18:23:33 UTC
Created attachment 254997 [details] stacktrace I guess there's a race condition involving computer_timer and computer_move_cb. Sometimes game.current_color is not the computer's color when this cb runs. Might be related to Bug #679137, but I've never seen it place the wrong color piece.
The following obvious workaround, avoiding the use of the untrustworthy current_color variable, private bool computer_move_cb () { assert (light_computer != null || dark_computer != null); if (light_computer != null) light_computer.move (); else dark_computer.move (); computer_timer = 0; return false; } initially appears to work, so I'm posting this warning: it eventually leads to the computer playing both players.
*** Bug 679137 has been marked as a duplicate of this bug. ***
I think the fix for Bug #708343 will fix this as well.
(For the record, Bug #679137, two posts above, is another color-swapping bug, but seems to have a different cause.) FYI I haven't tried your patch yet, but I think this issue is separate. The problem I've found here is that game_move_cb(), attached to the Game object's move() signal, is sometimes (not sure when) being triggered by the move() signal from a copy of the original Game object used in the AI's search, rather than the original Game. So I'm thinking this is a bug in Vala signals.... My "fix" is for the Game to pass itself when calling the move() signal, and at the top of game_move_cb() just verify that it matches the global Game.
Got it... after I posted the comment I realized there might be more going on here. The patch certainly does fix *some* cases of this happening, but it would only be in cases where we would fail to disable computer_timer, e.g. after an undo or after starting a new game.
A workaround has been pushed, but we need to figure out the underlying problem. I still suspect a Vala bug.
*** Bug 732067 has been marked as a duplicate of this bug. ***
Created attachment 278968 [details] sanity check A sanity check to make sure emitting a signal on a new object does not trigger a signal handler connected to the old object. It works -- nothing wrong here.
Created attachment 278969 [details] minimal reproducer A minimal reproducer that shows that when a new object is created and a signal is fired on the new object, a callback registered to that signal on a DIFFERENT object may sometimes be invoked. This happens exactly once every 98 objects. Expected result: ./test 1 Actual result: ./test 1 99 197 295 393 491 589 687 785 883 981 Two real-world examples of breakage: https://git.gnome.org/browse/iagno/commit/?id=73f3e72b6aa5425e33cb4c1f41f30518c23f0ab1 https://git.gnome.org/browse/gnome-chess/commit/?id=a3f1007fe7ea7270c188798e61cd4b7e7c00190e
The bug can be avoided by inheriting Signaler from GLib.Object, though I'm not sure why The Vala tutorial and documentation both state that signals don't work at all except for objects that inherit from GLib.Object, but that is clearly incorrect.
I can't find any culprit in the vala generated code. Maybe it's the case to create a simple C file to reproduce the problem and notify the glib developers.
I can't reproduce this with glib master and vala 0.24.0.
(In reply to comment #12) > I can't reproduce this with glib master and vala 0.24.0. I was using glib 2.38.2 with vala 0.22.1, but I can also reproduce using glib master with vala master :/
(In reply to comment #12) > I can't reproduce this with glib master and vala 0.24.0. I apologize -- my attachment is incorrect. I accidentally attached a file where Signaler inherits from GObject, which I discovered works fine, as noted in comment #10. To reproduce the bug, Signaler has to be a fundamental type (which is common in Vala, for better or for worse). I'll attach a corrected test file now.
Created attachment 279080 [details] minimal reproducer (for real this time)
Created attachment 279081 [details] C reproducer I'm not sure I'd call this "minimal," but it compiles and runs and demonstrates the bug without using Vala. I tried to define this type basically the same as Vala does.
It's fairly clear from the C example that Vala is in the wrong. It's making a fundamental type. The signal code boxes the object into a GValue (through collect_value) and then unsets the GValue (through free_value). Since the refcount is initialized to 0, this means that emitting a signal bumps it to 1 temporarily and then unrefs it back to 0, where it's freed. Because of the way the slice allocator works, and how signals on non-objects work, every so often the instance pointer is "recycled" and thus you get new objects with the same address. This never happens with GObjects since GObjects start their refcounting at 1. Chances are, you always want to be using GObjects. Vala emitting fundamentals "by default" is actually pretty crazy stuff I didn't know about, but that's their decision. It's still pretty broken to start the refcounting of the fundamental at 0 -- that means it's floating, and doing lots of stuff will then break.
(In reply to comment #17) > It's still pretty broken to start the refcounting of the > fundamental at 0 -- that means it's floating, and doing lots of stuff will then > break. (In reply to comment #17) > It's fairly clear from the C example that Vala is in the wrong. It's making a > fundamental type. The signal code boxes the object into a GValue (through > collect_value) and then unsets the GValue (through free_value). Since the > refcount is initialized to 0, this means that emitting a signal bumps it to 1 > temporarily and then unrefs it back to 0, where it's freed. Can you explain a little better? Where do you read it starts at 0? In the signaler_instance_init it's set to 1.
The refcount of the TestTestSignaler starts at 0, which means that a ref/unref pair will destroy the object.
(In reply to comment #19) > The refcount of the TestTestSignaler starts at 0, which means that a ref/unref > pair will destroy the object. The attached C test case is wrong. Vala generates signaler_instance_init that sets the ref_count to 1. static void signaler_instance_init (Signaler * self) { gint _tmp0_ = 0; gint _tmp1_ = 0; _tmp0_ = signaler_count; signaler_count = _tmp0_ + 1; _tmp1_ = signaler_count; self->id = _tmp1_; self->ref_count = 1; } Probably that has been overlooked when rewriting the cleaned up C test case.
Now the loop says: _tmp7_ = signaler_new (); _signaler_unref0 (signaler); signaler = _tmp_; That unref there will free the signaler, causing the next one to reuse the pointer. If you *really* want to use fundamentals by default, and I *strongly* suggest you don't because of weird bugs like this, then you need to put a g_signal_handlers_destroy (instnace); in the finalizer by default. This is what GObject does.
Where does glib keep track of signals? Is it a kind of dictionary instance->signals? Then I can understand more the problem.
So I'm going to add g_signal_handlers_destroy (self) in the finalize function, does that look fine to you Jasper? Except the fact that we use fundamentals (not something we can't change at this point anyway).
(In reply to comment #22) > Where does glib keep track of signals? Is it a kind of dictionary > instance->signals? Then I can understand more the problem. Where else would it store it? Your structure doesn't have any slots for signals. But yes, it's stored in a hashmap keyed on the instance pointer. Search for "g_handler_list_bsa_ht" in gsignal.c (In reply to comment #23) > So I'm going to add g_signal_handlers_destroy (self) in the finalize function, > does that look fine to you Jasper? Except the fact that we use fundamentals > (not something we can't change at this point anyway). That should be fine. Can you at least warn on that unless the user has an explicit annotation or something? This won't be the last issue with fundamentals you run into.
What other issue besides memory initialization?
(In reply to comment #23) > So I'm going to add g_signal_handlers_destroy (self) in the finalize function, > does that look fine to you Jasper? Wonderful! (In reply to comment #20) > The attached C test case is wrong. Vala generates signaler_instance_init that > sets the ref_count to 1. Yup, my bad -- though a good thing I got that wrong, since I wasn't unreffing TestTestSignaler, I wouldn't have been able to reproduce this otherwise. (In reply to comment #24) > Can you at least warn on that unless the user has an explicit annotation or > something? This won't be the last issue with fundamentals you run into. Roughly 80% (guess) of the classes used in GNOME Games and Calculator (the programs I checked today) are fundamental types, and there is no rhyme or reason why some are and others aren't: it's just a matter of whether the author remembered to inherit from GLib.Object when declaring the class. Usually not, since inheritance from Object is automatic in Java and C#, and omitting it in Vala doesn't seem to make any difference at all to the program's behavior, at least not to clueless programmers like me. So if inheriting from GLib.Object is recommended, then it's bad that's not the default. Jasper is suggesting that you could emit a warning whenever someone declares fundamental type that's not annotated with [Fundamental], similar to [SimpleType] and [Compact], except [Fundamental] would only serve to silence the warning. That would preserve backwards-compatibility, and anyone irritated by the warning can either add the annotation or derive from GLib.Object as they prefer.
(In reply to comment #26) > Roughly 80% (guess) of the classes used in GNOME Games and Calculator (the > programs I checked today) are fundamental types, and there is no rhyme or > reason why some are and others aren't: it's just a matter of whether the author > remembered to inherit from GLib.Object when declaring the class. Usually not, > since inheritance from Object is automatic in Java and C#, and omitting it in > Vala doesn't seem to make any difference at all to the program's behavior, at > least not to clueless programmers like me. So if inheriting from GLib.Object is > recommended, then it's bad that's not the default. > There's a big difference, you don't get data lists, gobject properties, weak references, and can't implement interfaces that require gobject. If gobject is not inherited in some code it's because it's not needed. > Jasper is suggesting that you could emit a warning whenever someone declares > fundamental type that's not annotated with [Fundamental], similar to > [SimpleType] and [Compact], except [Fundamental] would only serve to silence > the warning. That would preserve backwards-compatibility, and anyone irritated > by the warning can either add the annotation or derive from GLib.Object as they > prefer. The documentation explains the difference between a non-gobject class and a gobject class. If there's something to warn about, that should be in the documentation not in the valac output. Needing to add [Fundamental] is annoying and pointless thing. [Compact] and [SimpleType] are special and usually there for bindings reason.
(In reply to comment #27) > There's a big difference, you don't get data lists, gobject properties, weak > references, and can't implement interfaces that require gobject. > If gobject is not inherited in some code it's because it's not needed. It could very well be, but I think a good majority of people who are using Vala simply don't know better, and end up with fundamentals by accident. It's too easy to write "class Foo {" in your editor instead of "class Foo : Object {". If I didn't know better, I'd actually think the latter was redundant and delete it. This is really poor language design, and you're not setting your users up for success. Fundamental types are a pain to deal with, and you *should* be guiding users towards creating GObjects instead of fundamentals.
(In reply to comment #28) > This is really poor language design, and you're not setting your users up for > success. Fundamental types are a pain to deal with, and you *should* be guiding > users towards creating GObjects instead of fundamentals. I've asked for some example (other than this of signals), if you can provide any other case I'm up to fix these problems. Vala is there since several years, and this is the only problem we encountered so far.
commit b93f6e6d7ff3b79a6b97c92aea857fcb366fa7ba Author: Luca Bruno <lucabru@src.gnome.org> Date: Fri Jun 27 17:14:39 2014 +0200 Call g_signal_handlers_destroy on gtypeinstance finalizer Fixes bug 708130 This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
(In reply to comment #29) > (In reply to comment #28) > > This is really poor language design, and you're not setting your users up for > > success. Fundamental types are a pain to deal with, and you *should* be guiding > > users towards creating GObjects instead of fundamentals. > > I've asked for some example (other than this of signals), if you can provide > any other case I'm up to fix these problems. Vala is there since several years, > and this is the only problem we encountered so far. Bug #734927 seems related (though nowhere near as serious as this one was).
There's also introspection-related issues, where fundamentals aren't supported very well in most bindings. It really matters if you're creating a library using Vala.