Add CHECK for Layout::Destroy() called inside a finalizer

LayoutObject::Destroy() should not be called inside a finalizer. This becomes dangerous when we migrate LayoutObje ct to Oilpan as finalizers are not allowed to access other on-heap objects.

This CL adds AllowDestroyingLayoutObjectInFinalizerScope to mark certain SVGImage() as exempt from this CHECK. This is OK as SVGImage is retaining the LayoutObject via a Persistent and so it is guaranteed to survive the current GC cycle.

Bug: 1030176
Change-Id: Ibdd0a29e7630747ea118d92d442776cf1bac8537
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2352294Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798217}

third_party/blink/renderer/core/layout/layout_object.cc

@@ -149,6 +149,18 @@ LayoutObject* FindAncestorByPredicate(const LayoutObject* descendant,
 
 }  // namespace
 
+static int g_allow_destroying_layout_object_in_finalizer = 0;
+
+AllowDestroyingLayoutObjectInFinalizerScope::
+    AllowDestroyingLayoutObjectInFinalizerScope() {
+  ++g_allow_destroying_layout_object_in_finalizer;
+}
+AllowDestroyingLayoutObjectInFinalizerScope::
+    ~AllowDestroyingLayoutObjectInFinalizerScope() {
+  CHECK_GT(g_allow_destroying_layout_object_in_finalizer, 0);
+  --g_allow_destroying_layout_object_in_finalizer;
+}
+
 #if DCHECK_IS_ON()
 
 LayoutObject::SetLayoutNeededForbiddenScope::SetLayoutNeededForbiddenScope(
@@ -3560,6 +3572,9 @@ void LayoutObject::DestroyAndCleanupAnonymousWrappers() {
 }
 
 void LayoutObject::Destroy() {
+  CHECK(g_allow_destroying_layout_object_in_finalizer ||
+        !ThreadState::Current()->InAtomicSweepingPause());
+
   // Mark as being destroyed to avoid trouble with merges in |RemoveChild()| and
   // other house keepings.
   bitfields_.SetBeingDestroyed(true);

third_party/blink/renderer/core/layout/layout_object.h

@@ -135,6 +135,18 @@ struct AnnotatedRegionValue {
 const int kShowTreeCharacterOffset = 39;
 #endif
 
+// Usually calling LayooutObject::Destroy() is banned. This scope can be used to
+// exclude certain functions like ~SVGImage() from this rule. This is allowed
+// when a Persistent is guaranteeing to keep the LayoutObject alive for that GC
+// cycle.
+class AllowDestroyingLayoutObjectInFinalizerScope {
+  STACK_ALLOCATED();
+
+ public:
+  AllowDestroyingLayoutObjectInFinalizerScope();
+  ~AllowDestroyingLayoutObjectInFinalizerScope();
+};
+
 // LayoutObject is the base class for all layout tree objects.
 //
 // LayoutObjects form a tree structure that is a close mapping of the DOM tree.

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

@@ -135,6 +135,8 @@ TEST_P(ValidationMessageOverlayDelegateTest,
   EXPECT_EQ(external_clock.CurrentTime(), internal_clock.CurrentTime());
 
   GetPage().SetValidationMessageClientForTesting(original_client);
+
+  static_cast<ValidationMessageClient*>(client)->WillBeDestroyed();
 }
 
 }  // namespace blink

third_party/blink/renderer/core/svg/graphics/svg_image.cc

@@ -168,6 +168,8 @@ SVGImage::SVGImage(ImageObserver* observer, bool is_multipart)
       has_pending_timeline_rewind_(false) {}
 
 SVGImage::~SVGImage() {
+  AllowDestroyingLayoutObjectInFinalizerScope scope;
+
   if (frame_client_)
     frame_client_->ClearImage();