Reland "heap: Uniform object construction""

Switch to a uniform object construction that does not differentiate between
mixins and regular garabge collected objects and as a consequence can interrupt
both construction cases with garbage collections.

Object construction now uniformely works as follows:
1. The object under construction is marked so in the HeapObjectHeader.
2. Upon discovering such an object it is delayed in a separate marking worklist.
3. Upon hitting the main atomic pause all such objects are conservatively
   scanned for pointers without using the Trace method.

Special cases:
a. Mixins in construction: The HeapObjectHeader cannot retrived for such
   objects. A default GetTraceDescriptor() implementation returning a sentinel
   marker is used to discover that such objects should be delayed.
b. Upon reaching a safepoint (e.g. no stack), in-construction objects are moved
   to a marking worklist that, similar to the regular marking worklist, allows for
   incremental processing.

Effects:
- No more TLS access for no-GC scope on mixin construction.
- No more memory needed for the no-GC scope marker in mixin classes
- MakeGarbageCollected should require less binary size as implementations can be
  aligned.
- Incremental marking Start and Step operations are safe to be called with stack
  (even though it is preferable to not do so).
- Object construction is safe to be used with a concurrent marker as it is ok
  for any objects (mixins as well as normal ones) to be published to the object
  graph during constructors.

This reverts commit ad406608.

Bug: 911662
Change-Id: I7babc6bac25b446a63783bf2c3d19e9b525d9e9a
Reviewed-on: https://chromium-review.googlesource.com/c/1405991Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622098}

third_party/blink/renderer/core/dom/live_node_list_registry.cc

@@ -44,7 +44,7 @@ void LiveNodeListRegistry::RecomputeMask() {
 
 void LiveNodeListRegistry::ClearWeakMembers(Visitor*) {
   auto* it = std::remove_if(data_.begin(), data_.end(), [](Entry entry) {
-    return !ObjectAliveTrait<LiveNodeListBase>::IsHeapObjectAlive(entry.first);
+    return !ThreadHeap::IsHeapObjectAlive(entry.first);
   });
   if (it == data_.end())
     return;

third_party/blink/renderer/core/dom/shadow_root.cc

@@ -56,7 +56,6 @@ void ShadowRoot::Distribute() {
 }
 
 struct SameSizeAsShadowRoot : public DocumentFragment, public TreeScope {
-  char empty_class_fields_due_to_gc_mixin_marker[1];
   Member<void*> member[3];
   unsigned counters_and_flags[1];
 };

third_party/blink/renderer/modules/webmidi/midi_dispatcher.h

@@ -18,6 +18,7 @@ class MIDIDispatcher : public GarbageCollectedFinalized<MIDIDispatcher>,
                        public midi::mojom::blink::MidiSessionClient {
  public:
   static MIDIDispatcher& Instance();
+  MIDIDispatcher();
   ~MIDIDispatcher() override;
 
   void Trace(Visitor* visitor);
@@ -44,10 +45,6 @@ class MIDIDispatcher : public GarbageCollectedFinalized<MIDIDispatcher>,
                     base::TimeTicks timestamp) override;
 
  private:
-  friend class ConstructTrait<MIDIDispatcher>;
-
-  MIDIDispatcher();
-
   midi::mojom::blink::MidiSessionProvider& GetMidiSessionProvider();
   midi::mojom::blink::MidiSession& GetMidiSession();
 

third_party/blink/renderer/platform/heap/garbage_collected.h

@@ -47,12 +47,17 @@ struct IsGarbageCollectedMixin {
   static const bool value = sizeof(CheckMarker<T>(nullptr)) == sizeof(YesType);
 };
 
+// TraceDescriptor is used to describe how to trace an object.
 struct TraceDescriptor {
   STACK_ALLOCATED();
 
  public:
+  // The adjusted base pointer of the object that should be traced.
   void* base_object_payload;
+  // A callback for tracing the object.
   TraceCallback callback;
+  // Indicator whether this object can be traced recursively or whether it
+  // requires iterative tracing.
   bool can_trace_eagerly;
 };
 
@@ -72,11 +77,14 @@ struct TraceDescriptor {
 //   USING_GARBAGE_COLLECTED_MIXIN(A);
 // };
 //
-// With the helper, as long as we are using Member<B>, TypeTrait<B> will
-// dispatch dynamically to retrieve the necessary tracing and header methods.
-// Note that this is only enabled for Member<B>. For Member<A> which we can
-// compute the necessary methods and pointers statically and this dynamic
-// dispatch is not used.
+// The classes involved and the helper macros allow for properly handling
+// definitions for Member<B> and friends. The mechanisms for handling Member<B>
+// involve dynamic dispatch. Note that for Member<A> all methods and pointers
+// are statically computed and no dynamic dispatch is involved.
+//
+// Note that garbage collections are allowed during mixin construction as
+// conservative scanning of objects does not rely on the Trace method but rather
+// scans the object field by field.
 class PLATFORM_EXPORT GarbageCollectedMixin {
  public:
   typedef int IsGarbageCollectedMixinMarker;
@@ -86,7 +94,8 @@ class PLATFORM_EXPORT GarbageCollectedMixin {
   // these objects can processed later on. This is necessary as
   // not-fully-constructed mixin objects potentially require being processed
   // as part emitting a write barrier for incremental marking. See
-  // IncrementalMarkingTest::WriteBarrierDuringMixinConstruction as an example.
+  // |IncrementalMarkingTest::WriteBarrierDuringMixinConstruction| as an
+  // example.
   //
   // The not-fully-constructed objects are handled as follows:
   //   1. Write barrier or marking of not fully constructed mixin gets called.
@@ -127,95 +136,34 @@ class PLATFORM_EXPORT GarbageCollectedMixin {
                                                                              \
  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 prevent GCs in that restricted window of a mixin object's construction:
-//
-//  - 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().
-
-#define DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)           \
- public:                                                                  \
-  GarbageCollectedMixinConstructorMarker<ThreadingTrait<TYPE>::kAffinity> \
-      mixin_constructor_marker_;                                          \
-                                                                          \
+// The Oilpan GC plugin checks for proper usages of the
+// USING_GARBAGE_COLLECTED_MIXIN macro using a typedef marker.
+#define DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE) \
+ public:                                                        \
+  typedef int HasUsingGarbageCollectedMixinMacro;               \
+                                                                \
  private:
 
-// Mixins that wrap/nest others requires extra handling:
-//
-//  class A : public GarbageCollected<A>, public GarbageCollectedMixin {
-//  USING_GARBAGE_COLLECTED_MIXIN(A);
-//  ...
-//  }'
-//  public B final : public A, public SomeOtherMixinInterface {
-//  USING_GARBAGE_COLLECTED_MIXIN(B);
-//  ...
-//  };
-//
-// The "operator new" for B will enter the forbidden GC scope, but
-// upon construction, two GarbageCollectedMixinConstructorMarker constructors
-// will run -- one for A (first) and another for B (secondly). Only
-// the second one should leave the forbidden GC scope. This is realized by
-// recording the address of B's GarbageCollectedMixinConstructorMarker
-// when the "operator new" for B runs, and leaving the forbidden GC scope
-// when the constructor of the recorded GarbageCollectedMixinConstructorMarker
-// runs.
+// The USING_GARBAGE_COLLECTED_MIXIN macro defines all methods and markers
+// needed for handling mixins.
 #define USING_GARBAGE_COLLECTED_MIXIN(TYPE)    \
   IS_GARBAGE_COLLECTED_TYPE();                 \
   DEFINE_GARBAGE_COLLECTED_MIXIN_METHODS(TYPE) \
   DEFINE_GARBAGE_COLLECTED_MIXIN_CONSTRUCTOR_MARKER(TYPE)
 
-// 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.
-class GarbageCollectedMixinConstructorMarkerBase {};
-template <ThreadAffinity affinity>
-class GarbageCollectedMixinConstructorMarker
-    : public GarbageCollectedMixinConstructorMarkerBase {
- 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<affinity>::GetState();
-    state->LeaveGCForbiddenScopeIfNeeded(this);
-  }
-};
-
 // Merge two or more Mixins into one:
 //
 //  class A : public GarbageCollectedMixin {};
 //  class B : public GarbageCollectedMixin {};
 //  class C : public A, public B {
 //    // C::GetTraceDescriptor is now ambiguous because there are two
-//    candidates:
-//    // A::GetTraceDescriptor and B::GetTraceDescriptor.  Ditto for other
-//    functions.
+//    // candidates: A::GetTraceDescriptor and B::GetTraceDescriptor.  Ditto for
+//    // other functions.
 //
 //    MERGE_GARBAGE_COLLECTED_MIXINS();
 //    // The macro defines C::GetTraceDescriptor, etc. so that they are no
-//    longer
-//    // ambiguous. USING_GARBAGE_COLLECTED_MIXIN(TYPE) overrides them later
-//    // and provides the implementations.
+//    // longer ambiguous. USING_GARBAGE_COLLECTED_MIXIN(TYPE) overrides them
+//    // later and provides the implementations.
 //  };
 #define MERGE_GARBAGE_COLLECTED_MIXINS()                                 \
  public:                                                                 \

third_party/blink/renderer/platform/heap/heap.cc

@@ -165,10 +165,31 @@ void ThreadHeap::CommitCallbackStacks() {
 
 void ThreadHeap::DecommitCallbackStacks() {
   marking_worklist_.reset(nullptr);
-  not_fully_constructed_worklist_.reset(nullptr);
   previously_not_fully_constructed_worklist_.reset(nullptr);
   weak_callback_worklist_.reset(nullptr);
   ephemeron_callbacks_.clear();
+
+  // The fixed point iteration may have found not-fully-constructed objects.
+  // Such objects should have already been found through the stack scan though
+  // and should thus already be marked.
+  if (!not_fully_constructed_worklist_->IsGlobalEmpty()) {
+#if DCHECK_IS_ON()
+    NotFullyConstructedItem item;
+    while (not_fully_constructed_worklist_->Pop(WorklistTaskId::MainThread,
+                                                &item)) {
+      BasePage* const page = PageFromObject(item);
+      HeapObjectHeader* const header =
+          page->IsLargeObjectPage()
+              ? static_cast<LargeObjectPage*>(page)->ObjectHeader()
+              : static_cast<NormalPage*>(page)->FindHeaderFromAddress(
+                    reinterpret_cast<Address>(const_cast<void*>(item)));
+      DCHECK(header->IsMarked());
+    }
+#else
+    not_fully_constructed_worklist_->Clear();
+#endif
+  }
+  not_fully_constructed_worklist_.reset(nullptr);
 }
 
 HeapCompact* ThreadHeap::Compaction() {
@@ -260,8 +281,6 @@ bool DrainWorklistWithDeadline(TimeTicks deadline,
 }  // namespace
 
 bool ThreadHeap::AdvanceMarking(MarkingVisitor* visitor, TimeTicks deadline) {
-  // const size_t kDeadlineCheckInterval = 2500;
-  // size_t processed_callback_count = 0;
   bool finished;
   // Ephemeron fixed point loop.
   do {
@@ -271,11 +290,13 @@ bool ThreadHeap::AdvanceMarking(MarkingVisitor* visitor, TimeTicks deadline) {
       ThreadHeapStatsCollector::Scope stats_scope(
           stats_collector(), ThreadHeapStatsCollector::kMarkProcessWorklist);
 
-      finished =
-          DrainWorklistWithDeadline(deadline, marking_worklist_.get(),
-                                    [visitor](const MarkingItem& item) {
-                                      item.callback(visitor, item.object);
-                                    });
+      finished = DrainWorklistWithDeadline(
+          deadline, marking_worklist_.get(),
+          [visitor](const MarkingItem& item) {
+            DCHECK(!HeapObjectHeader::FromPayload(item.object)
+                        ->IsInConstruction());
+            item.callback(visitor, item.object);
+          });
       if (!finished)
         return false;
 
@@ -600,6 +621,12 @@ void ThreadHeap::WriteBarrier(void* value) {
   if (header->IsMarked())
     return;
 
+  if (header->IsInConstruction()) {
+    not_fully_constructed_worklist_->Push(WorklistTaskId::MainThread,
+                                          header->Payload());
+    return;
+  }
+
   // Mark and push trace callback.
   header->Mark();
   marking_worklist_->Push(

third_party/blink/renderer/platform/heap/heap.h

@@ -121,6 +121,8 @@ class WeakMember;
 template <typename T>
 class UntracedMember;
 
+namespace internal {
+
 template <typename T, bool = NeedsAdjustPointer<T>::value>
 class ObjectAliveTrait;
 
@@ -143,10 +145,17 @@ class ObjectAliveTrait<T, true> {
   NO_SANITIZE_ADDRESS
   static bool IsHeapObjectAlive(const T* object) {
     static_assert(sizeof(T), "T must be fully defined");
-    return object->GetHeapObjectHeader()->IsMarked();
+    const HeapObjectHeader* header = object->GetHeapObjectHeader();
+    if (header == BlinkGC::kNotFullyConstructedObject) {
+      // Objects under construction are always alive.
+      return true;
+    }
+    return header->IsMarked();
   }
 };
 
+}  // namespace internal
+
 class PLATFORM_EXPORT ThreadHeap {
  public:
   explicit ThreadHeap(ThreadState*);
@@ -173,7 +182,7 @@ class PLATFORM_EXPORT ThreadHeap {
       return true;
     DCHECK(&ThreadState::Current()->Heap() ==
            &PageFromObject(object)->Arena()->GetThreadState()->Heap());
-    return ObjectAliveTrait<T>::IsHeapObjectAlive(object);
+    return internal::ObjectAliveTrait<T>::IsHeapObjectAlive(object);
   }
   template <typename T>
   static inline bool IsHeapObjectAlive(const Member<T>& member) {
@@ -546,53 +555,18 @@ class GarbageCollected {
   DISALLOW_COPY_AND_ASSIGN(GarbageCollected);
 };
 
-template <typename T, bool is_mixin = IsGarbageCollectedMixin<T>::value>
-class ConstructTrait {
- public:
-};
-
-template <typename T>
-class ConstructTrait<T, false> {
- public:
-  template <typename... Args>
-  static T* Construct(Args&&... args) {
-    void* memory =
-        T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
-    HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
-    header->MarkIsInConstruction();
-    // Placement new as regular operator new() is deleted.
-    T* object = ::new (memory) T(std::forward<Args>(args)...);
-    header->UnmarkIsInConstruction();
-    return object;
-  }
-};
-
-template <typename T>
-class ConstructTrait<T, true> {
- public:
-  template <typename... Args>
-  NO_SANITIZE_UNRELATED_CAST static T* Construct(Args&&... args) {
-    void* memory =
-        T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
-    HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
-    header->MarkIsInConstruction();
-    ThreadState* state =
-        ThreadStateFor<ThreadingTrait<T>::kAffinity>::GetState();
-    state->EnterGCForbiddenScopeIfNeeded(
-        &(reinterpret_cast<T*>(memory)->mixin_constructor_marker_));
-    // Placement new as regular operator new() is deleted.
-    T* object = ::new (memory) T(std::forward<Args>(args)...);
-    header->UnmarkIsInConstruction();
-    return object;
-  }
-};
-
 // Constructs an instance of T, which is a garbage collected type.
 template <typename T, typename... Args>
 T* MakeGarbageCollected(Args&&... args) {
   static_assert(WTF::IsGarbageCollectedType<T>::value,
                 "T needs to be a garbage collected object");
-  return ConstructTrait<T>::Construct(std::forward<Args>(args)...);
+  void* memory = T::AllocateObject(sizeof(T), IsEagerlyFinalizedType<T>::value);
+  HeapObjectHeader* header = HeapObjectHeader::FromPayload(memory);
+  header->MarkIsInConstruction();
+  // Placement new as regular operator new() is deleted.
+  T* object = ::new (memory) T(std::forward<Args>(args)...);
+  header->UnmarkIsInConstruction();
+  return object;
 }
 
 // Assigning class types to their arenas.
@@ -753,7 +727,7 @@ void Visitor::HandleWeakCell(Visitor* self, void* object) {
       // preserved to avoid reviving objects in containers.
       return;
     }
-    if (!ObjectAliveTrait<T>::IsHeapObjectAlive(contents))
+    if (!ThreadHeap::IsHeapObjectAlive(contents))
       *cell = nullptr;
   }
 }

third_party/blink/renderer/platform/heap/heap_test.cc

@@ -5367,59 +5367,6 @@ static bool AllocateAndReturnBool() {
   return true;
 }
 
-static bool CheckGCForbidden() {
-  DCHECK(ThreadState::Current()->IsGCForbidden());
-  return true;
-}
-
-class MixinClass : public GarbageCollectedMixin {
- public:
-  MixinClass() : dummy_(CheckGCForbidden()) {}
-
- private:
-  bool dummy_;
-};
-
-class ClassWithGarbageCollectingMixinConstructor
-    : public GarbageCollected<ClassWithGarbageCollectingMixinConstructor>,
-      public MixinClass {
-  USING_GARBAGE_COLLECTED_MIXIN(ClassWithGarbageCollectingMixinConstructor);
-
- public:
-  static int trace_called_;
-
-  ClassWithGarbageCollectingMixinConstructor()
-      : trace_counter_(TraceCounter::Create()),
-        wrapper_(IntWrapper::Create(32)) {}
-
-  void Trace(blink::Visitor* visitor) override {
-    trace_called_++;
-    visitor->Trace(trace_counter_);
-    visitor->Trace(wrapper_);
-  }
-
-  void Verify() {
-    EXPECT_EQ(32, wrapper_->Value());
-    EXPECT_EQ(0, trace_counter_->TraceCount());
-    EXPECT_EQ(0, trace_called_);
-  }
-
- private:
-  Member<TraceCounter> trace_counter_;
-  Member<IntWrapper> wrapper_;
-};
-
-int ClassWithGarbageCollectingMixinConstructor::trace_called_ = 0;
-
-// Regression test for out of bounds call through vtable.
-// Passes if it doesn't crash.
-TEST(HeapTest, GarbageCollectionDuringMixinConstruction) {
-  ClassWithGarbageCollectingMixinConstructor::trace_called_ = 0;
-  ClassWithGarbageCollectingMixinConstructor* a =
-      MakeGarbageCollected<ClassWithGarbageCollectingMixinConstructor>();
-  a->Verify();
-}
-
 template <typename T>
 class TraceIfNeededTester
     : public GarbageCollectedFinalized<TraceIfNeededTester<T>> {
@@ -5928,11 +5875,7 @@ class TestMixinAllocationA : public GarbageCollected<TestMixinAllocationA>,
   USING_GARBAGE_COLLECTED_MIXIN(TestMixinAllocationA);
 
  public:
-  TestMixinAllocationA() {
-    // Completely wrong in general, but test only
-    // runs this constructor while constructing another mixin.
-    DCHECK(ThreadState::Current()->IsGCForbidden());
-  }
+  TestMixinAllocationA() = default;
 
   void Trace(blink::Visitor* visitor) override {}
 };
@@ -5942,14 +5885,8 @@ class TestMixinAllocationB : public TestMixinAllocationA {
 
  public:
   TestMixinAllocationB()
-      : a_(MakeGarbageCollected<TestMixinAllocationA>())  // Construct object
-                                                          // during a mixin
-                                                          // construction.
-  {
-    // Completely wrong in general, but test only
-    // runs this constructor while constructing another mixin.
-    DCHECK(ThreadState::Current()->IsGCForbidden());
-  }
+      // Construct object during a mixin construction.
+      : a_(MakeGarbageCollected<TestMixinAllocationA>()) {}
 
   void Trace(blink::Visitor* visitor) override {
     visitor->Trace(a_);
@@ -5994,47 +5931,6 @@ class ObjectWithLargeAmountsOfAllocationInConstructor {
   }
 };
 
-class TestMixinAllocatingObject final
-    : public TestMixinAllocationB,
-      public ObjectWithLargeAmountsOfAllocationInConstructor {
-  USING_GARBAGE_COLLECTED_MIXIN(TestMixinAllocatingObject);
-
- public:
-  static TestMixinAllocatingObject* Create(ClassWithMember* member) {
-    return MakeGarbageCollected<TestMixinAllocatingObject>(member);
-  }
-
-  TestMixinAllocatingObject(ClassWithMember* member)
-      : ObjectWithLargeAmountsOfAllocationInConstructor(600, member),
-        trace_counter_(TraceCounter::Create()) {
-    DCHECK(!ThreadState::Current()->IsGCForbidden());
-    ConservativelyCollectGarbage();
-    EXPECT_GT(member->TraceCount(), 0);
-    EXPECT_GT(TraceCount(), 0);
-  }
-
-  void Trace(blink::Visitor* visitor) override {
-    visitor->Trace(trace_counter_);
-    TestMixinAllocationB::Trace(visitor);
-  }
-
-  int TraceCount() const { return trace_counter_->TraceCount(); }
-
- private:
-  Member<TraceCounter> trace_counter_;
-};
-
-TEST(HeapTest, MixinConstructionNoGC) {
-  ClearOutOldGarbage();
-  Persistent<ClassWithMember> object = ClassWithMember::Create();
-  EXPECT_EQ(0, object->TraceCount());
-  TestMixinAllocatingObject* mixin =
-      TestMixinAllocatingObject::Create(object.Get());
-  EXPECT_TRUE(mixin);
-  EXPECT_GT(object->TraceCount(), 0);
-  EXPECT_GT(mixin->TraceCount(), 0);
-}
-
 class WeakPersistentHolder final {
  public:
   explicit WeakPersistentHolder(IntWrapper* object) : object_(object) {}
@@ -6420,85 +6316,30 @@ TEST(HeapTest, ShrinkVector) {
   vector.ShrinkToFit();
 }
 
-namespace {
-
-class MixinCheckingConstructionScope : public GarbageCollectedMixin {
- public:
-  MixinCheckingConstructionScope() {
-    // Oilpan treats mixin construction as forbidden scopes for garbage
-    // collection.
-    CHECK(ThreadState::Current()->IsMixinInConstruction());
-  }
-};
-
-class UsingMixinCheckingConstructionScope
-    : public GarbageCollected<UsingMixinCheckingConstructionScope>,
-      public MixinCheckingConstructionScope {
-  USING_GARBAGE_COLLECTED_MIXIN(UsingMixinCheckingConstructionScope);
-};
-
-}  // namespace
-
-TEST(HeapTest, NoConservativeGCDuringMixinConstruction) {
-  // Regression test: https://crbug.com/904546
-  MakeGarbageCollected<UsingMixinCheckingConstructionScope>();
+TEST(HeapTest, GarbageCollectedInConstruction) {
+  using O = ObjectWithCallbackBeforeInitializer<IntWrapper>;
+  MakeGarbageCollected<O>(base::BindOnce([](O* thiz) {
+    CHECK(HeapObjectHeader::FromPayload(thiz)->IsInConstruction());
+  }));
 }
 
-namespace {
-
-class ObjectCheckingForInConstruction
-    : public GarbageCollected<ObjectCheckingForInConstruction> {
- public:
-  ObjectCheckingForInConstruction() {
-    CHECK(HeapObjectHeader::FromPayload(this)->IsInConstruction());
-  }
-
-  virtual void Trace(Visitor* v) { v->Trace(foo_); }
-
- private:
-  Member<IntWrapper> foo_;
-};
-
-class MixinCheckingInConstruction : public GarbageCollectedMixin {
- public:
-  MixinCheckingInConstruction() {
-    BasePage* const page = PageFromObject(reinterpret_cast<Address>(this));
+TEST(HeapTest, GarbageCollectedMixinInConstruction) {
+  using O = ObjectWithMixinWithCallbackBeforeInitializer<IntWrapper>;
+  MakeGarbageCollected<O>(base::BindOnce([](O::Mixin* thiz) {
+    BasePage* const page = PageFromObject(thiz);
     HeapObjectHeader* const header =
-        static_cast<NormalPage*>(page)->FindHeaderFromAddress(
-            reinterpret_cast<Address>(
-                const_cast<MixinCheckingInConstruction*>(this)));
+        page->IsLargeObjectPage()
+            ? static_cast<LargeObjectPage*>(page)->ObjectHeader()
+            : static_cast<NormalPage*>(page)->FindHeaderFromAddress(
+                  reinterpret_cast<Address>(thiz));
     CHECK(header->IsInConstruction());
-  }
-
-  void Trace(Visitor* v) override { v->Trace(bar_); }
-
- private:
-  Member<IntWrapper> bar_;
-};
-
-class MixinAppCheckingInConstruction
-    : public GarbageCollected<MixinAppCheckingInConstruction>,
-      public MixinCheckingInConstruction {
-  USING_GARBAGE_COLLECTED_MIXIN(MixinAppCheckingInConstruction)
- public:
-  MixinAppCheckingInConstruction() {
-    CHECK(HeapObjectHeader::FromPayload(this)->IsInConstruction());
-  }
-
-  void Trace(Visitor* v) override { v->Trace(foo_); }
-
- private:
-  Member<IntWrapper> foo_;
-};
-
-}  // namespace
-
-TEST(HeapTest, GarbageCollectedInConstruction) {
-  MakeGarbageCollected<ObjectCheckingForInConstruction>();
+  }));
 }
 
-TEST(HeapTest, GarbageCollectedMixinInConstruction) {
-  MakeGarbageCollected<MixinAppCheckingInConstruction>();
+TEST(HeapTest, GarbageCollectedMixinIsAliveDuringConstruction) {
+  using O = ObjectWithMixinWithCallbackBeforeInitializer<IntWrapper>;
+  MakeGarbageCollected<O>(base::BindOnce(
+      [](O::Mixin* thiz) { CHECK(ThreadHeap::IsHeapObjectAlive(thiz)); }));
 }
 
 }  // namespace blink

third_party/blink/renderer/platform/heap/heap_test_utilities.h

@@ -7,7 +7,12 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_
 
+#include "base/callback.h"
 #include "third_party/blink/renderer/platform/heap/blink_gc.h"
+#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
+#include "third_party/blink/renderer/platform/heap/heap.h"
+#include "third_party/blink/renderer/platform/heap/trace_traits.h"
+#include "third_party/blink/renderer/platform/heap/visitor.h"
 
 namespace blink {
 
@@ -16,6 +21,90 @@ void ConservativelyCollectGarbage(
     BlinkGC::SweepingType sweeping_type = BlinkGC::kEagerSweeping);
 void ClearOutOldGarbage();
 
+template <typename T>
+class ObjectWithCallbackBeforeInitializer
+    : public GarbageCollected<ObjectWithCallbackBeforeInitializer<T>> {
+ public:
+  ObjectWithCallbackBeforeInitializer(
+      base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb,
+      T* value)
+      : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))), value_(value) {}
+
+  ObjectWithCallbackBeforeInitializer(
+      base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb)
+      : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))) {}
+
+  virtual void Trace(Visitor* visitor) { visitor->Trace(value_); }
+
+  T* value() const { return value_.Get(); }
+
+ private:
+  static bool ExecuteCallbackReturnTrue(
+      ObjectWithCallbackBeforeInitializer* thiz,
+      base::OnceCallback<void(ObjectWithCallbackBeforeInitializer<T>*)>&& cb) {
+    std::move(cb).Run(thiz);
+    return true;
+  }
+
+  bool bool_;
+  Member<T> value_;
+};
+
+template <typename T>
+class MixinWithCallbackBeforeInitializer : public GarbageCollectedMixin {
+ public:
+  MixinWithCallbackBeforeInitializer(
+      base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb,
+      T* value)
+      : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))), value_(value) {}
+
+  MixinWithCallbackBeforeInitializer(
+      base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb)
+      : bool_(ExecuteCallbackReturnTrue(this, std::move(cb))) {}
+
+  void Trace(Visitor* visitor) override { visitor->Trace(value_); }
+
+  T* value() const { return value_.Get(); }
+
+ private:
+  static bool ExecuteCallbackReturnTrue(
+      MixinWithCallbackBeforeInitializer* thiz,
+      base::OnceCallback<void(MixinWithCallbackBeforeInitializer<T>*)>&& cb) {
+    std::move(cb).Run(thiz);
+    return true;
+  }
+
+  bool bool_;
+  Member<T> value_;
+};
+
+class BoolMixin {
+ protected:
+  bool bool_ = false;
+};
+
+template <typename T>
+class ObjectWithMixinWithCallbackBeforeInitializer
+    : public GarbageCollected<ObjectWithMixinWithCallbackBeforeInitializer<T>>,
+      public BoolMixin,
+      public MixinWithCallbackBeforeInitializer<T> {
+  USING_GARBAGE_COLLECTED_MIXIN(ObjectWithMixinWithCallbackBeforeInitializer);
+
+ public:
+  using Mixin = MixinWithCallbackBeforeInitializer<T>;
+
+  ObjectWithMixinWithCallbackBeforeInitializer(
+      base::OnceCallback<void(Mixin*)>&& cb,
+      T* value)
+      : Mixin(std::move(cb), value) {}
+
+  ObjectWithMixinWithCallbackBeforeInitializer(
+      base::OnceCallback<void(Mixin*)>&& cb)
+      : Mixin(std::move(cb)) {}
+
+  void Trace(Visitor* visitor) override { Mixin::Trace(visitor); }
+};
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_HEAP_HEAP_TEST_UTILITIES_H_

third_party/blink/renderer/platform/heap/incremental_marking_test.cc

@@ -1563,28 +1563,30 @@ class IncrementalMarkingTestDriver {
     thread_state_->IncrementalMarkingStart(BlinkGC::GCReason::kTesting);
   }
 
-  bool SingleStep() {
+  bool SingleStep(BlinkGC::StackState stack_state =
+                      BlinkGC::StackState::kNoHeapPointersOnStack) {
     CHECK(thread_state_->IsIncrementalMarking());
     if (thread_state_->GetGCState() ==
         ThreadState::kIncrementalMarkingStepScheduled) {
-      thread_state_->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
+      thread_state_->IncrementalMarkingStep(stack_state);
       return true;
     }
     return false;
   }
 
-  void FinishSteps() {
+  void FinishSteps(BlinkGC::StackState stack_state =
+                       BlinkGC::StackState::kNoHeapPointersOnStack) {
     CHECK(thread_state_->IsIncrementalMarking());
-    while (SingleStep()) {
+    while (SingleStep(stack_state)) {
     }
   }
 
   void FinishGC() {
     CHECK(thread_state_->IsIncrementalMarking());
-    FinishSteps();
+    FinishSteps(BlinkGC::StackState::kNoHeapPointersOnStack);
     CHECK_EQ(ThreadState::kIncrementalMarkingFinalizeScheduled,
              thread_state_->GetGCState());
-    thread_state_->RunScheduledGC(BlinkGC::kNoHeapPointersOnStack);
+    thread_state_->RunScheduledGC(BlinkGC::StackState::kNoHeapPointersOnStack);
     CHECK(!thread_state_->IsIncrementalMarking());
     thread_state_->CompleteSweep();
   }
@@ -1801,6 +1803,80 @@ TEST(IncrementalMarkingTest, MemberSwap) {
   ConservativelyCollectGarbage();
 }
 
+namespace {
+
+template <typename T>
+class ObjectHolder : public GarbageCollected<ObjectHolder<T>> {
+ public:
+  ObjectHolder() = default;
+
+  virtual void Trace(Visitor* visitor) { visitor->Trace(holder_); }
+
+  void set_value(T* value) { holder_ = value; }
+  T* value() const { return holder_.Get(); }
+
+ private:
+  Member<T> holder_;
+};
+
+}  // namespace
+
+TEST(IncrementalMarkingTest, StepDuringObjectConstruction) {
+  // Test ensures that objects in construction are delayed for processing to
+  // allow omitting write barriers on initializing stores.
+
+  using O = ObjectWithCallbackBeforeInitializer<Object>;
+  using Holder = ObjectHolder<O>;
+  Persistent<Holder> holder(MakeGarbageCollected<Holder>());
+  IncrementalMarkingTestDriver driver(ThreadState::Current());
+  driver.Start();
+  MakeGarbageCollected<O>(
+      base::BindOnce(
+          [](IncrementalMarkingTestDriver* driver, Holder* holder, O* thiz) {
+            // Publish not-fully-constructed object |thiz| by triggering write
+            // barrier for the object.
+            holder->set_value(thiz);
+            CHECK(HeapObjectHeader::FromPayload(holder->value())->IsValid());
+            // Finish call incremental steps.
+            driver->FinishSteps(BlinkGC::StackState::kHeapPointersOnStack);
+          },
+          &driver, holder.Get()),
+      MakeGarbageCollected<Object>());
+  driver.FinishGC();
+  CHECK(HeapObjectHeader::FromPayload(holder->value())->IsValid());
+  CHECK(HeapObjectHeader::FromPayload(holder->value()->value())->IsValid());
+  PreciselyCollectGarbage();
+}
+
+TEST(IncrementalMarkingTest, StepDuringMixinObjectConstruction) {
+  // Test ensures that mixin objects in construction are delayed for processing
+  // to allow omitting write barriers on initializing stores.
+
+  using Parent = ObjectWithMixinWithCallbackBeforeInitializer<Object>;
+  using Mixin = MixinWithCallbackBeforeInitializer<Object>;
+  using Holder = ObjectHolder<Mixin>;
+  Persistent<Holder> holder(MakeGarbageCollected<Holder>());
+  IncrementalMarkingTestDriver driver(ThreadState::Current());
+  driver.Start();
+  MakeGarbageCollected<Parent>(
+      base::BindOnce(
+          [](IncrementalMarkingTestDriver* driver, Holder* holder,
+             Mixin* thiz) {
+            // Publish not-fully-constructed object
+            // |thiz| by triggering write barrier for
+            // the object.
+            holder->set_value(thiz);
+            // Finish call incremental steps.
+            driver->FinishSteps(BlinkGC::StackState::kHeapPointersOnStack);
+          },
+          &driver, holder.Get()),
+      MakeGarbageCollected<Object>());
+  driver.FinishGC();
+  CHECK(holder->value()->GetHeapObjectHeader()->IsValid());
+  CHECK(HeapObjectHeader::FromPayload(holder->value()->value())->IsValid());
+  PreciselyCollectGarbage();
+}
+
 }  // namespace incremental_marking_test
 }  // namespace blink
 

third_party/blink/renderer/platform/heap/marking_visitor.cc

@@ -64,11 +64,10 @@ void MarkingVisitor::ConservativelyMarkAddress(BasePage* page,
   if (!header || header->IsMarked())
     return;
 
-  // Simple case for fully constructed objects or those that have no vtable
-  // which make dispatching to member fields trivial.
+  // Simple case for fully constructed objects.
   const GCInfo* gc_info =
       GCInfoTable::Get().GCInfoFromIndex(header->GcInfoIndex());
-  if (!gc_info->HasVTable() || !header->IsInConstruction()) {
+  if (!header->IsInConstruction()) {
     MarkHeader(header, gc_info->trace_);
     return;
   }

third_party/blink/renderer/platform/heap/marking_visitor.h

@@ -51,11 +51,14 @@ class PLATFORM_EXPORT MarkingVisitor : public Visitor {
 
   // Marking implementation.
 
-  // Conservatively marks an object if pointed to by Address.
+  // Conservatively marks an object if pointed to by Address. The object may
+  // be in construction as the scan is conservative without relying on a
+  // Trace method.
   void ConservativelyMarkAddress(BasePage*, Address);
 
   // Marks an object dynamically using any address within its body and adds a
-  // tracing callback for processing of the object.
+  // tracing callback for processing of the object. The object is not allowed
+  // to be in construction.
   void DynamicallyMarkAddress(Address);
 
   // Marks an object and adds a tracing callback for processing of the object.
@@ -97,8 +100,11 @@ class PLATFORM_EXPORT MarkingVisitor : public Visitor {
       // that lead to many recursions.
       DCHECK(Heap().GetStackFrameDepth().IsAcceptableStackUse());
       if (LIKELY(Heap().GetStackFrameDepth().IsSafeToRecurse())) {
-        if (MarkHeaderNoTracing(
-                HeapObjectHeader::FromPayload(desc.base_object_payload))) {
+        HeapObjectHeader* header =
+            HeapObjectHeader::FromPayload(desc.base_object_payload);
+        if (header->IsInConstruction()) {
+          not_fully_constructed_worklist_.Push(desc.base_object_payload);
+        } else if (MarkHeaderNoTracing(header)) {
           desc.callback(this, desc.base_object_payload);
         }
         return;
@@ -199,7 +205,9 @@ inline void MarkingVisitor::MarkHeader(HeapObjectHeader* header,
   DCHECK(header);
   DCHECK(callback);
 
-  if (MarkHeaderNoTracing(header)) {
+  if (header->IsInConstruction()) {
+    not_fully_constructed_worklist_.Push(header->Payload());
+  } else if (MarkHeaderNoTracing(header)) {
     marking_worklist_.Push(
         {reinterpret_cast<void*>(header->Payload()), callback});
   }

third_party/blink/renderer/platform/heap/persistent.h

@@ -269,7 +269,7 @@ class PersistentBase {
                        weaknessConfiguration, crossThreadnessConfiguration>;
     Base* persistent = reinterpret_cast<Base*>(persistent_pointer);
     T* object = persistent->Get();
-    if (object && !ObjectAliveTrait<T>::IsHeapObjectAlive(object))
+    if (object && !ThreadHeap::IsHeapObjectAlive(object))
       ClearWeakPersistent(persistent);
   }
 

third_party/blink/renderer/platform/heap/thread_state.cc

@@ -177,7 +177,6 @@ ThreadState::ThreadState()
       mixins_being_constructed_count_(0),
       object_resurrection_forbidden_(false),
       in_atomic_pause_(false),
-      gc_mixin_marker_(nullptr),
       gc_state_(kNoGCScheduled),
       gc_phase_(GCPhase::kNone),
       reason_for_scheduled_gc_(BlinkGC::GCReason::kMaxValue),

third_party/blink/renderer/platform/heap/thread_state.h

@@ -62,7 +62,6 @@ class IncrementalMarkingScope;
 class IncrementalMarkingTestDriver;
 }  // namespace incremental_marking_test
 
-class GarbageCollectedMixinConstructorMarkerBase;
 class MarkingVisitor;
 class PersistentNode;
 class PersistentRegion;
@@ -480,29 +479,6 @@ class PLATFORM_EXPORT ThreadState final
     perform_cleanup_ = perform_cleanup;
   }
 
-  // By entering a gc-forbidden scope, conservative GCs will not
-  // be allowed while handling an out-of-line allocation request.
-  // Intended used when constructing subclasses of GC mixins, where
-  // the object being constructed cannot be safely traced & marked
-  // fully should a GC be allowed while its subclasses are being
-  // constructed.
-  void EnterGCForbiddenScopeIfNeeded(
-      GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker) {
-    DCHECK(CheckThread());
-    if (!gc_mixin_marker_) {
-      EnterMixinConstructionScope();
-      gc_mixin_marker_ = gc_mixin_marker;
-    }
-  }
-  void LeaveGCForbiddenScopeIfNeeded(
-      GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker) {
-    DCHECK(CheckThread());
-    if (gc_mixin_marker_ == gc_mixin_marker) {
-      LeaveMixinConstructionScope();
-      gc_mixin_marker_ = nullptr;
-    }
-  }
-
   void FreePersistentNode(PersistentRegion*, PersistentNode*);
 
   using PersistentClearCallback = void (*)(void*);
@@ -707,8 +683,6 @@ class PLATFORM_EXPORT ThreadState final
   TimeDelta next_incremental_marking_step_duration_;
   TimeDelta previous_incremental_marking_time_left_;
 
-  GarbageCollectedMixinConstructorMarkerBase* gc_mixin_marker_;
-
   GCState gc_state_;
   GCPhase gc_phase_;
   BlinkGC::GCReason reason_for_scheduled_gc_;