Ok...The browsermenu.cpp code for delete_sel had a small mistake.
It used a forward iterator on the p->mbSelection vector of items, which
were being deleted as it worked (apparantly by a callback in another thread
in browsertree.cpp in ctree_unselected).
So if you select alot of items and use the edit menu to remove them,
the iterator "i" would become lost, and could wander past the end of
the vector. Then (*i) would be null, which would be derefenced, and
thus segfault.
Most loops are reverse_iterators, which seems to fix this here. But I do
not like this fix here since it seems too fragile.
The better solution is to avoid alot of threads mucking with the list
at the same time. I am submitting a patch that creates a duplicate of
the mbSelection list (or the m_plSelected list in the other case in
delete_sel). The function can then iterate through this list without
problems.
I search for functions that touched mbSelection and tried to fix any
that looked suspicious (but not quite all). These other changes
are simple, but untested.
So there are three patches for the three files I altered, but the
browsermenu.cpp is the most critical one.
Time to sleep.
--
Chris Kuklewicz
--- freeamp/ui/musicbrowser/unix/src/gtkmusicbrowser.cpp Mon Sep 11 02:39:38
2000
+++ freeamp-mine/ui/musicbrowser/unix/src/gtkmusicbrowser.cpp Thu Sep 14 01:16:48
+2000
@@ -1189,13 +1189,16 @@
void GTKMusicBrowser::DeleteEvent(void)
{
if (GetClickState() == kContextPlaylist) {
- set<uint32>::reverse_iterator i = m_plSelected.rbegin();
- for (; i != m_plSelected.rend(); i++)
+ set<uint32> local_plSelected(m_plSelected);
+ set<uint32>::iterator i = local_plSelected.begin();
+ for (; i != local_plSelected.end(); i++) {
DeletePlaylistItem(*i);
+ }
}
else if (GetClickState() == kContextBrowser) {
- vector<TreeData *>::reverse_iterator i = mbSelections->rbegin();
- for (; i != mbSelections->rend(); i++) {
+ vector<TreeData *> local_mbSelections(*mbSelections);
+ vector<TreeData *>::iterator i = local_mbSelections.begin();
+ for (; i != local_mbSelections.end(); i++) {
switch ((*i)->type) {
case kTreePlaylist: {
m_context->catalog->RemovePlaylist((*i)->playlistname.c_str());
@@ -1393,8 +1396,9 @@
}
else if (GetClickState() == kContextBrowser) {
vector<PlaylistItem *> *list = NULL;
- vector<TreeData *>::iterator i = mbSelections->begin();
- for (; i != mbSelections->end(); i++) {
+ vector<TreeData *> local_mbSelections(*mbSelections);
+ vector<TreeData *>::iterator i = local_mbSelections.begin();
+ for (; i != local_mbSelections.end(); i++) {
switch ((*i)->type) {
case kTreePlaylist: {
CreateNewEditor((char*)(*i)->playlistname.c_str());
--- freeamp/ui/musicbrowser/unix/src/browsermenu.cpp Mon Sep 11 02:39:38 2000
+++ freeamp-mine/ui/musicbrowser/unix/src/browsermenu.cpp Thu Sep 14 00:57:14
+2000
@@ -301,10 +301,15 @@
static void delete_sel(GTKMusicBrowser *p, guint action, GtkWidget *w)
{
string urlToDel;
+
if (p->GetClickState() == kContextPlaylist) {
- set<uint32>::reverse_iterator i = p->m_plSelected.rbegin();
- for (; i != p->m_plSelected.rend(); i++) {
+ // Chris Kuklewicz <[EMAIL PROTECTED]> writes: To be paranoid, I
+ // am adding a local copy of this container, like in the
+ // kContextBrowser case.
+ set<uint32> local_plSelected(p->m_plSelected);
+ set<uint32>::iterator i = local_plSelected.begin();
+ for (; i != local_plSelected.end(); i++) {
urlToDel = p->GetContext()->plm->ItemAt(*i)->URL();
if (p->AskToDelete(urlToDel))
@@ -312,8 +317,15 @@
}
}
else if (p->GetClickState() == kContextBrowser) {
- vector<TreeData *>::iterator i = p->mbSelections->begin();
- for (; i != p->mbSelections->end(); i++) {
+ // Chris Kuklewicz <[EMAIL PROTECTED]> writes:
+ // p->mbSelections mutates badly as you erase elements! (in another thread?)
+ // This function *must* make a local copy of the list of items first
+ // The kContextPlaylist uses reverse iterators to fight this, but
+ // kContextBrowser still used forward iterators.
+ // Copying the TreeData*'s to another container is a cleaner solution.
+ vector<TreeData *> local_mbSelections(*(p->mbSelections));
+ vector<TreeData *>::iterator i = local_mbSelections.begin();
+ for (; i != local_mbSelections.end(); i++) {
TreeNodeType type = (*i)->type;
if (type == kTreePlaylist)
--- freeamp/ui/musicbrowser/unix/src/browsertree.cpp Wed Aug 30 16:00:33 2000
+++ freeamp-mine/ui/musicbrowser/unix/src/browsertree.cpp Thu Sep 14 01:26:46
+2000
@@ -95,8 +95,9 @@
{
vector<PlaylistItem *> *newlist = new vector<PlaylistItem *>;
- vector<TreeData *>::iterator iter = mbSelections->begin();
- for (; iter != mbSelections->end(); iter++) {
+ vector<TreeData *> local_mbSelections(*mbSelections);
+ vector<TreeData *>::iterator iter = local_mbSelections.begin();
+ for (; iter != local_mbSelections.end(); iter++) {
TreeData *data = *iter;
if (!data)
PGP signature
