Oilpan: Remove a vtable from the Persistent hierarhcy

Source/platform/heap/Handle.h

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 40 matching lines @@
 class PersistentNode {
 public:
     explicit PersistentNode(TraceCallback trace)
         : m_trace(trace)
     {
     }
 
     NO_LAZY_SWEEP_SANITIZE_ADDRESS
     bool isHeapObjectAlive() { return m_trace; }
 
-    virtual ~PersistentNode()
-    {
-        ASSERT(isHeapObjectAlive());
-        m_trace = nullptr;
-    }
-
     // This operator= is important. Without having the operator=, m_next and
     // m_prev are inproperly copied and it breaks the link list of the
     // persistent handles.
     inline PersistentNode& operator=(const PersistentNode& otherref) { return *this; }
 
 private:
     // Ideally the trace method should be virtual and automatically dispatch
     // to the most specific implementation. However having a virtual method
     // on PersistentNode leads to too eager template instantiation with MSVC
     // which leads to include cycles.
     // Instead we call the constructor with a TraceCallback which knows the
     // type of the most specific child and calls trace directly. See
     // TraceMethodDelegate in Visitor.h for how this is done.
     void tracePersistentNode(Visitor* visitor)
     {
         m_trace(visitor, this);
     }
 
+    ~PersistentNode()
+    {
+    }
+
     TraceCallback m_trace;
     PersistentNode* m_next;
     PersistentNode* m_prev;
 
     template<typename T> friend class CrossThreadPersistent;
     template<typename T> friend class Persistent;
     template<typename Collection> friend class PersistentHeapCollectionBase;
     friend class PersistentAnchor;
     friend class ThreadState;
 };
 
 // A dummy Persistent handle that ensures the list of persistents is never null.
 // This removes a test from a hot path.
 class PersistentAnchor : public PersistentNode {
 public:
     void tracePersistentNodes(Visitor* visitor)
     {
         for (PersistentNode* current = m_next; current != this; current = current->m_next)
             current->tracePersistentNode(visitor);
     }
 
     int numberOfPersistents()
     {
         int numberOfPersistents = 0;
         for (PersistentNode* current = m_next; current != this; current = current->m_next)
             ++numberOfPersistents;
         return numberOfPersistents;
     }
 
-    virtual ~PersistentAnchor()
+    ~PersistentAnchor()
     {
-        // FIXME: oilpan: Ideally we should have no left-over persistents at this point. However currently there is a
-        // large number of objects leaked when we tear down the main thread. Since some of these might contain a
-        // persistent or e.g. be RefCountedGarbageCollected we cannot guarantee there are no remaining Persistents at
-        // this point.

[haraken, 2015/06/09 08:58:42]

Removed this FIXME because it is not realistic to remove all Persistent handles
from the main thread.

+        m_trace = nullptr;
     }
 
     template<typename VisitorDispatcher>
     void trace(VisitorDispatcher visitor)
     {
         ASSERT_NOT_REACHED();
     }
 
 private:
     PersistentAnchor() : PersistentNode(TraceMethodDelegate<PersistentAnchor, &PersistentAnchor::trace>::trampoline)
@@ skipping 71 matching lines @@
     template<typename U>
     Persistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
         checkPointer();
         recordBacktrace();
     }
 
     void clear() { m_raw = nullptr; }
 
-    virtual ~Persistent()
+    ~Persistent()
     {
         uninitialize();
         m_raw = nullptr;
+        m_trace = nullptr;
     }
 
     template<typename VisitorDispatcher>
     void trace(VisitorDispatcher visitor)
     {
         static_assert(sizeof(T), "T must be fully defined");
         static_assert(IsGarbageCollectedType<T>::value, "T needs to be a garbage collected object");
 #if ENABLE(GC_PROFILING)
         visitor->setHostInfo(this, m_tracingName.isEmpty() ? "Persistent" : m_tracingName);
 #endif
@@ skipping 176 matching lines @@
     template<typename U>
     CrossThreadPersistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
         checkPointer();
         recordBacktrace();
     }
 
     void clear() { m_raw = nullptr; }
 
-    virtual ~CrossThreadPersistent()
+    ~CrossThreadPersistent()
     {
         uninitialize();
         m_raw = nullptr;
+        m_trace = nullptr;
     }
 
     template<typename VisitorDispatcher>
     void trace(VisitorDispatcher visitor)
     {
         static_assert(sizeof(T), "T must be fully defined");
         static_assert(IsGarbageCollectedType<T>::value, "T needs to be a garbage collected object");
 #if ENABLE(GC_PROFILING)
         visitor->setHostInfo(this, m_tracingName.isEmpty() ? "CrossThreadPersistent" : m_tracingName);
 #endif
@@ skipping 113 matching lines @@
             m_tracingName = Heap::createBacktraceString();
     }
 
     String m_tracingName;
 #else
     inline void recordBacktrace() const { }
 #endif
     T* m_raw;
 };
 
+// PersistentNode must be the left-most class to let the
+// visitor->trace(static_cast<Collection*>(this)) trace the correct position.

[haraken, 2015/06/09 08:58:42]

It took me a couple of hours to notice this... Before this CL, this was not a
problem (by accident?) thanks to the existence of the vtable in PersistentNode.

 // FIXME: derive affinity based on the collection.
 template<typename Collection>
-class PersistentHeapCollectionBase : public Collection, public PersistentNode {
+class PersistentHeapCollectionBase : public PersistentNode, public Collection {
     // We overload the various new and delete operators with using the WTF DefaultAllocator to ensure persistent
     // heap collections are always allocated off-heap. This allows persistent collections to be used in
     // DEFINE_STATIC_LOCAL et. al.
     WTF_USE_ALLOCATOR(PersistentHeapCollectionBase, WTF::DefaultAllocator);
 public:
     PersistentHeapCollectionBase() : PersistentNode(TraceMethodDelegate<PersistentHeapCollectionBase<Collection>, &PersistentHeapCollectionBase<Collection>::trace>::trampoline)
     {
         initialize();
     }
 
-    PersistentHeapCollectionBase(const PersistentHeapCollectionBase& other) : Collection(other), PersistentNode(TraceMethodDelegate<PersistentHeapCollectionBase<Collection>, &PersistentHeapCollectionBase<Collection>::trace>::trampoline)
+    PersistentHeapCollectionBase(const PersistentHeapCollectionBase& other) : PersistentNode(TraceMethodDelegate<PersistentHeapCollectionBase<Collection>, &PersistentHeapCollectionBase<Collection>::trace>::trampoline), Collection(other)
     {
         initialize();
     }
 
     template<typename OtherCollection>
-    PersistentHeapCollectionBase(const OtherCollection& other) : Collection(other), PersistentNode(TraceMethodDelegate<PersistentHeapCollectionBase<Collection>, &PersistentHeapCollectionBase<Collection>::trace>::trampoline)
+    PersistentHeapCollectionBase(const OtherCollection& other) : PersistentNode(TraceMethodDelegate<PersistentHeapCollectionBase<Collection>, &PersistentHeapCollectionBase<Collection>::trace>::trampoline), Collection(other)
     {
         initialize();
     }
 
     ~PersistentHeapCollectionBase()
     {
         uninitialize();
+        m_trace = nullptr;
     }
 
     template<typename VisitorDispatcher>
     void trace(VisitorDispatcher visitor)
     {
         static_assert(sizeof(Collection), "Collection must be fully defined");
 #if ENABLE(GC_PROFILING)
         visitor->setHostInfo(this, "PersistentHeapCollectionBase");
 #endif
         visitor->trace(*static_cast<Collection*>(this));
@@ skipping 705 matching lines @@
 struct ParamStorageTraits<RawPtr<T>> : public PointerParamStorageTraits<T*, blink::IsGarbageCollectedType<T>::value> {
     static_assert(sizeof(T), "T must be fully defined");
 };
 
 template<typename T>
 PassRefPtr<T> adoptRef(blink::RefCountedGarbageCollected<T>*) = delete;
 
 } // namespace WTF
 
 #endif