On Fri, Sep 15, 2000 at 01:42:03PM -0400, Isaac Richards wrote:
>
> On 14-Sep-2000 Chris Kuklewicz wrote:
> > 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).
>
> Ya. seems gtk's going into two callbacks at once, which it shouldn't be able
> to do.
>
Not good. I thought that two threads touching the container w/o a
mutex looked like a concurrency bug.
See the end of this message for an idea of how they are executing
sequentially. In a nutshell: the unselection event is fired for the
music browser window during the main loop for the confirmation dialog
box. (If the message box were "modal" then the other window may not
have its event fired. As it is, one has to expect this).
> > 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 reverse_iterators are safe enough, just in this case I didn't want to
> delete things backwards for some reason...
>
The user expects the list to be presented in the order on screen. When I tried
reverse iterators, it felt odd to be asked about the songs in the "wrong" order.
> > 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 applied the section pertinent to this particular case, and left out the rest.
> If there's still problems in this section of code, I'll get em in, but for
> now, I don't think the rest of the patches are necessary.
>
Like I said, I did not test the other sections for problems. I just tried to
identify where similar code existed. If the concurrecy problem is solved, then
none of these should be a problem. If what I explain below is right, then the
problem is executing the gtk main loop (for the dialog) during the iteration
over the container. This would make most other loops in the code "safe".
==============
EUREKA : I have just had some insight about the gtk loop!
I think this is what is happening (Working in my head, not with the
code, so details may be slightly off):
I always selected four songs to delete, since this created very reproducable behavior.
Before the patch:
Note: i is an iterator over the vector of selections. (*i) should never be NULL
* User selects the songs (vector holds ABCD) and clicks remove in the menu.
* In code, get vector0]=song A and pop up confirm dialog (synchronously, of course)
* User is in main loop, and clicks Yes to delete song A
* Back in code, erase song A from catalog and post the info event
* In code, i++, Get vector[1]=song B (still correct song name), pop up dialog
* User is in main loop, which causes info event to process. The main window is redrawn
without song A
and so the selection is updated so the vector is now BCD
* User clicks Yes to delete song B
* Back in code, erase song B (by name, so it works) from catalog and post info event
* In code, i++, Get third song as vector[2]=D (which skips song C) and pop up dialog
* User is in main loop, the window redraws and the selection is
updated without song B so that vector is now CD.
* User clicks Yes to delete song D
* In code, erase song D (by name, so it works) from catalog and post info event
* In code, i++. Here everything starts to go wrong. The code would put up the
dialog a fourth time (so D would be removed from the visible selection so the
vector is no C). But when I said to delete it, it would segfault.
I traced this at one point to (*i) now being a null pointer and causing a segfault.
_______________________________________________
[EMAIL PROTECTED]
http://www.freeamp.org/mailman/listinfo/freeamp-dev
