GNOME Bugzilla – Bug 661217
rhythmbox crashed with SIGSEGV in rb_ipod_static_playlist_source_get_itdb_playlist()
Last modified: 2012-04-08 22:17:32 UTC
this report has been filed here: https://bugs.launchpad.net/ubuntu/+source/rhythmbox/+bug/869445 "was attempting to switch from my iPod music back to the regular music library - I paused the playing son and it froze up before crashing" ".
+ Trace 228713
Thread 1 (Thread 0x7f53dff3e9e0 (LWP 4089))
exact version is : rhythmbox 2.90.1~20110908-0ubuntu1
Hi, I have Rythmbox 2.90.1~git20110919.2dfea6-3 with Debian Testing/Unstable. * my Ipod (Ipod Nano 4th generation, 8GB) is correctly recognized (e.g. I can browse the songs on it from rhythmbox) * when trying to delete a playlist, or synchronizing with the local library, Rythmbox segfaults in rb_ipod_static_playlist_source_get_itdb_playlist () * if I add or remove songs manually (by dragging songs from library to the Ipod icon in Rhytmbox instead than synchronizing the whole library), the songs gets correctly uploaded to the IPod, and nothing crashes (so I think it's something related to playlists management) (sorry, I don't remember if I initialized that IPod with ITunes or with Rhythmbox)
Created attachment 209441 [details] [review] Fix crash when syncing ipod with playlists The backtrace in the ubuntu bug isn't specific enough to say if this is the same bug, but rhythmbox was crashing for me in a similar spot. From the patch description: ipod: Don't call before-save handler on free'd playlist data We weren't unregistering the before-save handler when a playlist was removed. Move the handler register/unregister into static playlist code so we don't need to open code the cleanup. This hits on sync every time for me, since sync seems to by default remove every playlist on the device, then add the playlists user requested to sync. Reproduced with git head, patch is against git head.
Review of attachment 209441 [details] [review]: I don't like this much. This code is already very tangled and this patch only makes it worse. The various signal handlers for the playlist/query model should be in rb-ipod-static-playlist-source.c.
Created attachment 209550 [details] [review] Decouple playlist handlers from ipod source private data
Created attachment 209551 [details] [review] Move playlist handlers to private playlist code
Created attachment 209552 [details] [review] Fix reported segfault
Review of attachment 209550 [details] [review]: this looks OK, a few code style things that I'll fix since I wasn't really expecting you to make these changes.
Review of attachment 209551 [details] [review]: this mostly looks OK, but we can clean up this code a bit more while we're here. ::: plugins/ipod/rb-ipod-static-playlist-source.c @@ +158,3 @@ + Itdb_Playlist *ipod_pl = rb_ipod_static_playlist_source_get_itdb_playlist (playlist); + RBiPodSource *ipod = rb_ipod_static_playlist_source_get_ipod_source (playlist); + RbIpodDb *ipod_db = rb_ipod_static_playlist_source_get_ipod_db (playlist); now that this code is inside rb-ipod-static-playlist-source.c we can just use priv->ipod_source, priv->ipod_db etc. rather than these huge ugly function names. @@ +199,3 @@ + GtkTreeIter iter; + + Itdb_Playlist *ipod_pl = rb_ipod_static_playlist_source_get_itdb_playlist (playlist); just use priv->was_reordered instead. The 'was-reordered' property on this class can be removed now too. @@ +264,3 @@ + +static void + if (gtk_tree_model_get_iter_first (model, &iter)) { the arguments here should be the other way around, and the model parameter should be a RhythmDBQueryModel, not a GObject. This means more casting inside the function, but it makes more sense on the whole. @@ +301,3 @@ + +static void + I'd never looked at this signal handler before, but now I notice that it won't work, because the user_data parameter specified when it's connected is the source, not the base query model. It doesn't matter though, because the base query model for an ipod playlist source will never change. This should just be removed. @@ +333,3 @@ + "base-query-model", &model, NULL); + playlist_source_model_disconnect_signals (G_OBJECT (model), source); + g_object_unref (model); This should be done in impl_dispose, not impl_delete_thyself. Actually everything in impl_delete_thyself should be in impl_dispose.. @@ +364,3 @@ + g_signal_connect (source, "notify::base-query-model", + G_CALLBACK (playlist_source_model_changed), + source); as above, we don't need to handle this.
Review of attachment 209552 [details] [review]: this looks fine once the other changes have gone in.
Created attachment 209696 [details] [review] reworked first patch
Created attachment 209697 [details] [review] reworked second patch
Created attachment 209698 [details] [review] reworked final patch
Can you test this for a while to make sure things still work? Assuming it does, I'm happy to commit this as is.
Great, thanks for picking this up Jonathan. Patch 2 needs this on top, otherwise ipod playlist delete segfaults: diff --git a/plugins/ipod/rb-ipod-source.c b/plugins/ipod/rb-ipod-source.c index eb5b5d1..194807c 100644 --- a/plugins/ipod/rb-ipod-source.c +++ b/plugins/ipod/rb-ipod-source.c @@ -1876,7 +1876,7 @@ rb_ipod_source_remove_playlist (RBiPodSource *ipod_source, rb_display_page_delete_thyself (RB_DISPLAY_PAGE (source)); - g_object_get (playlist_source, "ipod-playlist", &playlist, NULL); + g_object_get (playlist_source, "itdb-playlist", &playlist, NULL); rb_ipod_db_remove_playlist (priv->ipod_db, playlist); } After that sync and playlist addition/deletion work fine. Also patch 2 isn't really mine anymore so please change author. Patch description isn't accurate anymore either, since a lot of extra cleanup is being done.
OK, I've made that change and pushed everything to master. Thanks for all your work on this. Help with ipod support is always welcome.
*** Bug 672882 has been marked as a duplicate of this bug. ***