Oilpan: improve ScrollableArea handling.

Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011, 2012 Apple Inc. All rights reserved.
  *
  * Portions are Copyright (C) 1998 Netscape Communications Corporation.
  *
  * Other contributors:
  *   Robert O'Callahan <roc+@cs.cmu.edu>
  *   David Baron <dbaron@fas.harvard.edu>
  *   Christian Biesinger <cbiesinger@web.de>
  *   Randall Jesup <rjesup@wgate.com>
@@ skipping 72 matching lines @@
     : m_layer(layer)
     , m_inResizeMode(false)
     , m_scrollsOverflow(false)
     , m_scrollDimensionsDirty(true)
     , m_inOverflowRelayout(false)
     , m_nextTopmostScrollChild(0)
     , m_topmostScrollChild(0)
     , m_needsCompositedScrolling(false)
     , m_scrollCorner(nullptr)
     , m_resizer(nullptr)
+#if ENABLE(ASSERT)
+    , m_hasBeenDisposed(false)
+#endif
 {
     Node* node = box().node();
     if (node && node->isElementNode()) {
         // We save and restore only the scrollOffset as the other scroll values are recalculated.
         Element* element = toElement(node);
         m_scrollOffset = element->savedLayerScrollOffset();
         if (!m_scrollOffset.isZero())
             scrollAnimator()->setCurrentPosition(FloatPoint(m_scrollOffset.width(), m_scrollOffset.height()));
         element->setSavedLayerScrollOffset(IntSize());
     }
 
     updateResizerAreaSet();
 }
 
 DeprecatedPaintLayerScrollableArea::~DeprecatedPaintLayerScrollableArea()
 {
+    ASSERT(m_hasBeenDisposed);
+}
+
+void DeprecatedPaintLayerScrollableArea::dispose()
+{
     if (inResizeMode() && !box().documentBeingDestroyed()) {
         if (LocalFrame* frame = box().frame())
             frame->eventHandler().resizeScrollableAreaDestroyed();
     }
 
     if (LocalFrame* frame = box().frame()) {
         if (FrameView* frameView = frame->view()) {
             frameView->removeScrollableArea(this);
             frameView->removeAnimatingScrollableArea(this);
         }
@@ skipping 16 matching lines @@
             frameView->removeResizerArea(box());
     }
 
     destroyScrollbar(HorizontalScrollbar);
     destroyScrollbar(VerticalScrollbar);
 
     if (m_scrollCorner)
         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
 {
     if (Page* page = box().frame()->page())
         return &page->chromeClient();
     return nullptr;
 }
 
 GraphicsLayer* DeprecatedPaintLayerScrollableArea::layerForScrolling() const
@@ skipping 773 matching lines @@
             didAddScrollbar(widget.get(), HorizontalScrollbar);
         else
             didAddScrollbar(widget.get(), VerticalScrollbar);
     }
     box().document().view()->addChild(widget.get());
     return widget.release();
 }
 
 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;
 
     if (!scrollbar->isCustomScrollbar())
         willRemoveScrollbar(scrollbar.get(), orientation);
 
     toFrameView(scrollbar->parent())->removeChild(scrollbar.get());
     scrollbar->disconnectFromScrollableArea();

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

I'd add #if !ENABLE(OILPAN).

     scrollbar = nullptr;
 }
 
 void DeprecatedPaintLayerScrollableArea::setHasHorizontalScrollbar(bool hasScrollbar)
 {
     if (hasScrollbar == hasHorizontalScrollbar())
         return;
 
     if (hasScrollbar) {
         // This doesn't hit in any tests, but since the equivalent code in setHasVerticalScrollbar
@@ skipping 436 matching lines @@
 void DeprecatedPaintLayerScrollableArea::setTopmostScrollChild(DeprecatedPaintLayer* scrollChild)
 {
     // We only want to track the topmost scroll child for scrollable areas with
     // overlay scrollbars.
     if (!hasOverlayScrollbars())
         return;
     m_nextTopmostScrollChild = scrollChild;
 }
 
 } // namespace blink