Revert "timer: Specialize TaskRunnerTimer for GCed classes"

This reverts commit dc16c8dc.

Reason for revert: Fails on regular ASAN builds because all unreachable objects are poisoned directly after marking.

Original change's description:
> timer: Specialize TaskRunnerTimer for GCed classes
> 
> This allows us getting rid of WillObjectBeLazilySwept which peeks into
> garbage collection internal state.
> 
> Change-Id: I82b38def090c33f1b6e6727ab4b685abb0409ea1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1598826
> Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#657650}

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

Change-Id: I38047dc29ca14158aa891d99c15e218c1fdb7570
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 960775
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1599623Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657686}

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

@@ -212,6 +212,39 @@ class PLATFORM_EXPORT ThreadHeap {
     return weak_callback_worklist_.get();
   }
 
+  // Is the finalizable GC object still alive, but slated for lazy sweeping?
+  // If a lazy sweep is in progress, returns true if the object was found
+  // to be not reachable during the marking phase, but it has yet to be swept
+  // and finalized. The predicate returns false in all other cases.
+  //
+  // Holding a reference to an already-dead object is not a valid state
+  // to be in; willObjectBeLazilySwept() has undefined behavior if passed
+  // such a reference.
+  template <typename T>
+  NO_SANITIZE_ADDRESS static bool WillObjectBeLazilySwept(
+      const T* object_pointer) {
+    static_assert(IsGarbageCollectedType<T>::value,
+                  "only objects deriving from GarbageCollected can be used.");
+    BasePage* page = PageFromObject(object_pointer);
+    // Page has been swept and it is still alive.
+    if (page->HasBeenSwept())
+      return false;
+    DCHECK(page->Arena()->GetThreadState()->IsSweepingInProgress());
+
+    // If marked and alive, the object hasn't yet been swept..and won't
+    // be once its page is processed.
+    if (ThreadHeap::IsHeapObjectAlive(const_cast<T*>(object_pointer)))
+      return false;
+
+    if (page->IsLargeObjectPage())
+      return true;
+
+    // If the object is unmarked, it may be on the page currently being
+    // lazily swept.
+    return page->Arena()->WillObjectBeLazilySwept(
+        page, const_cast<T*>(object_pointer));
+  }
+
   // Register an ephemeron table for fixed-point iteration.
   void RegisterWeakTable(void* container_object,
                          EphemeronCallback);

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

@@ -351,6 +351,68 @@ Address BaseArena::AllocateLargeObject(size_t allocation_size,
   return large_object;
 }
 
+bool BaseArena::WillObjectBeLazilySwept(BasePage* page,
+                                        void* object_pointer) const {
+  // If not on the current page being (potentially) lazily swept,
+  // |objectPointer| is an unmarked, sweepable object.
+  if (page != first_unswept_page_)
+    return true;
+
+  DCHECK(!page->IsLargeObjectPage());
+  // Check if the arena is currently being lazily swept.
+  NormalPage* normal_page = reinterpret_cast<NormalPage*>(page);
+  NormalPageArena* normal_arena = normal_page->ArenaForNormalPage();
+  if (!normal_arena->IsLazySweeping())
+    return true;
+
+  // Rare special case: unmarked object is on the page being lazily swept,
+  // and a finalizer for an object on that page calls
+  // ThreadHeap::willObjectBeLazilySwept().
+  //
+  // Need to determine if |objectPointer| represents a live (unmarked) object or
+  // an unmarked object that will be lazily swept later. As lazy page sweeping
+  // doesn't record a frontier pointer representing how far along it is, the
+  // page is scanned from the start, skipping past freed & unmarked regions.
+  //
+  // If no marked objects are encountered before |objectPointer|, we know that
+  // the finalizing object calling willObjectBeLazilySwept() comes later, and
+  // |objectPointer| has been deemed to be alive already (=> it won't be swept.)
+  //
+  // If a marked object is encountered before |objectPointer|, it will
+  // not have been lazily swept past already. Hence it represents an unmarked,
+  // sweepable object.
+  //
+  // As willObjectBeLazilySwept() is used rarely and it happening to be
+  // used while runnning a finalizer on the page being lazily swept is
+  // even rarer, the page scan is considered acceptable and something
+  // really wanted -- willObjectBeLazilySwept()'s result can be trusted.
+  Address page_end = normal_page->PayloadEnd();
+  for (Address header_address = normal_page->Payload();
+       header_address < page_end;) {
+    HeapObjectHeader* header =
+        reinterpret_cast<HeapObjectHeader*>(header_address);
+    size_t size = header->size();
+    // Scan made it to |objectPointer| without encountering any marked objects.
+    //  => lazy sweep will have processed this unmarked, but live, object.
+    //  => |object_pointer| will not be lazily swept.
+    //
+    // Notice that |object_pointer| might be pointer to a GarbageCollectedMixin,
+    // hence using |FromPayload| to derive the HeapObjectHeader isn't possible
+    // (and use its value to check if |header_address| is equal to it.)
+    if (header_address > object_pointer)
+      return false;
+    if (!header->IsFree() && header->IsMarked()) {
+      // There must be a marked object on this page and the one located must
+      // have room after it for the unmarked |objectPointer| object.
+      DCHECK(header_address + size < page_end);
+      return true;
+    }
+    header_address += size;
+  }
+  NOTREACHED();
+  return true;
+}
+
 NormalPageArena::NormalPageArena(ThreadState* state, int index)
     : BaseArena(state, index),
       current_allocation_point_(nullptr),

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

@@ -754,6 +754,8 @@ class PLATFORM_EXPORT BaseArena {
 
   Address AllocateLargeObject(size_t allocation_size, size_t gc_info_index);
 
+  bool WillObjectBeLazilySwept(BasePage*, void*) const;
+
   virtual void VerifyObjectStartBitmap() {}
   virtual void VerifyMarking() {}
 

third_party/blink/renderer/platform/timer.h

@@ -32,7 +32,6 @@
 #include "base/single_thread_task_runner.h"
 #include "base/time/time.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
-#include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/sanitizers.h"
@@ -112,35 +111,22 @@ class PLATFORM_EXPORT TimerBase {
   DISALLOW_COPY_AND_ASSIGN(TimerBase);
 };
 
-template <typename TimerFiredClass,
-          bool = WTF::IsGarbageCollectedTypeInternal<TimerFiredClass>::value>
-class TaskRunnerTimer;
-
-template <typename TimerFiredClass>
-class TaskRunnerTimer<TimerFiredClass, false> : public TimerBase {
+template <typename T, bool = IsGarbageCollectedType<T>::value>
+class TimerIsObjectAliveTrait {
  public:
-  using TimerFiredFunction = void (TimerFiredClass::*)(TimerBase*);
-
-  TaskRunnerTimer(scoped_refptr<base::SingleThreadTaskRunner> web_task_runner,
-                  TimerFiredClass* o,
-                  TimerFiredFunction f)
-      : TimerBase(std::move(web_task_runner)), object_(o), function_(f) {}
-
-  ~TaskRunnerTimer() override = default;
-
- protected:
-  void Fired() override { (object_->*function_)(this); }
-
-  NO_SANITIZE_ADDRESS
-  bool CanFire() const override { return true; }
+  static bool IsHeapObjectAlive(T*) { return true; }
+};
 
- private:
-  TimerFiredClass* object_;
-  TimerFiredFunction function_;
+template <typename T>
+class TimerIsObjectAliveTrait<T, true> {
+ public:
+  static bool IsHeapObjectAlive(T* object_pointer) {
+    return !ThreadHeap::WillObjectBeLazilySwept(object_pointer);
+  }
 };
 
 template <typename TimerFiredClass>
-class TaskRunnerTimer<TimerFiredClass, true> : public TimerBase {
+class TaskRunnerTimer : public TimerBase {
  public:
   using TimerFiredFunction = void (TimerFiredClass::*)(TimerBase*);
 
@@ -155,11 +141,20 @@ class TaskRunnerTimer<TimerFiredClass, true> : public TimerBase {
   void Fired() override { (object_->*function_)(this); }
 
   NO_SANITIZE_ADDRESS
-  bool CanFire() const override { return object_.Get(); }
+  bool CanFire() const override {
+    // Oilpan: if a timer fires while Oilpan heaps are being lazily
+    // swept, it is not safe to proceed if the object is about to
+    // be swept (and this timer will be stopped while doing so.)
+    return TimerIsObjectAliveTrait<TimerFiredClass>::IsHeapObjectAlive(object_);
+  }
 
  private:
+  // FIXME: Oilpan: TimerBase should be moved to the heap and m_object should be
+  // traced.  This raw pointer is safe as long as Timer<X> is held by the X
+  // itself (That's the case
+  // in the current code base).
   GC_PLUGIN_IGNORE("363031")
-  WeakPersistent<TimerFiredClass> object_;
+  TimerFiredClass* object_;
   TimerFiredFunction function_;
 };
 

third_party/blink/renderer/platform/timer_test.cc

@@ -645,18 +645,7 @@ TEST_F(TimerTest, DestructOnHeapTimer) {
   EXPECT_FALSE(record->TimerHasFired());
 }
 
-// The following test assumes that an object may still be accessed after the
-// garabge collector found it unreachable until its destructor is called. This
-// is not possible in a purely managed environment and only necessary because
-// timers pass around raw pointers to managed objects. When running with ASAN
-// all unreachable objects are poisoned directly after marking, prohibiting any
-// access until the destructor is called.
-#if defined(ADDRESS_SANITIZER)
-#define MAYBE_MarkOnHeapTimerAsUnreachable DISABLED_MarkOnHeapTimerAsUnreachable
-#else
-#define MAYBE_MarkOnHeapTimerAsUnreachable MarkOnHeapTimerAsUnreachable
-#endif  // ADDRESS_SANITIZER
-TEST_F(TimerTest, MAYBE_MarkOnHeapTimerAsUnreachable) {
+TEST_F(TimerTest, MarkOnHeapTimerAsUnreachable) {
   scoped_refptr<OnHeapTimerOwner::Record> record =
       OnHeapTimerOwner::Record::Create();
   Persistent<OnHeapTimerOwner> owner =

third_party/blink/renderer/platform/wtf/type_traits.h

@@ -180,10 +180,12 @@ struct IsDisallowNew {
 };
 
 template <typename T>
-class IsGarbageCollectedTypeInternal {
+class IsGarbageCollectedType {
   typedef char YesType;
   typedef struct NoType { char padding[8]; } NoType;
 
+  static_assert(sizeof(T), "T must be fully defined");
+
   using NonConstType = typename std::remove_const<T>::type;
   template <typename U>
   static YesType CheckGarbageCollectedType(
@@ -215,11 +217,6 @@ class IsGarbageCollectedTypeInternal {
        sizeof(CheckGarbageCollectedMixinType<NonConstType>(nullptr)));
 };
 
-template <typename T>
-class IsGarbageCollectedType : public IsGarbageCollectedTypeInternal<T> {
-  static_assert(sizeof(T), "T must be fully defined");
-};
-
 template <>
 class IsGarbageCollectedType<void> {
  public: