Bug 13908 - [fixed in git] Collection type specializations not registered correctly
Summary: [fixed in git] Collection type specializations not registered correctly
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: GLib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Rob Taylor
QA Contact: John (J5) Palmieri
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
: 3999 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-01-03 07:27 UTC by Mikkel Kamstrup Erlandsen
Modified: 2009-07-20 05:56 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Sample prog demonstrating the problem (479 bytes, text/plain)
2008-01-03 07:28 UTC, Mikkel Kamstrup Erlandsen
Details
Patch fixing type registration (1.61 KB, patch)
2008-01-04 01:46 UTC, Mikkel Kamstrup Erlandsen
Details | Splinter Review
Updated patch (1.63 KB, patch)
2008-01-04 01:50 UTC, Mikkel Kamstrup Erlandsen
Details | Splinter Review
Simplified patch (1.57 KB, patch)
2008-01-04 05:01 UTC, Mikkel Kamstrup Erlandsen
Details | Splinter Review
Use GOnce (1.41 KB, patch)
2009-05-28 10:49 UTC, Simon McVittie
Details | Splinter Review
Silently initialize whenever needed (9.70 KB, patch)
2009-05-28 10:50 UTC, Simon McVittie
Details | Splinter Review

Description Mikkel Kamstrup Erlandsen 2008-01-03 07:27:14 UTC
When I try to use a DBUS_TYPE_G_UINT_ARRAY or call

dbus_g_type_get_collection ("GArray", G_TYPE_UINT);

directly (which I think the macro does anyway), I get

** CRITICAL **: lookup_or_register_specialized: assertion `specialized_types_is_initialized ()' failed

If I first create a connection to the session bus everything works out fine. 

Calling dbus_g_type_specialized_init() instead of hooking up to the bus gives me an error:

** CRITICAL **: lookup_or_register_specialized: assertion `klass != NULL' failed

Attaching test prog in a sec
Comment 1 Mikkel Kamstrup Erlandsen 2008-01-03 07:28:01 UTC
Created attachment 13484 [details]
Sample prog demonstrating the problem
Comment 2 Mikkel Kamstrup Erlandsen 2008-01-04 01:46:22 UTC
Created attachment 13513 [details] [review]
Patch fixing type registration

The following patch makes it work here if I do a 

dbus_g_type_specialized_init ();

Before referring to any dbus specialized types.
Comment 3 Mikkel Kamstrup Erlandsen 2008-01-04 01:50:41 UTC
Created attachment 13514 [details] [review]
Updated patch

Duh. I forgot to set my type initializer guard to TRUE.

Which also makes we wonder if it was not a (potential) bug before to not have a guard or assertion on the public method dbus_g_type_specialized_init() before..?
Comment 4 Mikkel Kamstrup Erlandsen 2008-01-04 05:01:28 UTC
Created attachment 13515 [details] [review]
Simplified patch

Hmmm, I was in a bit of a rush there. The guard on dbus_g_type_specialized_init() now just checks if specialized_containers!=NULL

Sorry for the noise. I shall keep it at this :-)
Comment 5 Colin Walters 2008-05-27 21:30:08 UTC
*** Bug 3999 has been marked as a duplicate of this bug. ***
Comment 6 Simon McVittie 2008-10-06 09:09:51 UTC
This is hideous. As it stands at the moment, dbus_g_type_specialized_init is never safe for library users to call (it might coincidentally work if it's the very first thing in main() or something, but at that point it's a land-mine in your source code), yet it's public; _dbus_g_value_types_init appears to be vaguely safe (it at least has some attempt at having guards), but it's not public.

The attachment looks reasonable, although it's not thread-safe. (I don't know whether dbus-glib is documented as explicitly not thread-safe; if not, it probably ought to be, at the moment.) The code in the attachment could be made pre-emptively thread-friendly by using g_once_init_enter if dbus-glib depends on 2.14 anyway, or GOnce otherwise.
Comment 7 Mikkel Kamstrup Erlandsen 2008-10-06 12:04:13 UTC
Yes, a Google codesearch reveals that there are a few consumers of this function (there where many more at the time I wrote the patch). My problem is that I actually need it as part of the public API since xesam-glib's unit tests does some type shuffling, but never creates a dbus connection (although I do that now as a workaround).

A cursory browsing of the dbus-glib sources indicates to me that they are not thread safe, and in fact doesn't even depend on GThread.



Comment 8 Simon McVittie 2008-11-28 09:31:15 UTC
In git for your convenience:

http://git.collabora.co.uk/?p=user/smcv/dbus-glib-smcv.git;a=shortlog;h=refs/heads/13908
git://git.collabora.co.uk/git/user/smcv/dbus-glib-smcv.git
Comment 9 Simon McVittie 2009-03-26 11:12:52 UTC
I've rebased this branch, and added an optional patch to use GOnce. Please review.

http://git.collabora.co.uk/?p=user/smcv/dbus-glib-smcv.git;a=shortlog;h=refs/heads/13908
Comment 10 Simon McVittie 2009-04-15 13:16:34 UTC
Colin, any chance you could have a look at this?
Comment 11 Colin Walters 2009-04-16 11:08:40 UTC
Hm.  Is there a reason not to just do the initialization at the time someone calls one of the public API entry points that depends on it?  For example inside dbus-gtype-specialized.c:lookup_or_register_specialized, instead of doing:

  g_return_val_if_fail (specialized_types_is_initialized (), G_TYPE_INVALID);

we do:

  dbus_g_type_specialized_init ();

Comment 12 Simon McVittie 2009-05-28 10:49:02 UTC
Colin: thanks for reviewing. Your suggestion is not as trivial as it sounds, since some of the functions that assert that basic initialization has been completed are themselves called from the initialization code (to register "built-in" specializations, i.e. the only ones anyone ever uses - GHashTable, GPtrArray, GValueArray, GArray - plus for some reason GSList, which I had no idea was supported).

I'll attach a patch, though; up to you whether you consider it to be worthwhile.

This would also lead to code which uses only symbols that have always existed, but only works on sufficiently new versions. I'm not sure how much of a concern that is.
Comment 13 Simon McVittie 2009-05-28 10:49:57 UTC
Created attachment 26278 [details] [review]
Use GOnce
Comment 14 Simon McVittie 2009-05-28 10:50:23 UTC
Created attachment 26279 [details] [review]
Silently initialize whenever needed
Comment 15 Simon McVittie 2009-06-09 07:46:23 UTC
Any opinions? I'd like to get a dbus-glib release out soon with the pending patches applied; I suspect that otherwise I'll end up applying them all in Debian anyway, since several of the pending patches break Telepathy regression tests.
Comment 16 Colin Walters 2009-06-09 08:03:20 UTC
Both patches look good to me, assuming you got all the calls to specialized_types_is_initialized.  Thanks a lot for going the extra mile on these patches, one less spiked pit for dbus-glib users to fall into.

Please commit.  Agree on the release, I'll see if I can put one tother this week after these land.
Comment 17 Colin Walters 2009-06-09 08:07:24 UTC
I forgot to mention, on the second patch though, you might correct my surname =)

(It's not the first, and almost certainly not the last time the Colins have been confused)
Comment 18 Simon McVittie 2009-06-09 08:17:58 UTC
Pushed, thanks.
Comment 19 Simon McVittie 2009-07-20 05:56:20 UTC
Fixed in 0.82, according to Colin's release mail.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.