GNOME Bugzilla – Bug 733285
gboolean grl_initialized should be reset to FALSE in grl_deinit
Last modified: 2016-08-22 11:29:54 UTC
The grl_initialized static variable in grilo.c should be reset in grl_deinit() for reloads to work. Without this rhythmbox doesn't reload grilo plugins after an unload. static gboolean grl_initialized = FALSE; /** * grl_init: * @argc: (inout) (allow-none): number of input arguments, length of @argv * @argv: (inout) (element-type utf8) (array length=argc) (allow-none): list of arguments * * Initializes the Grilo library * * Since: 0.1.6 */ void grl_init (gint *argc, gchar **argv[]) { ... ... grl_initialized = TRUE; } /** * grl_deinit: * * Deinitializes the Grilo library. * * Call this function after finalizing using Grilo, in order to free and clean * up all the resources created. * * Since: 0.2.8 */ void grl_deinit (void) { GrlRegistry *registry; if (!grl_initialized) { GRL_WARNING ("Grilo has not been initialized"); return; } registry = grl_registry_get_default (); grl_registry_shutdown (registry); }
Steps to reproduce: 1. Open rhythmbox -> Plugins 2. Keep flipping the "Grilo media browser" option. Expected result: Jamendo and RadioFrance should be displayed everytime the plugin in enabled. Actual result: None of the 2 plugins is loaded when the grilo plugin is enabled. Note: This issue is with grl_deinit() ( which was recently added to grl ) call added to rb-grilo-plugin.c ( in impl_deactivate() ). This change is not in rhythmbox git yet.
Created attachment 280898 [details] Jamendo and RadioFrance are not loaded even when grilo is loaded.
I quickly checked by adding grl_deinit() to impl_deactivate() in rb-grilo-plugin.c followed by a "gdb break" at grl_deinit(), followed by a "gdb finish" on grl_deinit() and set grl_initialized=0 and "gdb continue" With this quick hack, both Jamendo and RadioFrance were displayed in rhythmbox.
Created attachment 280926 [details] [review] core: Make it possible to init() after a deinit() Reinitialise grl_initialized when calling grl_deinit(). This makes it possible to reload plugins again after they've been disabled. Spotted by vrishab <vrishab.in@gmail.com>
With the above change, Jamendo and RadioFrance are successfully loaded. However, there is a crash in grl-tracker-utils.c after a few attempts of load / unload of the grilo plugin in rhythmbox. The crash is consistently at the same location. There is a SIGSEGV because there is no keyname corresponding to a key in the grl registry. static tracker_grl_sparql_t * insert_key_mapping (GrlKeyID grl_key, const gchar *sparql_key_attr, const gchar *sparql_key_attr_call, const gchar *sparql_key_flavor) { ... ... gchar *canon_name = g_strdup (GRL_METADATA_KEY_GET_NAME (grl_key)); assoc->sparql_key_name = build_flavored_key (canon_name, sparql_key_flavor); ... ... } Hence, canon_name is NULL, and the code segfaults in build_flavored_key(). I've a few questions here: 1. Has libgrl been tested to work fine across reloads within the same process ? 2. Are there any tests for 1 ? I'm getting an idea, that this part of the code has not been tested, which is why this bug existed in the first place. Correct me if I'm wrong. If that's the case, fixing this issue might involve more changes than just adding the 'grl_initialized' flag to grl_deinit(). Comments ?
Crash location: Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 233837
Thread 5 (Thread 0x7ffff7f95a40 (LWP 25756))
(In reply to comment #5) > > 1. Has libgrl been tested to work fine across reloads within the same process ? > 2. Are there any tests for 1 ? > > I'm getting an idea, that this part of the code has not been tested, which is > why this bug existed in the first place. Correct me if I'm wrong. If that's the > case, fixing this issue might involve more changes than just adding the > 'grl_initialized' flag to grl_deinit(). > > The point is that we added grl_deinit() to avoid leaking some directories. The idea was that you run once the grl_init() and once the grl_deinit(). It was not designed to run a grl_deinit() followed by a grl_init() again. So I'm afraid it requires other changes too.
(In reply to comment #5) > With the above change, Jamendo and RadioFrance are successfully loaded. > However, there is a crash in grl-tracker-utils.c after a few attempts of load / > unload of the grilo plugin in rhythmbox. The crash is consistently at the same > location. There is a SIGSEGV because there is no keyname corresponding to a key > in the grl registry. > Do you know what is the value of grl_key? Could you upload the stack trace?
Do you want me to upload the core file ? I've pasted the stack trace already above ( Trace 233837 )
Before uploading the core file, could you tell me what is the value of grl_key when the crash happens? Maybe that's enough
(gdb) up
+ Trace 233862
grl_key = 2 sparql_key_attr = 0x0 sparql_key_attr_call = 0x7f69010846f8 "nmm:artistName(nmm:performer(?urn))" sparql_key_flavor = 0x7f6901084209 "audio" (gdb) info locals assoc = 0x3b64a20 assoc_list = 0x0 canon_name = <optimized out> I had 3 crashes. All 3 had the same values for 'info args'
Thanks! Finally I'm able to reproduce the bug. I'll be checking.
Comment on attachment 280926 [details] [review] core: Make it possible to init() after a deinit() Committed as @ce2f0d38b73b