Oilpan: improve ScrollableArea handling.

Source/platform/scroll/Scrollbar.h

Index: Source/platform/scroll/Scrollbar.h
diff --git a/Source/platform/scroll/Scrollbar.h b/Source/platform/scroll/Scrollbar.h
index 20979ff328b90fd6980364552842af5700a93e25..fd8224d8f90f25954a1c09bf5867f4ee5aad9e32 100644
--- a/Source/platform/scroll/Scrollbar.h
+++ b/Source/platform/scroll/Scrollbar.h
@@ -46,7 +46,6 @@ class ScrollableArea;
 class ScrollbarTheme;
 
 class PLATFORM_EXPORT Scrollbar : public Widget, public ScrollbarThemeClient {
-
 public:
     static PassRefPtrWillBeRawPtr<Scrollbar> create(ScrollableArea*, ScrollbarOrientation, ScrollbarControlSize);
 
@@ -153,6 +152,12 @@ public:
     virtual DisplayItemClient displayItemClient() const override { return toDisplayItemClient(this); }
     virtual String debugName() const override { return m_orientation == HorizontalScrollbar ? "HorizontalScrollbar" : "VerticalScrollbar"; }
 
+    EAGERLY_FINALIZE();

[haraken, 2015/06/29 15:20:22]

Would you elaborate on why this needs to be eagerly finalized?

[sof, 2015/06/29 15:59:42]

Unregistration from the ScrollbarTheme 'manager', as that is used by the Mac
animator implementation to look its 'painter delegate'; unregistering it heads
that off.

I'm not fully convinced it is required, but I am fully convinced that the object
structures here are very involved and the lifetimes finely tuned, hence adding
lazy sweeping to the mix makes me scared :)

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

ok, let's add some comment anyway :)

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

Done.

+#if ENABLE(OILPAN)
+    DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();

[haraken, 2015/06/29 15:20:22]

I'd prefer using pre-finalizers though. It wouldn't require this kind of macro
and would have less risk when combined with lazy sweeping.

[sof, 2015/06/29 15:59:42]

I don't understand that argument about risk; could you elaborate?

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

A pre-finalizer is allowed to touch any other on-heap object. A destructor of an
eager-swept object is allowed to touch only on-heap objects that are in
lazy-swept heaps. In that sense, a pre-finalizer has less risk of touching
objects that shouldn't be touched.

Here are my thoughts:

- It's not nice to mix pre-finalizers and eager-sweeping in the code base. We
should minimize the oilpan programming tax.

- Pre-finalizers provide a programming semantics that is easier to understand
(i.e., you can touch any on-heap object in pre-finalizers). Also pre-finalizers
don't require magical macros like DECLARE_EAGER_FINALIZATION_OPERATOR_NEW. So
I'd prefer replacing eager-sweeping with pre-finalizers, overall.

- For the above reason, pre-finalizers have less risk to cause issues associated
with destruction ordering. So I'd prefer using pre-finalizers for mixin objects
(which is likely to cause destruction ordering issues), in particular.

- However, I'm not yet fully convinced that the pre-finalizers will solve all
problems associated with lazy sweeping. Thus at the moment, I'm not planning to
propose replacing existing eager-sweeping with pre-finalizers.

In this particular case, using eager-sweeping adds
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW(), so I'd prefer using pre-finalizers.

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

Thanks for spelling those out; some comments..
This "not nice" meta argument isn't very scientific :) But eager sweeping has
the lowest programming tax, i'd argue.
Pre-finalizers do not have an easier-to-understand semantics. The prefinalizers
have no ordering between one for a mixin instance and the one for the object
implementing it; should they both have prefinalizers. Nor do they have any ASan
checking in place that a prefinalizer doesn't touch another object being
prefinalized.
See above, I don't think there is a winning risk argument for one over the
other.

In the case of Scrollbar, its destructor doesn't touch any heap objects (but the
derived LayoutScrollbar gets close to doing so.)

+#endif
+    DECLARE_VIRTUAL_TRACE();
+
 protected:
     Scrollbar(ScrollableArea*, ScrollbarOrientation, ScrollbarControlSize, ScrollbarTheme* = 0);
 
@@ -167,18 +172,11 @@ protected:
     ScrollDirectionPhysical pressedPartScrollDirectionPhysical();
     ScrollGranularity pressedPartScrollGranularity();
 
-    ScrollableArea* m_scrollableArea;
+    RawPtrWillBeMember<ScrollableArea> m_scrollableArea;
     ScrollbarOrientation m_orientation;
     ScrollbarControlSize m_controlSize;
     ScrollbarTheme* m_theme;

[haraken, 2015/06/29 15:20:22]

This raw pointer looks safe because the ScrollbarTheme is a singleton.

 
-#if ENABLE(OILPAN)
-    // To simplify Oilpan finalization, keep a copy of the ScrollableArea's
-    // scroll animator. Scrollbar is responsible for notifying the animator
-    // when it is destructed.
-    RefPtr<ScrollAnimator> m_animator;
-#endif
-
     int m_visibleSize;
     int m_totalSize;
     float m_currentPos;