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.

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 */

libs/ardour/tempo.cc

@@ -307,6 +307,21 @@ TempoMap::remove_tempo (const TempoSection& tempo, bool complete_operation)
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
+		if ((removed = remove_tempo_locked (tempo))) {
+			if (complete_operation) {
+				recompute_map (true);
+			}
+		}
+	}
+
+	if (removed && complete_operation) {
+		PropertyChanged (PropertyChange ());
+	}
+}
+
+bool
+TempoMap::remove_tempo_locked (const TempoSection& tempo)
+{
 	Metrics::iterator i;
 
 	for (i = metrics.begin(); i != metrics.end(); ++i) {
@@ -314,21 +329,13 @@ TempoMap::remove_tempo (const TempoSection& tempo, bool complete_operation)
 			if (tempo.frame() == (*i)->frame()) {
 				if ((*i)->movable()) {
 					metrics.erase (i);
-						removed = true;
+					return true;
-						break;
 				}
 			}
 		}
 	}
 
-		if (removed && complete_operation) {
+	return false;
-			recompute_map (false);
-		}
-	}
-
-	if (removed && complete_operation) {
-		PropertyChanged (PropertyChange ());
-	}
 }	
 	
 void
@@ -338,6 +345,21 @@ TempoMap::remove_meter (const MeterSection& tempo, bool complete_operation)
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
+		if ((removed = remove_meter_locked (tempo))) {
+			if (complete_operation) {
+				recompute_map (true);
+			}
+		}
+	}
+
+	if (removed && complete_operation) {
+		PropertyChanged (PropertyChange ());
+	}
+}
+
+bool
+TempoMap::remove_meter_locked (const MeterSection& tempo)
+{
 	Metrics::iterator i;
 	
 	for (i = metrics.begin(); i != metrics.end(); ++i) {
@@ -345,21 +367,13 @@ TempoMap::remove_meter (const MeterSection& tempo, bool complete_operation)
 			if (tempo.frame() == (*i)->frame()) {
 				if ((*i)->movable()) {
 					metrics.erase (i);
-						removed = true;
+					return true;
-						break;
 				}
 			}
 		}
 	}
 
-		if (removed && complete_operation) {
+	return false;
-			recompute_map (true);
-		}
-	}
-
-	if (removed && complete_operation) {
-		PropertyChanged (PropertyChange ());
-	}
 }
 
 void
@@ -476,19 +490,21 @@ TempoMap::do_insert (MetricSection* section)
 void
 TempoMap::replace_tempo (const TempoSection& ts, const Tempo& tempo, const BBT_Time& where)
 {
+	{
+		Glib::Threads::RWLock::WriterLock lm (lock);
 		TempoSection& first (first_tempo());
 		
 		if (ts.start() != first.start()) {
-		remove_tempo (ts, false);
+			remove_tempo_locked (ts);
-		add_tempo (tempo, where);
+			add_tempo_locked (tempo, where, true);
 		} else {
 			{
-			Glib::Threads::RWLock::WriterLock lm (lock);
 				/* cannot move the first tempo section */
 				*static_cast<Tempo*>(&first) = tempo;
 				recompute_map (false);
 			}
 		}
+	}
 
 	PropertyChanged (PropertyChange ());
 }
@@ -498,7 +514,16 @@ TempoMap::add_tempo (const Tempo& tempo, BBT_Time where)
 {
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
+		add_tempo_locked (tempo, where, true);
+	}
 
+
+	PropertyChanged (PropertyChange ());
+}
+
+void
+TempoMap::add_tempo_locked (const Tempo& tempo, BBT_Time where, bool recompute)
+{
 	/* new tempos always start on a beat */
 	where.ticks = 0;
 	
@@ -536,24 +561,22 @@ TempoMap::add_tempo (const Tempo& tempo, BBT_Time where)
 	
 	do_insert (ts);
 
+	if (recompute) {
 		recompute_map (false);
 	}
-
-
-	PropertyChanged (PropertyChange ());
 }	
 
 void
 TempoMap::replace_meter (const MeterSection& ms, const Meter& meter, const BBT_Time& where)
 {
+	{
+		Glib::Threads::RWLock::WriterLock lm (lock);
 		MeterSection& first (first_meter());
 		
 		if (ms.start() != first.start()) {
-		remove_meter (ms, false);
+			remove_meter_locked (ms);
-		add_meter (meter, where);
+			add_meter_locked (meter, where, true);
 		} else {
-		{
-			Glib::Threads::RWLock::WriterLock lm (lock);
 			/* cannot move the first meter section */
 			*static_cast<Meter*>(&first) = meter;
 			recompute_map (true);
@@ -568,7 +591,22 @@ TempoMap::add_meter (const Meter& meter, BBT_Time where)
 {
 	{
 		Glib::Threads::RWLock::WriterLock lm (lock);
+		add_meter_locked (meter, where, true);
+	}
 
+	
+#ifndef NDEBUG
+	if (DEBUG_ENABLED(DEBUG::TempoMap)) {
+		dump (std::cerr);
+	}
+#endif
+
+	PropertyChanged (PropertyChange ());
+}
+
+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
@@ -585,17 +623,11 @@ TempoMap::add_meter (const Meter& meter, BBT_Time where)
 	where.ticks = 0;
 	
 	do_insert (new MeterSection (where, meter.divisions_per_bar(), meter.note_divisor()));
+
+	if (recompute) {
 		recompute_map (true);
 	}
 		
-	
-#ifndef NDEBUG
-	if (DEBUG_ENABLED(DEBUG::TempoMap)) {
-		dump (std::cerr);
-	}
-#endif
-
-	PropertyChanged (PropertyChange ());
 }
 
 void
@@ -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;