Do not cast LifecycleObserver to its subclass before it's constructed

This CL fixes a CFI check failure on the Oilpan incremental marking.

The initialization of ContextLifecycleObserver is done as:
 1. Allocate sufficient memory for the instance
 2. Call GCM constructor
 3. Update the vptr to point LifecycleObserver's vtable
 4. Call the body of LifecycleObserver constructor.
 5. Update the vptr to point ContextLifecycleObserver's vtable
 6. Call the body of ContextLifecycleObserver constructor.

In the step 4, the constructor calls SetContext, which casts the
instance from LO to CLO and stores it to LifecycleNotifier, and that
causes Trace() or GetTraceDescriptor() call on CLO, rather than LO.
Note that the instance is ready as an LO instance, but not ready as an
CLO instance at the moment.
While a virtual method of CLO is being dispatched, CFI checks if the
vtable of the receiver is for CLO or its subclass, but that was not true
as the vptr points LO's vtable until the step 5.

After this CL, LO::Trace() or LO:GetTraceDescriptor() will be called
in SetContext(), rather than CLO::Trace() or CLO::GetTraceDescriptor(),
that should be ready to call before the step 5.

Bug: 854639
Change-Id: Ib27f147099ccb416d5f357fccd401d003de2dcaa
Reviewed-on: https://chromium-review.googlesource.com/1122140
Commit-Queue: Taiju Tsuiki <tzik@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572103}

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

@@ -34,7 +34,9 @@ namespace blink {
 
 void ContextLifecycleNotifier::NotifyResumingPausableObjects() {
   base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (ContextLifecycleObserver* observer : observers_) {
+  for (LifecycleObserverBase* observer_base : observers_) {
+    ContextLifecycleObserver* observer =
+        static_cast<ContextLifecycleObserver*>(observer_base);
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
       continue;
@@ -49,7 +51,9 @@ void ContextLifecycleNotifier::NotifyResumingPausableObjects() {
 
 void ContextLifecycleNotifier::NotifySuspendingPausableObjects() {
   base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (ContextLifecycleObserver* observer : observers_) {
+  for (LifecycleObserverBase* observer_base : observers_) {
+    ContextLifecycleObserver* observer =
+        static_cast<ContextLifecycleObserver*>(observer_base);
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
       continue;
@@ -65,7 +69,9 @@ void ContextLifecycleNotifier::NotifySuspendingPausableObjects() {
 unsigned ContextLifecycleNotifier::PausableObjectCount() const {
   DCHECK(!IsIteratingOverObservers());
   unsigned pausable_objects = 0;
-  for (ContextLifecycleObserver* observer : observers_) {
+  for (LifecycleObserverBase* observer_base : observers_) {
+    ContextLifecycleObserver* observer =
+        static_cast<ContextLifecycleObserver*>(observer_base);
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
       continue;
@@ -77,7 +83,9 @@ unsigned ContextLifecycleNotifier::PausableObjectCount() const {
 #if DCHECK_IS_ON()
 bool ContextLifecycleNotifier::Contains(PausableObject* object) const {
   DCHECK(!IsIteratingOverObservers());
-  for (ContextLifecycleObserver* observer : observers_) {
+  for (LifecycleObserverBase* observer_base : observers_) {
+    ContextLifecycleObserver* observer =
+        static_cast<ContextLifecycleObserver*>(observer_base);
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
       continue;

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

@@ -13,28 +13,40 @@ SynchronousMutationNotifier::SynchronousMutationNotifier() = default;
 
 void SynchronousMutationNotifier::NotifyChangeChildren(
     const ContainerNode& container) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->DidChangeChildren(container);
+  }
 }
 
 void SynchronousMutationNotifier::NotifyMergeTextNodes(
     const Text& node,
     const NodeWithIndex& node_to_be_removed_with_index,
     unsigned old_length) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->DidMergeTextNodes(node, node_to_be_removed_with_index,
                                 old_length);
+  }
 }
 
 void SynchronousMutationNotifier::NotifyMoveTreeToNewDocument(
     const Node& root) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->DidMoveTreeToNewDocument(root);
+  }
 }
 
 void SynchronousMutationNotifier::NotifySplitTextNode(const Text& node) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->DidSplitTextNode(node);
+  }
 }
 
 void SynchronousMutationNotifier::NotifyUpdateCharacterData(
@@ -42,7 +54,9 @@ void SynchronousMutationNotifier::NotifyUpdateCharacterData(
     unsigned offset,
     unsigned old_length,
     unsigned new_length) {
-  for (SynchronousMutationObserver* observer : observers_) {
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->DidUpdateCharacterData(character_data, offset, old_length,
                                      new_length);
   }
@@ -50,13 +64,19 @@ void SynchronousMutationNotifier::NotifyUpdateCharacterData(
 
 void SynchronousMutationNotifier::NotifyNodeChildrenWillBeRemoved(
     ContainerNode& container) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->NodeChildrenWillBeRemoved(container);
+  }
 }
 
 void SynchronousMutationNotifier::NotifyNodeWillBeRemoved(Node& node) {
-  for (SynchronousMutationObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    SynchronousMutationObserver* observer =
+        static_cast<SynchronousMutationObserver*>(observer_base);
     observer->NodeWillBeRemoved(node);
+  }
 }
 
 }  // namespace blink

third_party/blink/renderer/core/page/page_visibility_notifier.cc

@@ -32,8 +32,11 @@ namespace blink {
 
 void PageVisibilityNotifier::NotifyPageVisibilityChanged() {
   base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (PageVisibilityObserver* observer : observers_)
+  for (LifecycleObserverBase* observer_base : observers_) {
+    PageVisibilityObserver* observer =
+        static_cast<PageVisibilityObserver*>(observer_base);
     observer->PageVisibilityChanged();
+  }
 }
 
 }  // namespace blink

third_party/blink/renderer/platform/lifecycle_notifier.h

@@ -33,13 +33,15 @@
 
 namespace blink {
 
+class LifecycleObserverBase;
+
 template <typename T, typename Observer>
 class LifecycleNotifier : public GarbageCollectedMixin {
  public:
   virtual ~LifecycleNotifier();
 
-  void AddObserver(Observer*);
-  void RemoveObserver(Observer*);
+  void AddObserver(LifecycleObserverBase*);
+  void RemoveObserver(LifecycleObserverBase*);
 
   // notifyContextDestroyed() should be explicitly dispatched from an
   // observed context to detach its observers, and if the observer kind
@@ -61,7 +63,7 @@ class LifecycleNotifier : public GarbageCollectedMixin {
 
   T* Context() { return static_cast<T*>(this); }
 
-  using ObserverSet = HeapHashSet<WeakMember<Observer>>;
+  using ObserverSet = HeapHashSet<WeakMember<LifecycleObserverBase>>;
 
   enum IterationState {
     kAllowingNone = 0,
@@ -135,7 +137,8 @@ inline void LifecycleNotifier<T, Observer>::NotifyContextDestroyed() {
   base::AutoReset<IterationState> scope(&iteration_state_, kAllowingRemoval);
   ObserverSet observers;
   observers_.swap(observers);
-  for (Observer* observer : observers) {
+  for (LifecycleObserverBase* observer_base : observers) {
+    Observer* observer = static_cast<Observer*>(observer_base);
     DCHECK(observer->LifecycleContext() == Context());
     ContextDestroyedNotifier<Observer, T>::Call(observer, Context());
     observer->ClearContext();
@@ -143,13 +146,15 @@ inline void LifecycleNotifier<T, Observer>::NotifyContextDestroyed() {
 }
 
 template <typename T, typename Observer>
-inline void LifecycleNotifier<T, Observer>::AddObserver(Observer* observer) {
+inline void LifecycleNotifier<T, Observer>::AddObserver(
+    LifecycleObserverBase* observer) {
   CHECK(iteration_state_ & kAllowingAddition);
   observers_.insert(observer);
 }
 
 template <typename T, typename Observer>
-inline void LifecycleNotifier<T, Observer>::RemoveObserver(Observer* observer) {
+inline void LifecycleNotifier<T, Observer>::RemoveObserver(
+    LifecycleObserverBase* observer) {
   // If immediate removal isn't currently allowed,
   // |observer| is recorded for pending removal.
   if (iteration_state_ & kAllowPendingRemoval) {

third_party/blink/renderer/platform/lifecycle_observer.h

@@ -34,8 +34,10 @@ namespace blink {
 template <typename Context, typename Observer>
 class LifecycleNotifier;
 
+class LifecycleObserverBase : public GarbageCollectedMixin {};
+
 template <typename Context, typename Observer>
-class LifecycleObserver : public GarbageCollectedMixin {
+class LifecycleObserver : public LifecycleObserverBase {
  public:
   void Trace(blink::Visitor* visitor) override {
     visitor->Trace(lifecycle_context_);
@@ -64,15 +66,13 @@ inline void LifecycleObserver<Context, Observer>::SetContext(Context* context) {
     return;
 
   if (lifecycle_context_) {
-    static_cast<Notifier*>(lifecycle_context_)
-        ->RemoveObserver(static_cast<Observer*>(this));
+    static_cast<Notifier*>(lifecycle_context_)->RemoveObserver(this);
   }
 
   lifecycle_context_ = context;
 
   if (lifecycle_context_) {
-    static_cast<Notifier*>(lifecycle_context_)
-        ->AddObserver(static_cast<Observer*>(this));
+    static_cast<Notifier*>(lifecycle_context_)->AddObserver(this);
   }
 }