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 684557 - NULL pointer in folks_name_details_change_full_name callback => segfault
NULL pointer in folks_name_details_change_full_name callback => segfault
Status: RESOLVED FIXED
Product: vala
Classification: Core
Component: Code Generator: GAsync
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Vala maintainers
Vala maintainers
Depends on:
Blocks:
 
 
Reported: 2012-09-21 13:31 UTC by Patrick Ohly
Modified: 2012-11-11 18:35 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Patrick Ohly 2012-09-21 13:31:24 UTC
I'm trying to use folks_name_details_change_full_name() on a FolksIndividual which was created with folks_individual_new().

What I see is that my GAsyncReadyCallback gets invoked with a NULL GObject pointer. When it passes that pointer on to folks_name_details_change_full_name_finish(), that function segfaults.

  • #0 folks_name_details_change_full_name_finish
    at /work/gnome/checkout/folks/folks/name-details.vala line 214
  • #1 SyncEvo::GAsyncReadyCXX<void, void
  • #2 g_simple_async_result_complete
    at gsimpleasyncresult.c line 775
  • #3 complete_in_idle_cb
    at gsimpleasyncresult.c line 787
  • #4 g_main_dispatch
    at gmain.c line 2715
  • #5 g_main_context_dispatch
    at gmain.c line 3219
  • #6 g_main_context_iterate
    at gmain.c line 3290


The root cause is that folks_name_details_real_change_nickname() passes NULL to g_simple_async_result_new() instead of self:

static void folks_name_details_real_change_nickname (FolksNameDetails* self, const gchar* nickname, GAsyncReadyCallback _callback_, gpointer _user_data_) {
	FolksNameDetailsChangeNicknameData* _data_;

...	_data_ = g_slice_new0 (FolksNameDetailsChangeNicknameData);
	_data_->_async_result = g_simple_async_result_new (NULL, _callback_, _user_data_, folks_name_details_real_change_nickname);

I'm a bit at a loss here. This is in code generated by Vala - is this a bug in Vala?

Originally I tried using folks_name_details_set_full_name(), but that didn't have any effect. I'm still unsure why - stepping through the C source code in gdb is impossible, because gdb only shows the single line of Vala code that generated the C code.

I compiled GNOME master with jhbuild a few days ago.
Comment 1 Philip Withnall 2012-09-21 13:55:00 UTC
(In reply to comment #0)
> What I see is that my GAsyncReadyCallback gets invoked with a NULL GObject
> pointer. When it passes that pointer on to
> folks_name_details_change_full_name_finish(), that function segfaults.

Looks like Vala is producing bad C code for virtual async methods on interfaces. Normally this isn’t a problem, because the correct code is generated for the overriding method in a class implementing that interface has the correct code; but in this case, Individual doesn’t override change_full_name(), so the default (incorrect) implementation gets called and explodes.

This should only happen with change_full_name() and not change_nickname() because Individual does actually implement change_nickname() (and the C code is correct).

> I'm a bit at a loss here. This is in code generated by Vala - is this a bug in
> Vala?

In summary: yes. What Vala version are you using?

> Originally I tried using folks_name_details_set_full_name(), but that didn't
> have any effect. I'm still unsure why - stepping through the C source code in
> gdb is impossible, because gdb only shows the single line of Vala code that
> generated the C code.

set_full_name() should have an effect, but it won’t be immediate — you should get a property change notification in an idle callback some time later. As the documentation says, set_full_name() is not async, so won’t wait for the change to be propagated to the backing store. change_full_name(), however, will, and the property change will be completed before the async call returns.

To make gdb show C line numbers instead of Vala ones, remove the ‘-g’ from ERROR_VALAFLAGS in configure.ac.
Comment 2 Patrick Ohly 2012-09-21 13:58:13 UTC
If I read the following code correctly, full name is never writable:

static gboolean folks_name_details_real_change_full_name_co (FolksNameDetailsChangeFullNameData* _data_) {
	switch (_data_->_state_) {
		case 0:
		goto _state_0;
		default:
		g_assert_not_reached ();
	}
	_state_0:
	_data_->_tmp0_ = NULL;
	_data_->_tmp0_ = _ ("Full name is not writeable on this contact.");
	_data_->_tmp1_ = g_error_new_literal (FOLKS_PROPERTY_ERROR, FOLKS_PROPERTY_ERROR_NOT_WRITEABLE, _data_->_tmp0_);
	_data_->_inner_error_ = _data_->_tmp1_;

Is that because I constructed a new FolksIndividual which doesn't have a writable backend store?

I was trying to construct a non-empty FolksIndividual instance for testing. Is that approach doomed?

I was also expecting to populate a FolksIndividual before persisting it in the backend storage, but it seems that this is not how it works. Instead I have to fill in some hash for folks_individual_aggregator_add_persona_from_details(). Is that hash documented somewhere?

I understand why you are doing this (not all properties of a FolksIndividual can be stored), but IMHO it would be much more natural to use the existing FolksIndividual set API to create a new contact instead of another data structure.
Comment 3 Patrick Ohly 2012-09-21 14:01:49 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > I'm a bit at a loss here. This is in code generated by Vala - is this a bug in
> > Vala?
> 
> In summary: yes. What Vala version are you using?

My jhbuild source checkout says:

commit 69189c9204eac311c894fa5eafba649f3881f960
Author: Evan Nemerson <evan@coeus-group.com>
Date:   Tue Sep 4 20:49:24 2012 -0700

    gtk+-3.0: mark ResizeMode.IMMEDIATE as deprecated
    
    Fixes bug 679771.

That's 0.17.6-3-g69189c9

> > Originally I tried using folks_name_details_set_full_name(), but that didn't
> > have any effect. I'm still unsure why - stepping through the C source code in
> > gdb is impossible, because gdb only shows the single line of Vala code that
> > generated the C code.
> 
> set_full_name() should have an effect, but it won’t be immediate — you should
> get a property change notification in an idle callback some time later. As the
> documentation says, set_full_name() is not async, so won’t wait for the change

I missed that in the documentation. I was expecting it to be synchronous. Where is that documented?

> to be propagated to the backing store. change_full_name(), however, will, and
> the property change will be completed before the async call returns.

What if there is no backing store? See my other comment about "Full name is not writeable on this contact."

> To make gdb show C line numbers instead of Vala ones, remove the ‘-g’ from
> ERROR_VALAFLAGS in configure.ac.

Ah, good to know.
Comment 4 Jürg Billeter 2012-11-11 18:35:01 UTC
commit 123eead8c4998581f9789b41069f281612863471
Author: Jürg Billeter <j@bitron.ch>
Date:   Sun Nov 11 19:32:10 2012 +0100

    codegen: Fix g_simple_async_result_new in interface methods
    
    Fixes bug 684557.