After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 564944 - Constructors which throw exceptions return garbage
Constructors which throw exceptions return garbage
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Errors
0.5.x
Other All
: Normal normal
: ---
Assigned To: Jürg Billeter
Vala maintainers
Depends on:
Blocks: 567181
 
 
Reported: 2008-12-18 00:34 UTC by Evan Nemerson
Modified: 2009-05-11 08:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fix regression with error propagation (823 bytes, patch)
2009-05-11 08:08 UTC, Michael Chapman
none Details | Review

Description Evan Nemerson 2008-12-18 00:34:46 UTC
Constructors which throw errors generate code (in the *_construct function) code that looks something like this:

Foo* foo_construct (GType object_type, GError** error) {
	GError * inner_error;
	Foo * self;
	inner_error = NULL;
	self = g_object_newv (object_type, 0, NULL);
	inner_error = g_error_new (ERROR, ERROR_FOOBAR, "Foobar!");
	if (inner_error != NULL) {
		g_propagate_error (error, inner_error);
		return;
	}
	return self;
}

Notice that inside of the if block, there is a return without a value, which can cause problems later on (warnings about assertion `G_IS_OBJECT (object)' failed, segfaults). I think that if there is an error, it should either unref the object and return null, or return self and leave it to the caller to unref (which it does).
Comment 1 Jürg Billeter 2008-12-18 13:57:28 UTC
2008-12-18  Jürg Billeter  <j@bitron.ch>

	* gobject/valaccodemethodmodule.vala:

	Fix error propagation in creation methods, fixes part of 564944

Fixed in r2210.
Comment 2 Philip Van Hoof 2008-12-30 22:16:12 UTC
The remaining problem here seems to be that "current_symbol" is null in case the symbol is a constructor.
Comment 3 Philip Van Hoof 2008-12-30 22:16:59 UTC
Bug #557856 is related to this problem, but not a duplicate.
Comment 4 Philip Van Hoof 2008-12-30 22:19:00 UTC
valagerrormodule.vala:

} else if (current_method != null && current_method.get_error_types ().size > 0) {
...
	// free local variables
	var b = (Block) current_symbol;
	var local_vars = b.get_local_variables ();
	message ("free local %s %d\n", current_symbol.name, local_vars.size);
	var free_frag = new CCodeFragment ();
	append_local_free (current_symbol, free_frag, false);
	cerror_block.add_statement (free_frag);
...

}

pvanhoof@tinc:/tmp$ /opt/vala/bin/valac -C test.vala 
test.vala:21.18-21.40: warning: local variable `c' declared but never used
	  MySimpleClass c = new MySimpleClass();
	                ^^^^^^^^^^^^^^^^^^^^^^^
** Message: valagerrormodule.vala:153: free local (null) 0

Compilation succeeded - 2 warning(s)
pvanhoof@tinc:/tmp$
Comment 5 Michael Chapman 2009-05-11 08:04:21 UTC
I think there's been a regression with this.

Currently a constructor that throws an exception generates C code like this:

Foo* foo_construct (GType object_type, GError** error) {
        GError * _inner_error_;
        Foo * self;
        _inner_error_ = NULL;
        self = g_object_newv (object_type, 0, NULL);
        _inner_error_ = g_error_new_literal (ERROR, ERROR_FOOBAR, "Foobar!");
        if (_inner_error_ != NULL) {
                if (_inner_error_->domain == ERROR) {
                        g_propagate_error (error, _inner_error_);
                        return;
                } else {
                        g_critical ("file %s: line %d: uncaught error: %s", __FILE__, __LINE__, _inner_error_->message);
                        g_clear_error (&_inner_error_);
                        return NULL;
                }
        }
        return self;
}

Again, one of the inner branches fails to return a value. Looks like Jürg Billeter's earlier fix for this got lost somewhere.
Comment 6 Michael Chapman 2009-05-11 08:08:01 UTC
Created attachment 134379 [details] [review]
Fix regression with error propagation

Quick patch that seems to restore the correct behaviour.