Revert "heap: Keep object header on pre-finalizer registration"

This reverts commit 5342b454.

Reason for revert: Dynamic header lookup cannot work while concurrent sweeping is active. More details see bug.

Original change's description:
> heap: Keep object header on pre-finalizer registration
> 
> Pre-finalizers are heavy and not supposed to be used on short-living small
> objects. Spend an additional word to store the header on registration. This
> allows inspecting liveness without going through a call on every garbage
> collection.
> 
> Bug: 982754
> Change-Id: I30a2d60198dd6b2b49febcd49983d17175874dad
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1904177
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Omer Katz <omerkatz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#713880}

TBR=mlippautz@chromium.org,omerkatz@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 982754,1033662
Change-Id: Ice55879d9b94a413a324ad745329a6c0aecda42b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1992422Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#729705}

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

@@ -1082,21 +1082,6 @@ void ThreadState::FreePersistentNode(PersistentRegion* persistent_region,
     DCHECK(!static_persistents_.Contains(persistent_node));
 }
 
-void ThreadState::RegisterPreFinalizer(void* object,
-                                       PreFinalizerCallback callback) {
-#if DCHECK_IS_ON()
-  DCHECK(CheckThread());
-#endif
-  DCHECK(!SweepForbidden());
-
-  HeapObjectHeader* header = HeapObjectHeader::FromInnerAddress(object);
-  DCHECK(ordered_pre_finalizers_.end() ==
-         std::find(ordered_pre_finalizers_.begin(),
-                   ordered_pre_finalizers_.end(),
-                   PreFinalizer{header, object, callback}));
-  ordered_pre_finalizers_.push_back(PreFinalizer{header, object, callback});
-}
-
 void ThreadState::InvokePreFinalizers() {
   DCHECK(CheckThread());
   DCHECK(!SweepForbidden());
@@ -1104,22 +1089,23 @@ void ThreadState::InvokePreFinalizers() {
   ThreadHeapStatsCollector::Scope stats_scope(
       Heap().stats_collector(), ThreadHeapStatsCollector::kInvokePreFinalizers);
   SweepForbiddenScope sweep_forbidden(this);
-  // Pre finalizers are forbidden from allocating objects.
+  // Pre finalizers are forbidden from allocating objects
   NoAllocationScope no_allocation_scope(this);
 
   // Call the prefinalizers in the opposite order to their registration.
+  //
+  // LinkedHashSet does not support modification during iteration, so
+  // copy items first.
+  //
+  // The prefinalizer callback wrapper returns |true| when its associated
+  // object is unreachable garbage and the prefinalizer callback has run.
+  // The registered prefinalizer entry must then be removed and deleted.
   Deque<PreFinalizer> remaining_ordered_pre_finalizers;
   for (auto rit = ordered_pre_finalizers_.rbegin();
        rit != ordered_pre_finalizers_.rend(); ++rit) {
     const PreFinalizer& pre_finalizer = *rit;
-    // Check if pre-finalizer should be executed.
-    if (pre_finalizer.header->IsMarked()) {
-      // Re-queue for checking in next garbage collection.
+    if (!(pre_finalizer.second)(pre_finalizer.first))
       remaining_ordered_pre_finalizers.push_front(pre_finalizer);
-    } else {
-      // Execute pre-finalizer.
-      pre_finalizer.callback(pre_finalizer.object);
-    }
   }
 
   ordered_pre_finalizers_ = std::move(remaining_ordered_pre_finalizers);

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

@@ -62,7 +62,6 @@ class IncrementalMarkingScope;
 }  // namespace incremental_marking_test
 
 class CancelableTaskScheduler;
-class HeapObjectHeader;
 class MarkingVisitor;
 class PersistentNode;
 class PersistentRegion;
@@ -94,13 +93,17 @@ class Visitor;
 //     Member<Bar> bar_;
 //   };
 #define USING_PRE_FINALIZER(Class, preFinalizer)                          \
- private:                                                                 \
-  static void PreFinalizerDispatch(void* object) {                        \
-    reinterpret_cast<Class*>(object)->Class::preFinalizer();              \
+ public:                                                                  \
+  static bool InvokePreFinalizer(void* object) {                          \
+    Class* self = reinterpret_cast<Class*>(object);                       \
+    if (ThreadHeap::IsHeapObjectAlive(self))                              \
+      return false;                                                       \
+    self->Class::preFinalizer();                                          \
+    return true;                                                          \
   }                                                                       \
                                                                           \
-  friend class ThreadState::PreFinalizerRegistration<Class>;              \
-  ThreadState::PreFinalizerRegistration<Class> prefinalizer_dummy_{this}; \
+ private:                                                                 \
+  ThreadState::PrefinalizerRegistration<Class> prefinalizer_dummy_{this}; \
   using UsingPreFinalizerMacroNeedsTrailingSemiColon = char
 
 class PLATFORM_EXPORT BlinkGCObserver {
@@ -130,16 +133,24 @@ class PLATFORM_EXPORT ThreadState final {
   // Register the pre-finalizer for the |self| object. The class T be using
   // USING_PRE_FINALIZER() macro.
   template <typename T>
-  class PreFinalizerRegistration final {
+  class PrefinalizerRegistration final {
     DISALLOW_NEW();
 
    public:
-    PreFinalizerRegistration(T* self) {
-      static_assert(sizeof(&T::PreFinalizerDispatch) > 0,
+    PrefinalizerRegistration(T* self) {
+      static_assert(sizeof(&T::InvokePreFinalizer) > 0,
                     "USING_PRE_FINALIZER(T) must be defined.");
       ThreadState* state =
           ThreadStateFor<ThreadingTrait<T>::kAffinity>::GetState();
-      state->RegisterPreFinalizer(self, T::PreFinalizerDispatch);
+#if DCHECK_IS_ON()
+      DCHECK(state->CheckThread());
+#endif
+      DCHECK(!state->SweepForbidden());
+      DCHECK(std::find(state->ordered_pre_finalizers_.begin(),
+                       state->ordered_pre_finalizers_.end(),
+                       PreFinalizer(self, T::InvokePreFinalizer)) ==
+             state->ordered_pre_finalizers_.end());
+      state->ordered_pre_finalizers_.emplace_back(self, T::InvokePreFinalizer);
     }
   };
 
@@ -368,17 +379,6 @@ class PLATFORM_EXPORT ThreadState final {
  private:
   class IncrementalMarkingScheduler;
 
-  using PreFinalizerCallback = void (*)(void*);
-  struct PreFinalizer {
-    HeapObjectHeader* header;
-    void* object;
-    PreFinalizerCallback callback;
-
-    bool operator==(const PreFinalizer& other) const {
-      return object == other.object && callback == other.callback;
-    }
-  };
-
   // Duration of one incremental marking step. Should be short enough that it
   // doesn't cause jank even though it is scheduled as a normal task.
   static constexpr base::TimeDelta kDefaultIncrementalMarkingStepDuration =
@@ -518,7 +518,6 @@ class PLATFORM_EXPORT ThreadState final {
 
   void SynchronizeAndFinishConcurrentSweeping();
 
-  void RegisterPreFinalizer(void*, PreFinalizerCallback);
   void InvokePreFinalizers();
 
   // Adds the given observer to the ThreadState's observer list. This doesn't
@@ -567,6 +566,9 @@ class PLATFORM_EXPORT ThreadState final {
   BlinkGC::GCReason reason_for_scheduled_gc_ =
       BlinkGC::GCReason::kForcedGCForTesting;
 
+  using PreFinalizerCallback = bool (*)(void*);
+  using PreFinalizer = std::pair<void*, PreFinalizerCallback>;
+
   // Pre-finalizers are called in the reverse order in which they are
   // registered by the constructors (including constructors of Mixin objects)
   // for an object, by processing the ordered_pre_finalizers_ back-to-front.
@@ -615,7 +617,7 @@ class PLATFORM_EXPORT ThreadState final {
   friend class IncrementalMarkingTestDriver;
   friend class HeapAllocator;
   template <typename T>
-  friend class PreFinalizerRegistration;
+  friend class PrefinalizerRegistration;
   friend class TestGCScope;
   friend class TestSupportingGC;
   friend class ThreadStateSchedulingTest;