Oilpan: improve ScrollableArea handling.

Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp

Index: Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
diff --git a/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp b/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
index bb3d55ce33746ecf121ab814f27c981cc85ba63f..efd7abba32930d53bc37bbb1fbd6ad93efa8aa23 100644
--- a/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
+++ b/Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp
@@ -90,6 +90,9 @@ DeprecatedPaintLayerScrollableArea::DeprecatedPaintLayerScrollableArea(Deprecate
     , m_needsCompositedScrolling(false)
     , m_scrollCorner(nullptr)
     , m_resizer(nullptr)
+#if ENABLE(ASSERT)
+    , m_hasBeenDisposed(false)
+#endif
 {
     Node* node = box().node();
     if (node && node->isElementNode()) {
@@ -106,6 +109,11 @@ DeprecatedPaintLayerScrollableArea::DeprecatedPaintLayerScrollableArea(Deprecate
 
 DeprecatedPaintLayerScrollableArea::~DeprecatedPaintLayerScrollableArea()
 {
+    ASSERT(m_hasBeenDisposed);
+}
+
+void DeprecatedPaintLayerScrollableArea::dispose()
+{
     if (inResizeMode() && !box().documentBeingDestroyed()) {
         if (LocalFrame* frame = box().frame())
             frame->eventHandler().resizeScrollableAreaDestroyed();
@@ -142,6 +150,19 @@ DeprecatedPaintLayerScrollableArea::~DeprecatedPaintLayerScrollableArea()
         m_scrollCorner->destroy();
     if (m_resizer)
         m_resizer->destroy();
+
+    clearScrollAnimators();

[haraken, 2015/06/30 10:30:35]

Do we need to call clearScrollAnimators() in dispose()? I guess the
ScrollAnimator is cleared in ScrollableArea's destructor (which is eagerly
destructed) and that is sufficient.

If we need to explicitly call clearScrollAnimators() for
DeprecatedPaintLayerScrollableArea, I wonder why we don't need to explicitly
call clearScrollAnimators() for ScrollableArea.

[sof, 2015/06/30 11:34:09]

This is what https://code.google.com/p/chromium/issues/detail?id=504655#c13 is
all about.

DeprecatedPaintLayer is _not_ on the Oilpan heap and it explicitly calls
dispose() on DeprecatedPaintLayerScrollableArea when letting go of it.

[haraken, 2015/06/30 14:14:24]

My question is whether we need to call clearScrollAnimators() in the dispose().
Would it be a problem if we delay m_animator.clear() until the
DeprecatedPaintLayerScrollableArea gets collected (in the eager sweeping phase)?

I'm asking this because ScrollableArea delays m_animator.clear() until the
ScrollableArea gets collected (in the eager sweeping phase).

[sof, 2015/06/30 14:28:03]

It is a problem to delay its clearing until it is eagerly swept after the next
GC, subtly so.

When this scrollable area is dispose()d other layout objects are destructed,
including its layout box. The animator may try to access that layout object (see
https://code.google.com/p/chromium/issues/detail?id=504655#c12) when a timer
fires.

[haraken, 2015/06/30 14:30:24]

Makes sense. Why doesn't it become a problem for ScrollableArea?

[sof, 2015/06/30 14:35:48]

It takes care of its owned objects via eager finalization along with the
explicit willRemove*() detach protocol over Scrollbars, having them detach from
the scrollable area's scroll animators.

[haraken, 2015/06/30 14:37:21]

ah, understood; thanks!

+
+#if ENABLE(ASSERT)
+    m_hasBeenDisposed = true;
+#endif
+}
+
+DEFINE_TRACE(DeprecatedPaintLayerScrollableArea)
+{
+    visitor->trace(m_hBar);
+    visitor->trace(m_vBar);
+    ScrollableArea::trace(visitor);
 }
 
 HostWindow* DeprecatedPaintLayerScrollableArea::hostWindow() const
@@ -935,7 +956,7 @@ PassRefPtrWillBeRawPtr<Scrollbar> DeprecatedPaintLayerScrollableArea::createScro
 
 void DeprecatedPaintLayerScrollableArea::destroyScrollbar(ScrollbarOrientation orientation)
 {
-    RefPtrWillBePersistent<Scrollbar>& scrollbar = orientation == HorizontalScrollbar ? m_hBar : m_vBar;
+    RefPtrWillBeMember<Scrollbar>& scrollbar = orientation == HorizontalScrollbar ? m_hBar : m_vBar;
     if (!scrollbar)
         return;