From bc0fa4d689a4bbcc4afa8a86fff53567bac80a57 Mon Sep 17 00:00:00 2001
From: Robin Gareus <robin@gareus.org>
Date: Thu, 24 Nov 2016 09:02:47 +0100
Subject: [PATCH] Fix mysterious crashes such as #7049

Fixes an issue with corrupted std::lists<> due to concurrent writes
to the invalidation list which eventually resulted in
EventLoop::invalidate_request() not invalidating requests.
Concurrency sucks rocks hard.
---
 libs/pbd/pbd/abstract_ui.cc | 26 ++++++++++++++++++++++++++
 libs/pbd/pbd/abstract_ui.h  |  1 +
 2 files changed, 27 insertions(+)

diff --git a/libs/pbd/pbd/abstract_ui.cc b/libs/pbd/pbd/abstract_ui.cc
index 6f03f2554e..5b289c68ea 100644
--- a/libs/pbd/pbd/abstract_ui.cc
+++ b/libs/pbd/pbd/abstract_ui.cc
@@ -240,6 +240,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 					request_buffer_map_lock.lock ();
 					if (vec.buf[0]->invalidation) {
 						DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: removing invalidation record for that request\n", event_loop_name()));
+						Glib::Threads::Mutex::Lock lm (request_invalidation_lock);
 						vec.buf[0]->invalidation->requests.remove (vec.buf[0]);
 					} else {
 						DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1: no invalidation record for that request\n", event_loop_name()));
@@ -301,6 +302,7 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 		 */
 
 		if (req->invalidation) {
+			Glib::Threads::Mutex::Lock lm (request_invalidation_lock);
 			DEBUG_TRACE (PBD::DEBUG::AbstractUI, string_compose ("%1/%2 remove request from its invalidation list\n", event_loop_name(), pthread_name()));
 
 			/* after this call, if the object referenced by the
@@ -309,6 +311,17 @@ AbstractUI<RequestObject>::handle_ui_requests ()
 			 */
 
 			req->invalidation->requests.remove (req);
+
+			/* req->invalidation->requests is identical to
+			 * EventLoop::invalidate_request()'s ir->requests.
+			 *
+			 * However EventLoop::invalidate_request() cannot
+			 * run in parallel to this, because sigc slots are always
+			 * called in the same thread as signal emission (this event-loop).
+			 * So we do not need to expose "request_invalidation_lock",
+			 * and only provide a safeguard for modifications to the
+			 * container itself.
+			 */
 		}
 
 		/* at this point, an object involved in a functor could be
@@ -434,6 +447,19 @@ AbstractUI<RequestObject>::call_slot (InvalidationRecord* invalidation, const bo
 	req->invalidation = invalidation;
 
 	if (invalidation) {
+		/* We're about to modify a std::list<> container, this must not
+		 * happen concurrently with other container modifications
+		 * (invalidation->requests.remove in handle_ui_requests).
+		 *
+		 * Let's take a dedicated lock that is less contended than
+		 * request_buffer_map_lock.
+		 *
+		 * This works because object's d'tor disconnect signals
+		 * (member variable) before the sigc::trackable's destroy_notify
+		 * runs. So there is no race between signal-emissions/call_slot()
+		 * and adding new requests after the object has been invalidated.
+		 */
+		Glib::Threads::Mutex::Lock lm (request_invalidation_lock);
 		invalidation->requests.push_back (req);
 		invalidation->event_loop = this;
 	}
diff --git a/libs/pbd/pbd/abstract_ui.h b/libs/pbd/pbd/abstract_ui.h
index 78a337fc40..278f8a2603 100644
--- a/libs/pbd/pbd/abstract_ui.h
+++ b/libs/pbd/pbd/abstract_ui.h
@@ -63,6 +63,7 @@ class ABSTRACT_UI_API AbstractUI : public BaseUI
         Glib::Threads::Mutex& slot_invalidation_mutex() { return request_buffer_map_lock; }
 
 	Glib::Threads::Mutex request_buffer_map_lock;
+	Glib::Threads::Mutex request_invalidation_lock;
 
 	static void* request_buffer_factory (uint32_t num_requests);