Crash due to abort() calling libc++.1.dylib: std::__1::thread::detach()
authorggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Jul 2016 00:16:19 +0000 (00:16 +0000)
committerggaren@apple.com <ggaren@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 12 Jul 2016 00:16:19 +0000 (00:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159655

Reviewed by Sam Weinig.

It's not entirely clear what was happening in these crashes, but our
use of detach() was 100% forward-looking, so we can just remove it for
now.

This patch removes the ability for the scavenger owner to die before
the scavenger thread dies (which was unused) and also removes the
ability for the scavenger thread to exit (which was used, but we
messed up and did thread joining lazily, so we never got any benefit
from thread exit.)

We can add these features back when we need them, and make them work then.

* bmalloc/AsyncTask.h:
(bmalloc::Function>::AsyncTask): We start out in the running state now
because we know that starting our thread will run it.

(bmalloc::Function>::~AsyncTask): We don't support destruction anymore.

(bmalloc::Function>::runSlowCase): I removed the Exited state.

(bmalloc::Function>::threadRunLoop): I removed the Exited and
ExitRequested states.

* bmalloc/Heap.h:

* bmalloc/VMHeap.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@203100 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/bmalloc/ChangeLog
Source/bmalloc/bmalloc/AsyncTask.h
Source/bmalloc/bmalloc/Heap.h
Source/bmalloc/bmalloc/VMHeap.h

index e9244a6..1da64a7 100644 (file)
@@ -1,3 +1,37 @@
+2016-07-11  Geoffrey Garen  <ggaren@apple.com>
+
+        Crash due to abort() calling libc++.1.dylib: std::__1::thread::detach()
+        https://bugs.webkit.org/show_bug.cgi?id=159655
+
+        Reviewed by Sam Weinig.
+
+        It's not entirely clear what was happening in these crashes, but our
+        use of detach() was 100% forward-looking, so we can just remove it for
+        now.
+
+        This patch removes the ability for the scavenger owner to die before
+        the scavenger thread dies (which was unused) and also removes the
+        ability for the scavenger thread to exit (which was used, but we
+        messed up and did thread joining lazily, so we never got any benefit
+        from thread exit.)
+
+        We can add these features back when we need them, and make them work then.
+
+        * bmalloc/AsyncTask.h:
+        (bmalloc::Function>::AsyncTask): We start out in the running state now
+        because we know that starting our thread will run it.
+
+        (bmalloc::Function>::~AsyncTask): We don't support destruction anymore.
+
+        (bmalloc::Function>::runSlowCase): I removed the Exited state.
+
+        (bmalloc::Function>::threadRunLoop): I removed the Exited and
+        ExitRequested states.
+
+        * bmalloc/Heap.h:
+
+        * bmalloc/VMHeap.h:
+
 2016-06-12  David Kilzer  <ddkilzer@apple.com>
 
         Crash in com.apple.WebKit.WebContent at std::__1::__call_once_proxy<std::__1::tuple<CrashReporterSupportLibrary()::$_0&&> >
index f0bae6d..6708402 100644 (file)
@@ -44,9 +44,7 @@ public:
     void run();
 
 private:
-    enum State { Exited, ExitRequested, Sleeping, Running, RunRequested };
-
-    static const constexpr std::chrono::seconds exitDelay = std::chrono::seconds(1);
+    enum State { Sleeping, Running, RunRequested };
 
     void runSlowCase();
 
@@ -64,13 +62,11 @@ private:
     Function m_function;
 };
 
-template<typename Object, typename Function> const constexpr std::chrono::seconds AsyncTask<Object, Function>::exitDelay;
-
 template<typename Object, typename Function>
 AsyncTask<Object, Function>::AsyncTask(Object& object, const Function& function)
-    : m_state(Exited)
+    : m_state(Running)
     , m_condition()
-    , m_thread()
+    , m_thread(std::thread(&AsyncTask::threadEntryPoint, this))
     , m_object(object)
     , m_function(function)
 {
@@ -79,19 +75,9 @@ AsyncTask<Object, Function>::AsyncTask(Object& object, const Function& function)
 template<typename Object, typename Function>
 AsyncTask<Object, Function>::~AsyncTask()
 {
-    // Prevent our thread from entering the running or sleeping state.
-    State oldState = m_state.exchange(ExitRequested);
-
-    // Wake our thread if it was already in the sleeping state.
-    if (oldState == Sleeping) {
-        std::lock_guard<Mutex> lock(m_conditionMutex);
-        m_condition.notify_all();
-    }
-
-    // Wait for our thread to exit because it uses our data members (and it may
-    // use m_object's data members).
-    if (m_thread.joinable())
-        m_thread.join();
+    // We'd like to mark our destructor deleted but C++ won't allow it because
+    // we are an automatic member of Heap.
+    RELEASE_BASSERT(0);
 }
 
 template<typename Object, typename Function>
@@ -109,16 +95,9 @@ NO_INLINE void AsyncTask<Object, Function>::runSlowCase()
     if (oldState == RunRequested || oldState == Running)
         return;
 
-    if (oldState == Sleeping) {
-        std::lock_guard<Mutex> lock(m_conditionMutex);
-        m_condition.notify_all();
-        return;
-    }
-
-    BASSERT(oldState == Exited);
-    if (m_thread.joinable())
-        m_thread.detach();
-    m_thread = std::thread(&AsyncTask::threadEntryPoint, this);
+    BASSERT(oldState == Sleeping);
+    std::lock_guard<Mutex> lock(m_conditionMutex);
+    m_condition.notify_all();
 }
 
 template<typename Object, typename Function>
@@ -130,10 +109,9 @@ void AsyncTask<Object, Function>::threadEntryPoint(AsyncTask* asyncTask)
 template<typename Object, typename Function>
 void AsyncTask<Object, Function>::threadRunLoop()
 {
-    // This loop ratchets downward from most active to least active state, and
-    // finally exits. While we ratchet downward, any other thread may reset our
-    // state to RunRequested or ExitRequested.
-    
+    // This loop ratchets downward from most active to least active state. While
+    // we ratchet downward, any other thread may reset our state.
+
     // We require any state change while we are sleeping to signal to our
     // condition variable and wake us up.
 
@@ -145,16 +123,8 @@ void AsyncTask<Object, Function>::threadRunLoop()
         expectedState = Running;
         if (m_state.compare_exchange_weak(expectedState, Sleeping)) {
             std::unique_lock<Mutex> lock(m_conditionMutex);
-            m_condition.wait_for(lock, exitDelay, [=]() { return this->m_state != Sleeping; });
+            m_condition.wait(lock, [&]() { return m_state != Sleeping; });
         }
-
-        expectedState = Sleeping;
-        if (m_state.compare_exchange_weak(expectedState, Exited))
-            return;
-        
-        expectedState = ExitRequested;
-        if (m_state.compare_exchange_weak(expectedState, Exited))
-            return;
     }
 }
 
index d863e38..9c2da24 100644 (file)
@@ -26,6 +26,7 @@
 #ifndef Heap_h
 #define Heap_h
 
+#include "AsyncTask.h"
 #include "BumpRange.h"
 #include "Environment.h"
 #include "LineMetadata.h"
index d91d8a6..97226cf 100644 (file)
@@ -26,7 +26,6 @@
 #ifndef VMHeap_h
 #define VMHeap_h
 
-#include "AsyncTask.h"
 #include "Chunk.h"
 #include "FixedVector.h"
 #include "Map.h"