From 2c8343aa8233333dae0caf91f25bf1fe4689c88e Mon Sep 17 00:00:00 2001
From: John Firebaugh <john.firebaugh@gmail.com>
Date: Mon, 22 May 2017 16:05:58 -0700
Subject: [core] Block in Mailbox::close() until neither receive nor push are
 in progress
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Otherwise, an ActorRef that's in the process of sending a message could attempt to access an invalid Scheduler reference:

      Thread 1                     Thread 2
--------------------------------------------------
 Scheduler::Scheduler
     Actor::Actor
                              weakMailbox.lock()
     Actor::~Actor
 Scheduler::~Scheduler
                               mailbox->push()
                             scheduler.schedule() 💣
---
 src/mbgl/actor/mailbox.cpp | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

(limited to 'src')

diff --git a/src/mbgl/actor/mailbox.cpp b/src/mbgl/actor/mailbox.cpp
index 5f60629833..947f6f9028 100644
--- a/src/mbgl/actor/mailbox.cpp
+++ b/src/mbgl/actor/mailbox.cpp
@@ -10,8 +10,23 @@ Mailbox::Mailbox(Scheduler& scheduler_)
     : scheduler(scheduler_) {
 }
 
+void Mailbox::close() {
+    // Block until neither receive() nor push() are in progress. Two mutexes are used because receive()
+    // must not block send(). Of the two, the receiving mutex must be acquired first, because that is
+    // the order that an actor will obtain them when it self-sends a message, and consistent lock
+    // acquisition order prevents deadlocks.
+    std::lock_guard<std::mutex> receivingLock(receivingMutex);
+    std::lock_guard<std::mutex> pushingLock(pushingMutex);
+
+    closed = true;
+}
+
 void Mailbox::push(std::unique_ptr<Message> message) {
-    assert(!closing);
+    std::lock_guard<std::mutex> pushingLock(pushingMutex);
+
+    if (closed) {
+        return;
+    }
 
     std::lock_guard<std::mutex> queueLock(queueMutex);
     bool wasEmpty = queue.empty();
@@ -21,16 +36,10 @@ void Mailbox::push(std::unique_ptr<Message> message) {
     }
 }
 
-void Mailbox::close() {
-    // Block until the scheduler is guaranteed not to be executing receive().
-    std::lock_guard<std::mutex> closingLock(closingMutex);
-    closing = true;
-}
-
 void Mailbox::receive() {
-    std::lock_guard<std::mutex> closingLock(closingMutex);
+    std::lock_guard<std::mutex> receivingLock(receivingMutex);
 
-    if (closing) {
+    if (closed) {
         return;
     }
 
-- 
cgit v1.2.1