remove race condition when editing tempo/meter information.

Lock was not held across a replace_{tempo,meter}() operation because of re-use
of {remove,add}_{tempo,meter}. Moved functional code into _locked variants so
that replace operation can hold lock across its entire active lifetime.

2 files changed, 144 insertions, 101 deletions

diff --git a/libs/ardour/ardour/tempo.h b/libs/ardour/ardour/tempo.h
index f2d314651d..6e6818bf19 100644
--- a/libs/ardour/ardour/tempo.h
+++ b/libs/ardour/ardour/tempo.h
@@ -364,6 +364,13 @@ class LIBARDOUR_API TempoMap : public PBD::StatefulDestructible
 	TempoSection&       first_tempo();
 	
 	void do_insert (MetricSection* section);
+
+	void add_tempo_locked (const Tempo&, Timecode::BBT_Time where, bool recompute);
+	void add_meter_locked (const Meter&, Timecode::BBT_Time where, bool recompute);
+
+	bool remove_tempo_locked (const TempoSection&);
+	bool remove_meter_locked (const MeterSection&);
+
 };
 
 }; /* namespace ARDOUR */
diff --git a/libs/ardour/tempo.cc b/libs/ardour/tempo.cc
index 2f29a0dabf..521b91147d 100644
--- a/libs/ardour/tempo.cc
+++ b/libs/ardour/tempo.cc
@@ -307,23 +307,11 @@ TempoMap::remove_tempo (const TempoSection& tempo, bool complete_operation)
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
-		Metrics::iterator i;
-
-		for (i = metrics.begin(); i != metrics.end(); ++i) {
-			if (dynamic_cast<TempoSection*> (*i) != 0) {
-				if (tempo.frame() == (*i)->frame()) {
-					if ((*i)->movable()) {
-						metrics.erase (i);
-						removed = true;
-						break;
-					}
-				}
+		if ((removed = remove_tempo_locked (tempo))) {
+			if (complete_operation) {
+				recompute_map (true);
 			}
 		}
-
-		if (removed && complete_operation) {
-			recompute_map (false);
-		}
 	}
 
 	if (removed && complete_operation) {
@@ -331,6 +319,25 @@ TempoMap::remove_tempo (const TempoSection& tempo, bool complete_operation)
 	}
 }
 
+bool
+TempoMap::remove_tempo_locked (const TempoSection& tempo)
+{
+	Metrics::iterator i;
+
+	for (i = metrics.begin(); i != metrics.end(); ++i) {
+		if (dynamic_cast<TempoSection*> (*i) != 0) {
+			if (tempo.frame() == (*i)->frame()) {
+				if ((*i)->movable()) {
+					metrics.erase (i);
+					return true;
+				}
+			}
+		}
+	}
+
+	return false;
+}	
+	
 void
 TempoMap::remove_meter (const MeterSection& tempo, bool complete_operation)
 {
@@ -338,23 +345,11 @@ TempoMap::remove_meter (const MeterSection& tempo, bool complete_operation)
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
-		Metrics::iterator i;
-
-		for (i = metrics.begin(); i != metrics.end(); ++i) {
-			if (dynamic_cast<MeterSection*> (*i) != 0) {
-				if (tempo.frame() == (*i)->frame()) {
-					if ((*i)->movable()) {
-						metrics.erase (i);
-						removed = true;
-						break;
-					}
-				}
+		if ((removed = remove_meter_locked (tempo))) {
+			if (complete_operation) {
+				recompute_map (true);
 			}
 		}
-
-		if (removed && complete_operation) {
-			recompute_map (true);
-		}
 	}
 
 	if (removed && complete_operation) {
@@ -362,6 +357,25 @@ TempoMap::remove_meter (const MeterSection& tempo, bool complete_operation)
 	}
 }
 
+bool
+TempoMap::remove_meter_locked (const MeterSection& tempo)
+{
+	Metrics::iterator i;
+	
+	for (i = metrics.begin(); i != metrics.end(); ++i) {
+		if (dynamic_cast<MeterSection*> (*i) != 0) {
+			if (tempo.frame() == (*i)->frame()) {
+				if ((*i)->movable()) {
+					metrics.erase (i);
+					return true;
+				}
+			}
+		}
+	}
+
+	return false;
+}
+
 void
 TempoMap::do_insert (MetricSection* section)
 {
@@ -476,17 +490,19 @@ TempoMap::do_insert (MetricSection* section)
 void
 TempoMap::replace_tempo (const TempoSection& ts, const Tempo& tempo, const BBT_Time& where)
 {
-	TempoSection& first (first_tempo());
-
-	if (ts.start() != first.start()) {
-		remove_tempo (ts, false);
-		add_tempo (tempo, where);
-	} else {
-		{
-			Glib::Threads::RWLock::WriterLock lm (lock);
-			/* cannot move the first tempo section */
-			*static_cast<Tempo*>(&first) = tempo;
-			recompute_map (false);
+	{
+		Glib::Threads::RWLock::WriterLock lm (lock);
+		TempoSection& first (first_tempo());
+		
+		if (ts.start() != first.start()) {
+			remove_tempo_locked (ts);
+			add_tempo_locked (tempo, where, true);
+		} else {
+			{
+				/* cannot move the first tempo section */
+				*static_cast<Tempo*>(&first) = tempo;
+				recompute_map (false);
+			}
 		}
 	}
 
@@ -498,62 +514,69 @@ TempoMap::add_tempo (const Tempo& tempo, BBT_Time where)
 {
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
+		add_tempo_locked (tempo, where, true);
+	}
 
-		/* new tempos always start on a beat */
-		where.ticks = 0;
 
-		TempoSection* ts = new TempoSection (where, tempo.beats_per_minute(), tempo.note_type());
-		
-		/* find the meter to use to set the bar offset of this
-		 * tempo section.
-		 */
+	PropertyChanged (PropertyChange ());
+}
 
-		const Meter* meter = &first_meter();
+void
+TempoMap::add_tempo_locked (const Tempo& tempo, BBT_Time where, bool recompute)
+{
+	/* new tempos always start on a beat */
+	where.ticks = 0;
+	
+	TempoSection* ts = new TempoSection (where, tempo.beats_per_minute(), tempo.note_type());
+	
+	/* find the meter to use to set the bar offset of this
+	 * tempo section.
+	 */
+	
+	const Meter* meter = &first_meter();
+	
+	/* as we start, we are *guaranteed* to have m.meter and m.tempo pointing
+	   at something, because we insert the default tempo and meter during
+	   TempoMap construction.
+	   
+	   now see if we can find better candidates.
+	*/
+	
+	for (Metrics::const_iterator i = metrics.begin(); i != metrics.end(); ++i) {
 		
-		/* as we start, we are *guaranteed* to have m.meter and m.tempo pointing
-		   at something, because we insert the default tempo and meter during
-		   TempoMap construction.
-		   
-		   now see if we can find better candidates.
-		*/
+		const MeterSection* m;
 		
-		for (Metrics::const_iterator i = metrics.begin(); i != metrics.end(); ++i) {
-			
-			const MeterSection* m;
-			
-			if (where < (*i)->start()) {
-				break;
-			}
-			
-			if ((m = dynamic_cast<const MeterSection*>(*i)) != 0) {
-				meter = m;
-			}
+		if (where < (*i)->start()) {
+			break;
 		}
-
-		ts->update_bar_offset_from_bbt (*meter);
-
-		/* and insert it */
 		
-		do_insert (ts);
+		if ((m = dynamic_cast<const MeterSection*>(*i)) != 0) {
+			meter = m;
+		}
+	}
+	
+	ts->update_bar_offset_from_bbt (*meter);
+	
+	/* and insert it */
+	
+	do_insert (ts);
 
+	if (recompute) {
 		recompute_map (false);
 	}
-
-
-	PropertyChanged (PropertyChange ());
-}
+}	
 
 void
 TempoMap::replace_meter (const MeterSection& ms, const Meter& meter, const BBT_Time& where)
 {
-	MeterSection& first (first_meter());
-
-	if (ms.start() != first.start()) {
-		remove_meter (ms, false);
-		add_meter (meter, where);
-	} else {
-		{
-			Glib::Threads::RWLock::WriterLock lm (lock);
+	{
+		Glib::Threads::RWLock::WriterLock lm (lock);
+		MeterSection& first (first_meter());
+		
+		if (ms.start() != first.start()) {
+			remove_meter_locked (ms);
+			add_meter_locked (meter, where, true);
+		} else {
 			/* cannot move the first meter section */
 			*static_cast<Meter*>(&first) = meter;
 			recompute_map (true);
@@ -568,24 +591,7 @@ TempoMap::add_meter (const Meter& meter, BBT_Time where)
 {
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
-
-		/* a new meter always starts a new bar on the first beat. so
-		   round the start time appropriately. remember that
-		   `where' is based on the existing tempo map, not
-		   the result after we insert the new meter.
-
-		*/
-
-		if (where.beats != 1) {
-			where.beats = 1;
-			where.bars++;
-		}
-
-		/* new meters *always* start on a beat. */
-		where.ticks = 0;
-		
-		do_insert (new MeterSection (where, meter.divisions_per_bar(), meter.note_divisor()));
-		recompute_map (true);
+		add_meter_locked (meter, where, true);
 	}
 
 	
@@ -599,6 +605,32 @@ TempoMap::add_meter (const Meter& meter, BBT_Time where)
 }
 
 void
+TempoMap::add_meter_locked (const Meter& meter, BBT_Time where, bool recompute)
+{
+	/* a new meter always starts a new bar on the first beat. so
+	   round the start time appropriately. remember that
+	   `where' is based on the existing tempo map, not
+	   the result after we insert the new meter.
+	   
+	*/
+	
+	if (where.beats != 1) {
+		where.beats = 1;
+		where.bars++;
+	}
+	
+	/* new meters *always* start on a beat. */
+	where.ticks = 0;
+	
+	do_insert (new MeterSection (where, meter.divisions_per_bar(), meter.note_divisor()));
+
+	if (recompute) {
+		recompute_map (true);
+	}
+		
+}
+
+void
 TempoMap::change_initial_tempo (double beats_per_minute, double note_type)
 {
 	Tempo newtempo (beats_per_minute, note_type);
@@ -687,6 +719,8 @@ TempoMap::first_meter ()
 {
 	MeterSection *m = 0;
 
+	/* CALLER MUST HOLD LOCK */
+
 	for (Metrics::iterator i = metrics.begin(); i != metrics.end(); ++i) {
 		if ((m = dynamic_cast<MeterSection *> (*i)) != 0) {
 			return *m;
@@ -703,6 +737,8 @@ TempoMap::first_tempo () const
 {
 	const TempoSection *t = 0;
 
+	/* CALLER MUST HOLD LOCK */
+	
 	for (Metrics::const_iterator i = metrics.begin(); i != metrics.end(); ++i) {
 		if ((t = dynamic_cast<const TempoSection *> (*i)) != 0) {
 			return *t;