Commit 7ad6e330 authored by Andrés G. Aragoneses's avatar Andrés G. Aragoneses

SourceMessage: fix race in access to actions collection (bgo#751582)

This foreach() loop had the potential problem of a typical race
throwing a 'System.InvalidOperationException: Collection was modified;
enumeration operation may not execute.'

The normal thing to do in these cases is just to make a copy of the
collection (via the .ToArray() method, for instance), at the
beginning of the loop. However, this approach alone:

a) would have presented a new race between the check for null
and the call to the ToArray method.
b) wouldn't have worked at compile time because IEnumerable<X>
doesn't have this method (List<X> does).

The proper way to fix (a) involves also avoiding to have
a null value. How? Initializing the field always at
construction time of the SourceMessage class.

This has the good side-effect of being able to remove all
null-checks around this field and its wrapper property.

WRT (b) we could have created a new list at the beginning
of the foreach loop, but it's more practical to just do the
copy in the SourceMessage class, because, in the end, a read
access should also involve lock()ing, otherwise writers to
the collection could race with the readers of it.
parent 5726cda3
......@@ -36,7 +36,7 @@ namespace Banshee.Sources
private bool updated_when_frozen;
private int freeze_count;
private List<MessageAction> actions;
private List<MessageAction> actions = new List<MessageAction> ();
private Source source;
private bool is_spinning;
......@@ -55,9 +55,6 @@ namespace Banshee.Sources
public void AddAction (MessageAction action)
{
lock (this) {
if (actions == null) {
actions = new List<MessageAction> ();
}
actions.Add (action);
OnUpdated ();
}
......@@ -66,9 +63,7 @@ namespace Banshee.Sources
public void ClearActions ()
{
lock (this) {
if (actions != null) {
actions.Clear ();
}
actions.Clear ();
OnUpdated ();
}
}
......@@ -143,7 +138,11 @@ namespace Banshee.Sources
}
public IEnumerable<MessageAction> Actions {
get { return actions; }
get {
lock (this) {
return actions.ToArray ();
}
}
}
}
}
......@@ -107,12 +107,10 @@ namespace Banshee.Gui.Widgets
ClearButtons ();
if (source.CurrentMessage.Actions != null) {
foreach (MessageAction action in source.CurrentMessage.Actions) {
Button button = new ActionButton (action);
button.UseStock = action.IsStock;
AddButton (button);
}
foreach (MessageAction action in source.CurrentMessage.Actions) {
Button button = new ActionButton (action);
button.UseStock = action.IsStock;
AddButton (button);
}
Show ();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment