[oilpan]: Make thread shutdown more robust.

Source/platform/heap/HeapTest.cpp

Index: Source/platform/heap/HeapTest.cpp
diff --git a/Source/platform/heap/HeapTest.cpp b/Source/platform/heap/HeapTest.cpp
index 44a4285eab9c520004971e10849a8e8a74fe7537..b48b479b469d432f29ea02331c59d4c769f9406f 100644
--- a/Source/platform/heap/HeapTest.cpp
+++ b/Source/platform/heap/HeapTest.cpp
@@ -1697,8 +1697,8 @@ TEST(HeapTest, TypedHeapSanity)
     // We use TraceCounter for allocating an object on the general heap.
     Persistent<TraceCounter> generalHeapObject = TraceCounter::create();
     Persistent<TestTypedHeapClass> typedHeapObject = TestTypedHeapClass::create();
-    EXPECT_NE(pageHeaderAddress(reinterpret_cast<Address>(generalHeapObject.get())),
-        pageHeaderAddress(reinterpret_cast<Address>(typedHeapObject.get())));
+    EXPECT_NE(pageHeaderFromObject(generalHeapObject.get()),
+        pageHeaderFromObject(typedHeapObject.get()));
 }
 
 TEST(HeapTest, NoAllocation)
@@ -3429,6 +3429,7 @@ TEST(HeapTest, PersistentHeapCollectionTypes)
     typedef PersistentHeapListHashSet<Member<IntWrapper> > PListSet;
     typedef PersistentHeapLinkedHashSet<Member<IntWrapper> > PLinkedSet;
     typedef PersistentHeapHashMap<Member<IntWrapper>, Member<IntWrapper> > PMap;
+    typedef PersistentHeapHashMap<WeakMember<IntWrapper>, Member<IntWrapper> > WeakPMap;
     typedef PersistentHeapDeque<Member<IntWrapper> > PDeque;
 
     clearOutOldGarbage(&initialHeapSize);
@@ -3439,6 +3440,7 @@ TEST(HeapTest, PersistentHeapCollectionTypes)
         PListSet pListSet;
         PLinkedSet pLinkedSet;
         PMap pMap;
+        WeakPMap wpMap;
 
         IntWrapper* one(IntWrapper::create(1));
         IntWrapper* two(IntWrapper::create(2));
@@ -3449,6 +3451,8 @@ TEST(HeapTest, PersistentHeapCollectionTypes)
         IntWrapper* seven(IntWrapper::create(7));
         IntWrapper* eight(IntWrapper::create(8));
         IntWrapper* nine(IntWrapper::create(9));
+        Persistent<IntWrapper> ten(IntWrapper::create(10));
+        IntWrapper* eleven(IntWrapper::create(11));
 
         pVec.append(one);
         pVec.append(two);
@@ -3466,6 +3470,7 @@ TEST(HeapTest, PersistentHeapCollectionTypes)
         pListSet.add(eight);
         pLinkedSet.add(nine);
         pMap.add(five, six);
+        wpMap.add(ten, eleven);
 
         // Collect |vec| and |one|.
         vec = 0;
@@ -3494,11 +3499,17 @@ TEST(HeapTest, PersistentHeapCollectionTypes)
 
         EXPECT_EQ(1u, pMap.size());
         EXPECT_EQ(six, pMap.get(five));
+
+        EXPECT_EQ(1u, wpMap.size());
+        EXPECT_EQ(eleven, wpMap.get(ten));
+        ten.clear();
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+        EXPECT_EQ(0u, wpMap.size());
     }
 
     // Collect previous roots.
     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
-    EXPECT_EQ(9, IntWrapper::s_destructorCalls);
+    EXPECT_EQ(11, IntWrapper::s_destructorCalls);
 }
 
 TEST(HeapTest, CollectionNesting)
@@ -4583,5 +4594,240 @@ TEST(HeapTest, IndirectStrongToWeak)
     EXPECT_EQ(0u, map->size());
 }
 
+class CrossThreadObject : public GarbageCollectedFinalized<CrossThreadObject> {
+public:
+    static CrossThreadObject* create(IntWrapper* workerObjectPointer)
+    {
+        return new CrossThreadObject(workerObjectPointer);
+    }
+
+    virtual ~CrossThreadObject()
+    {
+        ++s_destructorCalls;
+    }
+
+    static int s_destructorCalls;
+    void trace(Visitor* visitor) { visitor->trace(m_workerObject); }
+
+private:
+    CrossThreadObject(IntWrapper* workerObjectPointer) : m_workerObject(workerObjectPointer) { }
+
+private:
+    Member<IntWrapper> m_workerObject;
+};
+
+int CrossThreadObject::s_destructorCalls = 0;
+
+class CrossThreadPointerTester {
+public:
+    static void test()
+    {
+        CrossThreadObject::s_destructorCalls = 0;
+        IntWrapper::s_destructorCalls = 0;
+        createThread(&workerThreadMain, 0, "Worker Thread");
+
+        // Wait for the worker to run.
+        while (!s_workerRunning) {
+            yield();
+        }
+
+        uintptr_t stackPtrValue = 0;
+        {
+            // Create an object with a pointer to the other heap's IntWrapper.
+            Persistent<CrossThreadObject> cto = CrossThreadObject::create(const_cast<IntWrapper*>(s_workerObjectPointer));
+            s_workerObjectPointer = 0;
+
+            Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+
+            // Nothing should have been collected/destructed.
+            EXPECT_EQ(0, CrossThreadObject::s_destructorCalls);
+            EXPECT_EQ(0, IntWrapper::s_destructorCalls);
+
+            // Put cto into a stack value. This is used to check that a conservative
+            // GC succeeds even though we are tracing the other thread heap after
+            // shutting it down.
+            stackPtrValue = reinterpret_cast<uintptr_t>(cto.get());
+        }
+
+        // At this point it is "programatically" okay to shut down the worker thread
+        // since the cto object should be dead. However out stackPtrValue will cause a
+        // trace of the object when doing a conservative GC.
+        // The worker thread's thread local GC's should just add the worker thread's
+        // pages to the heap after finalizing IntWrapper.
+        s_workerFinish = true;
+
+        // Wait for the worker to finish.
+        while (s_workerRunning) {
+            // No need to enter a safepoint since the GC's done by the worker at detach
+            // are thread local and doesn't require other threads to enter a safepoint.
+            yield();
+        }
+        // After the worker thread has detached it should have finalized the
+        // IntWrapper object on its heaps. Since there has been no global GC
+        // the cto object should not have been finalized.
+        EXPECT_EQ(0, CrossThreadObject::s_destructorCalls);
+        EXPECT_EQ(1, IntWrapper::s_destructorCalls);
+
+        // Now do a conservative GC. The stackPtrValue should keep cto alive
+        // and will also cause the orphaned page of the other thread to be
+        // traced. At this point cto should still not be finalized.
+        Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+        EXPECT_EQ(0, CrossThreadObject::s_destructorCalls);
+        EXPECT_EQ(1, IntWrapper::s_destructorCalls);
+
+        // Set stackPtrValue to zero and see the cto object being collected in the
+        // conservative GC.
+        stackPtrValue = 0;
+        Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+        EXPECT_EQ(1, CrossThreadObject::s_destructorCalls);
+        EXPECT_EQ(1, IntWrapper::s_destructorCalls);
+    }
+
+private:
+    static void workerThreadMain(void* data)
+    {
+        ThreadState::attach();
+
+        {
+            // Create a worker object that is only kept alive by a cross thread
+            // pointer (from CrossThreadObject).
+            IntWrapper* workerObject = IntWrapper::create(42);
+            s_workerObjectPointer = workerObject;
+            s_workerRunning = true;
+        }
+
+        // Wait for main thread to signal the worker to finish.
+        while (!s_workerFinish) {
+            ThreadState::current()->safePoint(ThreadState::NoHeapPointersOnStack);
+            yield();
+        }
+
+        ThreadState::detach();
+        s_workerRunning = false;
+    }
+
+    static volatile IntWrapper* s_workerObjectPointer;
+    static volatile bool s_workerRunning;
+    static volatile bool s_workerFinish;
+};
+
+volatile bool CrossThreadPointerTester::s_workerRunning = false;
+volatile bool CrossThreadPointerTester::s_workerFinish = false;
+volatile IntWrapper* CrossThreadPointerTester::s_workerObjectPointer = 0;
+
+TEST(HeapTest, CrossThreadPointerToOrphanedPage)
+{
+    CrossThreadPointerTester::test();
+}
+
+class DeadBitTester {
+public:
+    static void test()
+    {
+        IntWrapper::s_destructorCalls = 0;
+        createThread(&workerThreadMain, 0, "Worker Thread");
+
+        // Wait for the worker to run.
+        while (!s_workerRunning) {
+            yield();
+        }
+
+        // Do a GC. This will not find the worker threads object since it
+        // is not referred from any of the threads. Even a conservative
+        // GC will not find it.
+        Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
+
+        // Since the worker thread is not sweeping the worker object should
+        // not have been finalized.
+        EXPECT_EQ(0, IntWrapper::s_destructorCalls);
+
+        // Put the worker thread's object address on the stack and do a
+        // conservative GC. This should find the worker object, but since
+        // it was dead in the previous GC it should not be traced in this
+        // GC.
+        uintptr_t stackPtrValue = s_workerObjectPointer;
+        s_workerObjectPointer = 0;
+        ASSERT_UNUSED(stackPtrValue, stackPtrValue);
+        Heap::collectGarbage(ThreadState::HeapPointersOnStack);
+
+        // Since the worker thread is not sweeping the worker object should
+        // not have been finalized.
+        EXPECT_EQ(0, IntWrapper::s_destructorCalls);
+
+        // Tell the worker thread to finish its busy loop, but not detach.
+        // This means there is no further GC, however the worker thread gets
+        // to sweep so the worker object should now be finalized since the
+        // above conservative GC should not have traced the object that was
+        // found dead in the first GC.
+        s_workerContinue = true;
+
+        // Wait for the worker to sweep.
+        while (!s_workerDidSweep) {
+            // No need to enter a safepoint since the worker thread is just sweeping
+            // its own heap.
+            yield();
+        }
+        EXPECT_EQ(1, IntWrapper::s_destructorCalls);
+
+        // Tell the worker to shut down.
+        s_workerFinish = true;
+    }
+
+private:
+    static void workerThreadMain(void* data)
+    {
+        ThreadState::attach();
+
+        {
+            // Create a worker object that is not kept alive except the
+            // main thread will keep it as an integer value on its stack.
+            IntWrapper* workerObject = IntWrapper::create(42);
+            s_workerObjectPointer = reinterpret_cast<uintptr_t>(workerObject);
+            s_workerRunning = true;
+        }
+
+        // Enter a safepoint, but don't exit until the main thread tells us it
+        // has done it works. This means this thread will not sweep its heap
+        // since the sweep is done when the safepoint scope is destructed.
+        {
+            ThreadState::SafePointScope scope(ThreadState::NoHeapPointersOnStack);
+            while (!s_workerContinue) {
+                yield();
+            }
+        }
+
+        // At this point the worker have swept its heaps. Tell the main thread its
+        // done.
+        s_workerDidSweep = true;
+
+        // Wait with detach until the main thread says so. This is not strictly
+        // necessary, but it means the worker thread will not do its thread local
+        // GCs just yet, making it easier to reason about that no new GC has occurred
+        // and the above sweep was the one finalizing the worker object.
+        while (!s_workerFinish) {
+            yield();
+        }
+
+        ThreadState::detach();
+        s_workerRunning = false;
+    }
+
+    static volatile bool s_workerRunning;

[Mads Ager (chromium), 2014/07/11 10:59:34]

I guess we should either use atomic ops here or have mutex protection when
updating? Since the tests are not performance critical I would just add mutex
protection for the access to these variables?

[wibling-chromium, 2014/07/11 13:06:54]

Done.

+    static volatile bool s_workerContinue;
+    static volatile bool s_workerDidSweep;
+    static volatile bool s_workerFinish;
+    static volatile uintptr_t s_workerObjectPointer;
+};
+
+volatile bool DeadBitTester::s_workerRunning = false;
+volatile bool DeadBitTester::s_workerContinue = false;
+volatile bool DeadBitTester::s_workerDidSweep = false;
+volatile bool DeadBitTester::s_workerFinish = false;
+volatile uintptr_t DeadBitTester::s_workerObjectPointer = 0;
+
+TEST(HeapTest, ObjectDeadBit)
+{
+    DeadBitTester::test();
+}
 
 } // WebCore namespace