Oilpan: improve ScrollableArea handling.

Source/platform/scroll/Scrollbar.h

 /*
  * Copyright (C) 2004, 2006 Apple Computer, Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 28 matching lines @@
 
 class GraphicsContext;
 class IntRect;
 class PlatformGestureEvent;
 class PlatformMouseEvent;
 class ScrollAnimator;
 class ScrollableArea;
 class ScrollbarTheme;
 
 class PLATFORM_EXPORT Scrollbar : public Widget, public ScrollbarThemeClient {
-
 public:
     static PassRefPtrWillBeRawPtr<Scrollbar> create(ScrollableArea*, ScrollbarOrientation, ScrollbarControlSize);
 
     virtual ~Scrollbar();
 
     // ScrollbarThemeClient implementation.
     virtual int x() const override { return Widget::x(); }
     virtual int y() const override { return Widget::y(); }
     virtual int width() const override { return Widget::width(); }
     virtual int height() const override { return Widget::height(); }
@@ skipping 86 matching lines @@
 
     virtual float elasticOverscroll() const override { return m_elasticOverscroll; }
     virtual void setElasticOverscroll(float elasticOverscroll) override { m_elasticOverscroll = elasticOverscroll; }
 
     bool overlapsResizer() const { return m_overlapsResizer; }
     void setOverlapsResizer(bool overlapsResizer) { m_overlapsResizer = overlapsResizer; }
 
     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);
 
     void updateThumb();
     virtual void updateThumbPosition();
     virtual void updateThumbProportion();
 
     void autoscrollTimerFired(Timer<Scrollbar>*);
     void startTimerIfNeeded(double delay);
     void stopTimerIfNeeded();
     void autoscrollPressedPart(double delay);
     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;
     float m_dragOrigin;
 
     ScrollbarPart m_hoveredPart;
     ScrollbarPart m_pressedPart;
     int m_pressedPos;
     float m_scrollPos;
     bool m_draggingDocument;
@@ skipping 14 matching lines @@
     virtual bool isScrollbar() const override { return true; }
 
     float scrollableAreaCurrentPos() const;
 };
 
 DEFINE_TYPE_CASTS(Scrollbar, Widget, widget, widget->isScrollbar(), widget.isScrollbar());
 
 } // namespace blink
 
 #endif // Scrollbar_h