Oilpan: disable conservative GCs during initial GC mixin construction.

Source/platform/heap/Visitor.h

Index: Source/platform/heap/Visitor.h
diff --git a/Source/platform/heap/Visitor.h b/Source/platform/heap/Visitor.h
index adbdbc564a49569d2e1b0cec208cfbcfef12c3a7..b7c4ce7d337d1c4433b331f7a550bfff60eb0fd1 100644
--- a/Source/platform/heap/Visitor.h
+++ b/Source/platform/heap/Visitor.h
@@ -897,19 +897,117 @@ public:
         return visitor->isAlive(this);                                                                                                  \
     }                                                                                                                                   \
 private:
+
+// A C++ object's vptr will be initialized to its leftmost base's
+// vtable after the constructors of all its subclasses have run,
+// so if a subclass constructor tries to access any of the vtbl
+// entries of its leftmost base prematurely, it'll find an as-yet
+// incorrect vptr and fail. Which is exactly what a garbage collector
+// will try to do if it tries to access the leftmost base while one
+// of the subclass constructors of a GC mixin object triggers a GC.
+// It is consequently not safe to allow any GCs while these objects
+// are under (sub constructor) construction.
+//
+// To achieve that, the construction of mixins are handled in a
+// special manner:
+//
+//  - The initial allocation of the mixin object will enter a no GC scope.
+//    This is done by overriding 'operator new' for mixin instances.
+//  - When the constructor for the mixin is invoked, after all the
+//    derived constructors have run, it will invoke the constructor
+//    for a field whose only purpose is to leave the GC scope.
+//    GarbageCollectedMixinConstructorMarker's constructor takes care of
+//    this and the field is declared by way of USING_GARBAGE_COLLECTED_MIXIN().
+
+// Trait template to resolve the effective base GC class to use when allocating
+// objects at some type T. Requires specialization for Node and CSSValue
+// derived types to have these be allocated on appropriate heaps.
+template<typename T, typename Enabled = void>
+class EffectiveGCBaseTrait {
+public:
+    using Type = T;
+};
+
+// FIXME: move these forward declarations and the
+// EffectiveGCBaseTrait<> specializations out of
+// here and into the header files for the respective types.
+class Node;
+class CSSValue;
+
+template<typename T>
+class EffectiveGCBaseTrait<T, typename WTF::EnableIf<WTF::IsSubclass<T, blink::Node>::value>::Type> {
+public:
+    using Type = Node;
+};
+
+template<typename T>
+class EffectiveGCBaseTrait<T, typename WTF::EnableIf<WTF::IsSubclass<T, blink::CSSValue>::value>::Type> {
+public:
+    using Type = CSSValue;
+};
+
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)                         \
+public:                                                                                 \
+    GC_PLUGIN_IGNORE("crbug.com/456823")                                                \
+    void* operator new(size_t size)                                                     \

[haraken, 2015/03/06 13:01:01]

As far as I grep in the code base, GARBAGE_COLLECTED_MIXIN is not used in
performance-sensitive classes. So the overhead in operator new would be OK, I
hope.

+    {                                                                                   \
+        void* object = Heap::allocate<typename EffectiveGCBaseTrait<TYPE>::Type>(size); \
+        ThreadState* state = ThreadStateFor<ThreadingTrait<TYPE>::Affinity>::state();   \
+        state->enterGCForbiddenScope(TYPE::mixinLevels);                                \
+        return object;                                                                  \
+    }                                                                                   \
+private:                                                                                \
+    GarbageCollectedMixinConstructorMarker<TYPE> m_mixinConstructorMarker;
+
+#if ENABLE(ASSERT)
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NONNESTED_DEBUG()     \
+protected:                                                   \
+    virtual void mixinsCannotBeImplicitlyNested() final { }
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED_DEBUG(TYPE, SUBTYPE)   \
+protected:                                                           \
+    static void mixinsNestingOthersMustBeDeclaredAsSuch()            \
+    {                                                                \
+        static_assert(WTF::IsSubclass<blink::TYPE, blink::SUBTYPE>::value, "Mixin class does not derive from its nested mixin class; use USING_GARBAGE_COLLECTED_MIXIN_NESTED()."); \
+    }
+#else
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NONNESTED_DEBUG()
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED_DEBUG(TYPE, SUBTYPE)
+#endif
+
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NONNESTED()           \
+protected:                                                   \
+    static const unsigned mixinLevels = 1;
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, SUBTYPE)      \

[haraken, 2015/03/06 13:01:01]

Just to confirm: This mechanism can detect the following two kinds of errors,
right?

- Forgot to add NESTED.
- Added NESTED by mistake.

[sof, 2015/03/06 16:11:06]

The final mixinsCannotBeImplicitlyNested() empty method catches this.
e.g., USING_GARBAGE_COLLECTED_MIXIN_NESTED(A, B);

If B isn't a GC mixin, compilation would fail. If it is, but A does not derive
from it, compilation would fail. If both those properties hold, then A is indeed
a mixin that nests B & this declaration would be the correct thing to supply.

+protected:                                                        \
+    static const unsigned mixinLevels = SUBTYPE::mixinLevels + 1;
+
 #if ENABLE(INLINED_TRACE)
-#define USING_GARBAGE_COLLECTED_MIXIN(TYPE)                       \
+#define USING_GARBAGE_COLLECTED_MIXIN_BASE(TYPE)                  \
     DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::Visitor*, TYPE) \
-    DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::InlinedGlobalMarkingVisitor, TYPE)
+    DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::InlinedGlobalMarkingVisitor, TYPE) \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)
 #else
-#define USING_GARBAGE_COLLECTED_MIXIN(TYPE) \
-    DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::Visitor*, TYPE)
+#define USING_GARBAGE_COLLECTED_MIXIN_BASE(TYPE)                  \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(blink::Visitor*, TYPE) \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)
 #endif
 
+#define USING_GARBAGE_COLLECTED_MIXIN(TYPE)          \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_NONNESTED_DEBUG() \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_NONNESTED()       \
+    USING_GARBAGE_COLLECTED_MIXIN_BASE(TYPE)
+
+#define USING_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, SUBTYPE)    \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED_DEBUG(TYPE, SUBTYPE) \
+    DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, SUBTYPE)       \
+    USING_GARBAGE_COLLECTED_MIXIN_BASE(TYPE)
+
 #if ENABLE(OILPAN)
 #define WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(TYPE) USING_GARBAGE_COLLECTED_MIXIN(TYPE)
+#define WILL_BE_USING_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, NESTEDMIXIN) USING_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, NESTEDMIXIN)
 #else
 #define WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(TYPE)
+#define WILL_BE_USING_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, NESTEDMIXIN)
 #endif
 
 // Template to determine if a class is a GarbageCollectedMixin by checking if it
@@ -929,6 +1027,28 @@ public:
     static const bool value = sizeof(checkMarker<T>(nullptr)) == sizeof(YesType);
 };
 
+// An empty class with a constructor that's arranged invoked when all derived constructors
+// of a mixin instance have completed and it is safe to allow GCs again. See
+// AllocateObjectTrait<> comment for more.
+//
+// USING_GARBAGE_COLLECTED_MIXIN() declares a GarbageCollectedMixinConstructorMarker<> private
+// field. By following Blink convention of using the macro at the top of a class declaration,
+// its constructor will run first.
+template<typename T>
+class GarbageCollectedMixinConstructorMarker {
+public:
+    GarbageCollectedMixinConstructorMarker()
+    {
+        // FIXME: if prompt conservative GCs are needed, forced GCs that
+        // were denied while within this scope, could now be performed.
+        // For now, assume the next out-of-line allocation request will
+        // happen soon enough and take care of it. Mixin objects aren't
+        // overly common.
+        ThreadState* state = ThreadStateFor<ThreadingTrait<T>::Affinity>::state();
+        state->leaveGCForbiddenScope();
+    }
+};
+
 #if ENABLE(GC_PROFILING)
 template<typename T>
 struct TypenameStringTrait {