Oilpan: improve ScrollableArea handling.

Oilpan: improve ScrollableArea handling.

Have all ScrollableArea objects reside on the Oilpan heap,
making ScrollableArea be a GC mixin.

Without it, some ScrollableAreas (FrameView) were on the heap
whereas others (*Viewport) weren't, making for unpredictable
and incorrect finalization as the lifetimes were skewed.

Additionally, simplify Scrollbar's unregistration with its
ScrollableArea (and its ScrollAnimator). The Oilpan-specific
RefPtr<ScrollAnimator> that Scrollbar kept, no longer served
a purpose, hence removed. As a result, ScrollAnimator can again
be fully owned by ScrollableAreas & no longer be RefCounted<>.

R=haraken
BUG=504655
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198064

[sof, 2015-06-29 11:56:14]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-06-29 11:56:15]

please take a look.

need to understand/resolve the Mac unit test failures for root layer scrolling,
but otherwise ready.

[haraken, 2015-06-29 15:20:20]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

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

Thanks for working on this! Here is the first round of comments.

https://codereview.chromium.org/1215973002/diff/20001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.h:157: HashSet<const
DeprecatedPaintLayer*> m_layersWithTouchRects;

This raw pointer looks safe since it's removed in ~DeprecatedPaintLayer.

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:152:
DeprecatedPaintLayer::~DeprecatedPaintLayer()

DeprecatedPaintLayer is explicitly destructed in
LayoutBoxModelObject::destroyLayer. I'd prefer introducing
DeprecatedPaintLayer::dispose and move the contents of ~DeprecatedPaintLayer()
into the dispose method. It will make it clearer that we're not relying on
destructors.

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:1450: m_scrollableArea = nullptr;

Don't we need to call m_scrollableArea->dispose() here?

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimator.h:107: ScrollableArea* m_scrollableArea;

Maybe can we move ScrollAnimator to the heap and use a Member for this?

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.h:303: // Need to promptly let go of owned
animator objects.

I don't fully understand this -- why do we need to promptly destruct
ScrollAnimator?

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (left):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:104:
m_animator->willRemoveHorizontalScrollbar(this);

Help me understand: Who calls these willRemoveScrollbar after this CL?

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:85: Scrollbar::~Scrollbar()

Can we add ASSERT(!m_scrollableArea) to verify that
disconnectFromScrollableArea() has been called?

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:144: void
Scrollbar::disconnectFromScrollableArea()

In oilpan, do we need to call Scrollbar::disconnectFromScrollableArea()? It
seems that it is just clearing a back pointer. In oilpan, the clear wouldn't be
needed since the back pointer is a Member.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:155: EAGERLY_FINALIZE();

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

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:157:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();

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.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:178: ScrollbarTheme* m_theme;

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

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

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:152:
DeprecatedPaintLayer::~DeprecatedPaintLayer()
On 2015/06/29 15:20:21, haraken wrote:
> 
> DeprecatedPaintLayer is explicitly destructed in
> LayoutBoxModelObject::destroyLayer. I'd prefer introducing
> DeprecatedPaintLayer::dispose and move the contents of ~DeprecatedPaintLayer()
> into the dispose method. It will make it clearer that we're not relying on
> destructors.
> 

How is that related to Oilpan? DeprecatedPaintLayer is not on the heap.

https://codereview.chromium.org/1215973002/diff/20001/Source/core/paint/Depre...
Source/core/paint/DeprecatedPaintLayer.cpp:1450: m_scrollableArea = nullptr;
On 2015/06/29 15:20:21, haraken wrote:
> 
> Don't we need to call m_scrollableArea->dispose() here?

I'm not familiar with the naming for this helper, but it's only called from the
constructor.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ScrollAnimator.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ScrollAnimator.h:107: ScrollableArea* m_scrollableArea;
On 2015/06/29 15:20:22, haraken wrote:
> 
> Maybe can we move ScrollAnimator to the heap and use a Member for this?

Keeping "platform near" objects like ScrollAnimatorMac on the heap & managing
that is the problem.

I'm not pushing for that here - there's enough complexity as it is.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.h:303: // Need to promptly let go of owned
animator objects.
On 2015/06/29 15:20:22, haraken wrote:
> 
> I don't fully understand this -- why do we need to promptly destruct
> ScrollAnimator?

Look at ScrollAnimatorMac and the back references it keeps to ScrollableArea.
And timers, which make them susceptible to lazy sweeping instability.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (left):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:104:
m_animator->willRemoveHorizontalScrollbar(this);
On 2015/06/29 15:20:22, haraken wrote:
> 
> Help me understand: Who calls these willRemoveScrollbar after this CL?

The ScrollableAreas will now enforce this, as they did previously really. This
code actually had no effect for the problematic Mac scroll animators -- the
theme unregistration caused their implementations of willRemove*() to leave
early & making them be no-ops, in effect.

The RefPtr<> may have served a purpose initially with render objects and frames
being on/off-heap; unclear. It had no purpose now, I realized.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:85: Scrollbar::~Scrollbar()
On 2015/06/29 15:20:22, haraken wrote:
> 
> Can we add ASSERT(!m_scrollableArea) to verify that
> disconnectFromScrollableArea() has been called?

It doesn't hold in all cases, and I didn't see the value of enforcing it, so I
didn't in the end. Look at the WIP CL for versions that did.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:144: void
Scrollbar::disconnectFromScrollableArea()
On 2015/06/29 15:20:22, haraken wrote:
> 
> In oilpan, do we need to call Scrollbar::disconnectFromScrollableArea()? It
> seems that it is just clearing a back pointer. In oilpan, the clear wouldn't
be
> needed since the back pointer is a Member.
> 

yes, I think we can get rid of this step once we switch over.

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:155: EAGERLY_FINALIZE();
On 2015/06/29 15:20:22, haraken wrote:
> 
> Would you elaborate on why this needs to be eagerly finalized?

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 :)

https://codereview.chromium.org/1215973002/diff/20001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:157:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2015/06/29 15:20:22, haraken wrote:
> 
> 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.
> 

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

[sof, 2015-06-30 07:46:50]

http://build.chromium.org/p/tryserver.blink/builders/mac_blink_oilpan_dbg/bui...
has the all-green Mac Oilpan build.

[haraken, 2015-06-30 10:30:36]

Mostly looks good. A final round of comments...

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.cpp:85: Scrollbar::~Scrollbar()
On 2015/06/29 15:59:42, sof wrote:
> On 2015/06/29 15:20:22, haraken wrote:
> > 
> > Can we add ASSERT(!m_scrollableArea) to verify that
> > disconnectFromScrollableArea() has been called?
> 
> It doesn't hold in all cases, and I didn't see the value of enforcing it, so I
> didn't in the end. Look at the WIP CL for versions that did.

It doesn't hold in non-oilpan or/and oilpan? In oilpan, it's ok it doesn't hold
because disconnectFromScrollableArea() is just clearing the back pointer (which
is a Member). In non-oilpan, it's not ok if it doesn't hold -- it would imply
m_scrollableArea can be a stale pointer.

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
File Source/platform/scroll/Scrollbar.h (right):

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.h:155: EAGERLY_FINALIZE();
On 2015/06/29 15:59:42, sof wrote:
> On 2015/06/29 15:20:22, haraken wrote:
> > 
> > Would you elaborate on why this needs to be eagerly finalized?
> 
> 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 :)

ok, let's add some comment anyway :)

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.h:157:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2015/06/29 15:59:42, sof wrote:
> On 2015/06/29 15:20:22, haraken wrote:
> > 
> > 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.
> > 
> 
> I don't understand that argument about risk; could you elaborate?

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.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/fram...
File Source/core/frame/FrameView.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/fram...
Source/core/frame/FrameView.cpp:3068:
m_horizontalScrollbar->disconnectFromScrollableArea();

Shall we add #if !ENABLE(OILPAN)?

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/fram...
Source/core/frame/FrameView.cpp:3097:
m_verticalScrollbar->disconnectFromScrollableArea();

#if !ENABLE(OILPAN)

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/page...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/page...
Source/core/page/scrolling/ScrollingCoordinator.cpp:99: {

Not mandatory, but it would be a good idea to add ASSERT(!m_page) to verify that
willBeDestroyed has been called.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayer.cpp:1450: m_scrollableArea = nullptr;

To avoid confusion, I'd move m_scrollableArea(nullptr) to the constructor and
add ASSERT(!m_scrollableArea) at the head of updateScrollableArea().

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();

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.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:967:
scrollbar->disconnectFromScrollableArea();

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

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/platform/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/platform/...
Source/platform/scroll/ScrollableArea.cpp:56: VerifyEagerFinalization
verifyEager;

Nit: This could be removed if we use a pre-finalizer...

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/platform/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/platform/...
Source/platform/scroll/Scrollbar.cpp:144: void
Scrollbar::disconnectFromScrollableArea()

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

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

Thanks for the review; it doesn't help that the scroll abstractions straddle
Oilpan and non-Oilpan worlds, but I think it looks reasonably tidy now.

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.cpp:85: Scrollbar::~Scrollbar()
On 2015/06/30 10:30:35, haraken wrote:
> On 2015/06/29 15:59:42, sof wrote:
> > On 2015/06/29 15:20:22, haraken wrote:
> > > 
> > > Can we add ASSERT(!m_scrollableArea) to verify that
> > > disconnectFromScrollableArea() has been called?
> > 
> > It doesn't hold in all cases, and I didn't see the value of enforcing it, so
I
> > didn't in the end. Look at the WIP CL for versions that did.
> 
> It doesn't hold in non-oilpan or/and oilpan? In oilpan, it's ok it doesn't
hold
> because disconnectFromScrollableArea() is just clearing the back pointer
(which
> is a Member). In non-oilpan, it's not ok if it doesn't hold -- it would imply
> m_scrollableArea can be a stale pointer.

It holds in neither setting; with non-Oilpan, the cases where it doesn't the
lifetimes of the scrollbars are such that isn't an issue.

https://codereview.chromium.org/1209293004/#ps260001 did this & had the assert.
cf. FrameViewAutoSize and WebFrameTest changes, in particular. i.e., it looked
artificial to have the explicit disconnects when dealing with local RefPtr<>s.

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.cpp:146: m_scrollableArea = nullptr;
Made clearing conditional on !ENABLE(OILPAN) + added a TODO.

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
File Source/platform/scroll/Scrollbar.h (right):

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.h:155: EAGERLY_FINALIZE();
On 2015/06/30 10:30:35, haraken wrote:
> On 2015/06/29 15:59:42, sof wrote:
> > On 2015/06/29 15:20:22, haraken wrote:
> > > 
> > > Would you elaborate on why this needs to be eagerly finalized?
> > 
> > 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 :)
> 
> ok, let's add some comment anyway :)

Done.

https://chromiumcodereview.appspot.com/1215973002/diff/20001/Source/platform/...
Source/platform/scroll/Scrollbar.h:157:
DECLARE_EAGER_FINALIZATION_OPERATOR_NEW();
On 2015/06/30 10:30:35, haraken wrote:
> On 2015/06/29 15:59:42, sof wrote:
> > On 2015/06/29 15:20:22, haraken wrote:
> > > 
> > > 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.
> > > 
> > 
> > I don't understand that argument about risk; could you elaborate?
> 
> 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:
> 

Thanks for spelling those out; some comments..


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

This "not nice" meta argument isn't very scientific :) But eager sweeping has
the lowest programming tax, i'd argue.

> - 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.
> 

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.

> - 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.
> 

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.)

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/fram...
File Source/core/frame/FrameView.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/fram...
Source/core/frame/FrameView.cpp:3068:
m_horizontalScrollbar->disconnectFromScrollableArea();
On 2015/06/30 10:30:35, haraken wrote:
> 
> Shall we add #if !ENABLE(OILPAN)?

No, I don't think it is worth adding the CPPery at the call sites at this stage.
We can remove the method once transitioned, so let's add a TODO there instead.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/page...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/page...
Source/core/page/scrolling/ScrollingCoordinator.cpp:99: {
On 2015/06/30 10:30:35, haraken wrote:
> 
> Not mandatory, but it would be a good idea to add ASSERT(!m_page) to verify
that
> willBeDestroyed has been called.

Added

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayer.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayer.cpp:1450: m_scrollableArea = nullptr;
On 2015/06/30 10:30:35, haraken wrote:
> 
> To avoid confusion, I'd move m_scrollableArea(nullptr) to the constructor and
> add ASSERT(!m_scrollableArea) at the head of updateScrollableArea().

Added assert to this and another update*() method so as have them follow the
same pattern.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 10:30:35, haraken wrote:
> 
> 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.
> 

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]

LGTM

Regarding pre-finalizers vs eager-sweeping, let's discuss in a separate thread.
At the very least, it's scientifically not nice to mix them without having a
clear distinction, and we need to figure it out and minimize tax in the oilpan
programming :)

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 11:34:09, sof wrote:
> On 2015/06/30 10:30:35, haraken wrote:
> > 
> > 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.
> > 
> 
> 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.

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]

Thanks, let's try to arrive at a conclusion on that twisty topic separately.

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 14:14:24, haraken wrote:
> On 2015/06/30 11:34:09, sof wrote:
> > On 2015/06/30 10:30:35, haraken wrote:
> > > 
> > > 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.
> > > 
> > 
> > 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.
> 
> 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).

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]

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 14:28:03, sof wrote:
> On 2015/06/30 14:14:24, haraken wrote:
> > On 2015/06/30 11:34:09, sof wrote:
> > > On 2015/06/30 10:30:35, haraken wrote:
> > > > 
> > > > 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.
> > > > 
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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

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

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 14:30:24, haraken wrote:
> On 2015/06/30 14:28:03, sof wrote:
> > On 2015/06/30 14:14:24, haraken wrote:
> > > On 2015/06/30 11:34:09, sof wrote:
> > > > On 2015/06/30 10:30:35, haraken wrote:
> > > > > 
> > > > > 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.
> > > > > 
> > > > 
> > > > 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.
> > > 
> > > 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).
> > 
> > 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.
> 
> Makes sense. Why doesn't it become a problem for ScrollableArea?
> 

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]

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
File Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp (right):

https://chromiumcodereview.appspot.com/1215973002/diff/40001/Source/core/pain...
Source/core/paint/DeprecatedPaintLayerScrollableArea.cpp:154:
clearScrollAnimators();
On 2015/06/30 14:35:48, sof wrote:
> On 2015/06/30 14:30:24, haraken wrote:
> > On 2015/06/30 14:28:03, sof wrote:
> > > On 2015/06/30 14:14:24, haraken wrote:
> > > > On 2015/06/30 11:34:09, sof wrote:
> > > > > On 2015/06/30 10:30:35, haraken wrote:
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > > 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).
> > > 
> > > 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.
> > 
> > Makes sense. Why doesn't it become a problem for ScrollableArea?
> > 
> 
> 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.

ah, understood; thanks!

[sof, 2015-06-30 14:41:02]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-06-30 14:41:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215973002/60001

[commit-bot: I haz the power, 2015-06-30 14:45:22]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198064