Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1477113003:
[Oilpan] Move ScrollAnimators onto oilpan heap (Closed)
|
Created:
5 years ago by peria Modified:
5 years ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, darktears, blink-reviews, Eric Willigers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap.
Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea.
BUG=550325
Committed: https://crrev.com/8e25ffb821fc66cbca271b2ca5b7b1908811f977
Cr-Commit-Position: refs/heads/master@{#363117}
Patch Set 1 : #Patch Set 2 : Move ProgrammaticScrollAnimator #
Total comments: 4
Patch Set 3 : Remove ENABLE(OILPAN) #Patch Set 4 : Fix compile on Mac #Patch Set 5 : Call trace of superclass #Patch Set 6 : Stop timers on Mac #Patch Set 7 : Rename #
Total comments: 2
Patch Set 8 : Do not touch on-heap objects in destructors #
Total comments: 10
Patch Set 9 : #Patch Set 10 : #
Total comments: 2
Patch Set 11 : Make AnimatorBase NoBaseWillBeGarbageCollectedFinalized #
Total comments: 6
Patch Set 12 : #
Total comments: 2
Patch Set 13 : rename #Messages
Total messages: 75 (35 generated)
Description was changed from ========== Move ScrollAnimatorBase on heap BUG=550325 ========== to ========== Move ScrollAnimatorBase onto oilpan heap. Before this CL, the relationship around ScrollableArea is ScrollableArea =(OwnPtr)=> ScrollAnimators ScrollAnimators =(OwnPtr)=> ScrollAnimatorBase ScrollAnimators =(OwnPtr)=> ProgrammaticScrollAnimator ScrollAnimatorBase -(raw pointer)-> ScrollableArea ProgrammaticScrollAnimator -(raw pointer) and after this CL, it will be ScrollableArea =(PersistentWillBeMember)=> ScrollAnimatorBase ScrollAnimatorBase =(RawPtrWillBeMember)=> ScrollableArea ScrollableArea =(OwnPtr)=> ProgrammaticScrollAnimator ProgrammaticScrollAnimator -(RawPtrWillBeWeakPersistent)-> ScrollableArea BUG=550325 ==========
Description was changed from ========== Move ScrollAnimatorBase onto oilpan heap. Before this CL, the relationship around ScrollableArea is ScrollableArea =(OwnPtr)=> ScrollAnimators ScrollAnimators =(OwnPtr)=> ScrollAnimatorBase ScrollAnimators =(OwnPtr)=> ProgrammaticScrollAnimator ScrollAnimatorBase -(raw pointer)-> ScrollableArea ProgrammaticScrollAnimator -(raw pointer) and after this CL, it will be ScrollableArea =(PersistentWillBeMember)=> ScrollAnimatorBase ScrollAnimatorBase =(RawPtrWillBeMember)=> ScrollableArea ScrollableArea =(OwnPtr)=> ProgrammaticScrollAnimator ProgrammaticScrollAnimator -(RawPtrWillBeWeakPersistent)-> ScrollableArea BUG=550325 ========== to ========== Move ScrollAnimatorBase onto oilpan heap. Beside it, this CL removes an internal struct ScrollAnimators to simplify the code. BUG=550325 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Move ScrollAnimatorBase onto oilpan heap. Beside it, this CL removes an internal struct ScrollAnimators to simplify the code. BUG=550325 ========== to ========== Move ScrollAnimatorBase onto oilpan heap. In this CL, I did following changes. - Move ScrollAnimatorBase to Oilpan heap - Remove a redundant struct ScrollableAreaAnimators - Replace a raw pointer in ProgrammaticAnimator to ScrollableArea with RawPtrWillBeWeakPersistent BUG=550325 ==========
Patchset #1 (id:20001) has been deleted
peria@chromium.org changed reviewers: + oilpan-reviews@chromium.org
PTL. This CL is a replacement of https://codereview.chromium.org/1469353011/
On 2015/11/27 02:13:06, peria wrote: > PTL. > > This CL is a replacement of https://codereview.chromium.org/1469353011/ Would it be possible to move ProgrammaticScrollAnimator to the heap at the same time? At a first glance, it wouldn't be that hard.
Description was changed from ========== Move ScrollAnimatorBase onto oilpan heap. In this CL, I did following changes. - Move ScrollAnimatorBase to Oilpan heap - Remove a redundant struct ScrollableAreaAnimators - Replace a raw pointer in ProgrammaticAnimator to ScrollableArea with RawPtrWillBeWeakPersistent BUG=550325 ========== to ========== Move ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap. Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea. BUG=550325 ==========
Patchset #2 (id:60001) has been deleted
On 2015/11/27 03:53:54, haraken wrote: > On 2015/11/27 02:13:06, peria wrote: > > PTL. > > > > This CL is a replacement of https://codereview.chromium.org/1469353011/ > > Would it be possible to move ProgrammaticScrollAnimator to the heap at the same > time? At a first glance, it wouldn't be that hard. Yes, it is! PTAL.
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:45: m_compositorPlayer->setAnimationDelegate(nullptr); Not related to your CL, this is not nice... This setAnimationDelegate creates a new unnecessary object in content/ (i.e., WebToCCAnimationDelegateAdapter) that just holds the nullptr... https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.cpp (right): https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.cpp:100: #if ENABLE(OILPAN) You won't need #if ENABLE(OILPAN).
https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right): https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.cpp:45: m_compositorPlayer->setAnimationDelegate(nullptr); On 2015/11/27 05:10:14, haraken wrote: > > Not related to your CL, this is not nice... This setAnimationDelegate creates a > new unnecessary object in content/ (i.e., WebToCCAnimationDelegateAdapter) that > just holds the nullptr... Acknowledged. https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.cpp (right): https://codereview.chromium.org/1477113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.cpp:100: #if ENABLE(OILPAN) On 2015/11/27 05:10:14, haraken wrote: > > You won't need #if ENABLE(OILPAN). Done.
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1477113003/#ps100001 (title: "Remove ENABLE(OILPAN)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1477113003/#ps130001 (title: "Call trace of superclass")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/130001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #7 (id:170001) has been deleted
Patchset #7 (id:190001) has been deleted
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:150001) has been deleted
Patchset #6 (id:210001) has been deleted
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/250001
PTAL. The reason of the failure seems to be Timer<> in ScrollAnimatorMac.
https://codereview.chromium.org/1477113003/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:93: m_scrollAnimator->cleanup(); m_scrollAnimator is an on-heap object, so you cannot touch it in a destructor. Maybe would it be better to add a pre-finalizer to ScrollAnimatorBase and call cleanup()?
https://codereview.chromium.org/1477113003/diff/250001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/250001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:93: m_scrollAnimator->cleanup(); On 2015/12/03 02:45:30, haraken wrote: > > m_scrollAnimator is an on-heap object, so you cannot touch it in a destructor. > > Maybe would it be better to add a pre-finalizer to ScrollAnimatorBase and call > cleanup()? Thanks. Yes! ScrollAnimatorMac::cleanup() must be called before releasing ScrollableArea. Added pre-finalizer in ScrollAnimatorMac.
https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:708: cleanup(); Remove this. cleanup() is already called in the pre-finalizer. https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:52: virtual void cleanup() { } Does this need to be virtual? https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:94: m_scrollAnimator->cleanup(); You don't need this, right? https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:101: m_scrollAnimator->cleanup(); Ditto. https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:590: #if ENABLE(OILPAN) Remove this.
https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mac/ScrollAnimatorMac.mm:708: cleanup(); On 2015/12/03 03:47:11, haraken wrote: > > Remove this. cleanup() is already called in the pre-finalizer. > Done. https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:52: virtual void cleanup() { } On 2015/12/03 03:47:11, haraken wrote: > > Does this need to be virtual? Yes. Without it, ((ScrollAnimatorBase*)ptr)->cleanup() calls ScrollAnimatorBase::cleanup() even if ptr figures a ScrollAnimatorMac object. https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:94: m_scrollAnimator->cleanup(); On 2015/12/03 03:47:11, haraken wrote: > > You don't need this, right? This is needed. On non-oilpan build, ScrollableArea can die before GC runs. In such a case, Timer in ScrollAnimatorMac can be triggered and tries to access dead ScrollableArea before GC runs. https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:101: m_scrollAnimator->cleanup(); On 2015/12/03 03:47:11, haraken wrote: > > Ditto. This is required. The failure in PS5 happens as following. 1. m_scrollAnimator (*) is cleared here. (its object is not reachable, but alive) 2. 'this' object dies, but it has no reference to (*), the dtor does not clean it up. 3. Timer in (*) is kicked and tries to access 'this' object. => BAD_ACCESS 4. GC should run and collect untraced ScrollAnimatorMac (*). https://codereview.chromium.org/1477113003/diff/270001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:590: #if ENABLE(OILPAN) On 2015/12/03 03:47:11, haraken wrote: > > Remove this. Done.
This CL is getting pretty tricky... https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:111: RawPtrWillBeMember<ScrollableArea> m_scrollableArea; In non-oilpan, this raw pointer has a risk of becoming a stale pointer (if the ScrollableArea gets destructed before the next GC collects the ScrollAnimatorBase). Sorry about getting you back and forth, but I'd suggest making ScrollableArea NoBaseWillBeGarbageCollected (not GarbageCollected). Then the risk will be gone. Also you don't need to call cleanup() only in non-oilpan when the ScrollableArea gets destructed.
On 2015/12/03 05:34:35, haraken wrote: > This CL is getting pretty tricky... > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:111: > RawPtrWillBeMember<ScrollableArea> m_scrollableArea; > > In non-oilpan, this raw pointer has a risk of becoming a stale pointer (if the > ScrollableArea gets destructed before the next GC collects the > ScrollAnimatorBase). > > Sorry about getting you back and forth, but I'd suggest making ScrollableArea > NoBaseWillBeGarbageCollected (not GarbageCollected). Then the risk will be gone. > Also you don't need to call cleanup() only in non-oilpan when the ScrollableArea > gets destructed. ScrollableArea is already NoBaseWillBeGarbageCollectedMixin, and ScrollAnimatorBase/ScrollAnimatorMac becomes GarbageCollected.
On 2015/12/03 06:03:16, peria wrote: > On 2015/12/03 05:34:35, haraken wrote: > > This CL is getting pretty tricky... > > > > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): > > > > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:111: > > RawPtrWillBeMember<ScrollableArea> m_scrollableArea; > > > > In non-oilpan, this raw pointer has a risk of becoming a stale pointer (if the > > ScrollableArea gets destructed before the next GC collects the > > ScrollAnimatorBase). > > > > Sorry about getting you back and forth, but I'd suggest making ScrollableArea > > NoBaseWillBeGarbageCollected (not GarbageCollected). Then the risk will be > gone. > > Also you don't need to call cleanup() only in non-oilpan when the > ScrollableArea > > gets destructed. > > ScrollableArea is already NoBaseWillBeGarbageCollectedMixin, and > ScrollAnimatorBase/ScrollAnimatorMac becomes GarbageCollected. Sorry, I wanted to say: Make ScrollAnimatorBase/ScrollAnimatorMac NoBaseWillBeGarbageCollected. The complexity of this CL comes from the fact that ScrollableArea and ScrollAnimatorBase do not die at the same time in non-oilpan. If both are NoBase, then the two objects are guaranteed to die at the same time (because ScrollableArea holds an OwnPtr to ScrollAnimatorBase).
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/330001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:330001) has been deleted
The CQ bit was checked by peria@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/350001
https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:111: RawPtrWillBeMember<ScrollableArea> m_scrollableArea; On 2015/12/03 05:34:35, haraken wrote: > > In non-oilpan, this raw pointer has a risk of becoming a stale pointer (if the > ScrollableArea gets destructed before the next GC collects the > ScrollAnimatorBase). > > Sorry about getting you back and forth, but I'd suggest making ScrollableArea > NoBaseWillBeGarbageCollected (not GarbageCollected). Then the risk will be gone. > Also you don't need to call cleanup() only in non-oilpan when the ScrollableArea > gets destructed. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.h (right): https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.h:24: class ProgrammaticScrollAnimator : public GarbageCollectedFinalized<ProgrammaticScrollAnimator>, private WebCompositorAnimationPlayerClient, WebCompositorAnimationDelegate { You need to change ProgrammaticScrollAnimator to NoBaseWillBeGarbageCollectedFinalized. Otherwise, it can happen that in non-oilpan ScrollableArea gets destructed but ProgrammaticScrollAnimator survives until the next GC is triggered. Then ProgrammaticScrollAnimator::m_scrollableArea becomes a stale pointer. https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right): https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:107: ScrollAnimator* scrollAnimator = new ScrollAnimator(scrollableArea.get(), getMockedTime); Doesn't this need to be OwnPtrWillBeRawPtr? https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:175: ScrollAnimator* scrollAnimator = new ScrollAnimator(scrollableArea.get(), getMockedTime); Ditto.
On 2015/12/03 05:34:35, haraken wrote: > This CL is getting pretty tricky... > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h (right): > > https://codereview.chromium.org/1477113003/diff/310001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scroll/ScrollAnimatorBase.h:111: > RawPtrWillBeMember<ScrollableArea> m_scrollableArea; > > In non-oilpan, this raw pointer has a risk of becoming a stale pointer (if the > ScrollableArea gets destructed before the next GC collects the > ScrollAnimatorBase). > > Sorry about getting you back and forth, but I'd suggest making ScrollableArea > NoBaseWillBeGarbageCollected (not GarbageCollected). Then the risk will be gone. > Also you don't need to call cleanup() only in non-oilpan when the ScrollableArea > gets destructed. It seems that this issue has been existing before this CL on Oilpan build. Fortunately, such accesses may not happen. For example, PaintLayerScrollableArea can be created in PaintLayer::updateScrollableArea(). If the PaintLayer dies, the PaintLayerScrollableArea and ScrollableAnimators become unreachable. However, until the next GC runs, the Timer is still active, and may access the dead PaintLayer via mouseLocationInContentAreaForScrollerImpPair().
https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.h (right): https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ProgrammaticScrollAnimator.h:24: class ProgrammaticScrollAnimator : public GarbageCollectedFinalized<ProgrammaticScrollAnimator>, private WebCompositorAnimationPlayerClient, WebCompositorAnimationDelegate { On 2015/12/03 10:45:49, haraken wrote: > > You need to change ProgrammaticScrollAnimator to > NoBaseWillBeGarbageCollectedFinalized. Otherwise, it can happen that in > non-oilpan ScrollableArea gets destructed but ProgrammaticScrollAnimator > survives until the next GC is triggered. Then > ProgrammaticScrollAnimator::m_scrollableArea becomes a stale pointer. Done. https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp (right): https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:107: ScrollAnimator* scrollAnimator = new ScrollAnimator(scrollableArea.get(), getMockedTime); On 2015/12/03 10:45:49, haraken wrote: > > Doesn't this need to be OwnPtrWillBeRawPtr? Done. https://codereview.chromium.org/1477113003/diff/350001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollAnimatorTest.cpp:175: ScrollAnimator* scrollAnimator = new ScrollAnimator(scrollableArea.get(), getMockedTime); On 2015/12/03 10:45:49, haraken wrote: > > Ditto. Done. https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if OS(MACOSX) && ENABLE(OILPAN) This clearing is added to make the probability to kick the timer in ScrollAnimatorMac small.
https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if OS(MACOSX) && ENABLE(OILPAN) On 2015/12/03 13:42:08, peria wrote: > This clearing is added to make the probability to kick the timer in > ScrollAnimatorMac small. Add something to make the likelihood small sounds bad, since it means that we have a bug... In oilpan, why do we need to explicitly call cleanup() here? Given the following facts, it should not happen that the ScrollAnimator's timer is invoked after the ScrollableArea dies. - Both ScrollAnimator and ScrollableArea are on oilpan's heap. - The ScrollAnimator has a strong reference to the ScrollableArea. Thus it should not happen that the ScrollableArea dies before the ScrollAnimator. - If the ScrollableArea and the ScrollAnimator die in the same GC cycle (this can happen), the ScrollAnimator's pre-finalizer runs cleanup() and stops the timer. Thus the timer should not be triggered after the ScrollableArea gets destructed. In conclusion, I think that we don't need to call cleanup() here. Am I missing something?
On 2015/12/03 14:48:47, haraken wrote: > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if OS(MACOSX) > && ENABLE(OILPAN) > On 2015/12/03 13:42:08, peria wrote: > > This clearing is added to make the probability to kick the timer in > > ScrollAnimatorMac small. > > Add something to make the likelihood small sounds bad, since it means that we > have a bug... Strongly agree. But without this, webkit_unit_tests fails. Before this CL, clearing OwnPtr<> did the same thing. > In oilpan, why do we need to explicitly call cleanup() here? Given the following > facts, it should not happen that the ScrollAnimator's timer is invoked after the > ScrollableArea dies. > > - Both ScrollAnimator and ScrollableArea are on oilpan's heap. > - The ScrollAnimator has a strong reference to the ScrollableArea. Thus it > should not happen that the ScrollableArea dies before the ScrollAnimator. > - If the ScrollableArea and the ScrollAnimator die in the same GC cycle (this > can happen), the ScrollAnimator's pre-finalizer runs cleanup() and stops the > timer. Thus the timer should not be triggered after the ScrollableArea gets > destructed. > > In conclusion, I think that we don't need to call cleanup() here. Am I missing > something? When ScrollableArea::clearAnimators() is called or ScrollableArea becomes unreachable, they are dead in Oilpan context, but they are actually alive (not released). It means that the timer can be triggered by the next GC collects them, and it can access other objects via RetainPtr<> in ScrollAnimatorMac.
On 2015/12/03 15:08:13, peria wrote: > On 2015/12/03 14:48:47, haraken wrote: > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): > > > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if > OS(MACOSX) > > && ENABLE(OILPAN) > > On 2015/12/03 13:42:08, peria wrote: > > > This clearing is added to make the probability to kick the timer in > > > ScrollAnimatorMac small. > > > > Add something to make the likelihood small sounds bad, since it means that we > > have a bug... > > Strongly agree. > But without this, webkit_unit_tests fails. Before this CL, clearing OwnPtr<> > did the same thing. > > > > In oilpan, why do we need to explicitly call cleanup() here? Given the > following > > facts, it should not happen that the ScrollAnimator's timer is invoked after > the > > ScrollableArea dies. > > > > - Both ScrollAnimator and ScrollableArea are on oilpan's heap. > > - The ScrollAnimator has a strong reference to the ScrollableArea. Thus it > > should not happen that the ScrollableArea dies before the ScrollAnimator. > > - If the ScrollableArea and the ScrollAnimator die in the same GC cycle (this > > can happen), the ScrollAnimator's pre-finalizer runs cleanup() and stops the > > timer. Thus the timer should not be triggered after the ScrollableArea gets > > destructed. > > > > In conclusion, I think that we don't need to call cleanup() here. Am I missing > > something? > > When ScrollableArea::clearAnimators() is called or ScrollableArea becomes > unreachable, > they are dead in Oilpan context, but they are actually alive (not released). > > It means that the timer can be triggered by the next GC collects them, and it > can > access other objects via RetainPtr<> in ScrollAnimatorMac. Yes, it can happen (i.e., the timer is triggered before the next GC hits) but it should not be a problem because both of the ScrollableArea and ScrollableAnimator are not yet destructed, right? The scenario we should avoid is: - ScrollableArea gets destructed. - ScrollableAnimator is not yet destructed. - The timer is triggered. The timer touches the ScrollableArea, which has already been destructed. This scenario is already avoided by the pre-finalizer of ScrollableAnimator. So if the test still fails, it would mean that we're seeing another issue.
On 2015/12/03 15:28:01, haraken wrote: > On 2015/12/03 15:08:13, peria wrote: > > On 2015/12/03 14:48:47, haraken wrote: > > > > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): > > > > > > > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if > > OS(MACOSX) > > > && ENABLE(OILPAN) > > > On 2015/12/03 13:42:08, peria wrote: > > > > This clearing is added to make the probability to kick the timer in > > > > ScrollAnimatorMac small. > > > > > > Add something to make the likelihood small sounds bad, since it means that > we > > > have a bug... > > > > Strongly agree. > > But without this, webkit_unit_tests fails. Before this CL, clearing OwnPtr<> > > did the same thing. > > > > > > > In oilpan, why do we need to explicitly call cleanup() here? Given the > > following > > > facts, it should not happen that the ScrollAnimator's timer is invoked after > > the > > > ScrollableArea dies. > > > > > > - Both ScrollAnimator and ScrollableArea are on oilpan's heap. > > > - The ScrollAnimator has a strong reference to the ScrollableArea. Thus it > > > should not happen that the ScrollableArea dies before the ScrollAnimator. > > > - If the ScrollableArea and the ScrollAnimator die in the same GC cycle > (this > > > can happen), the ScrollAnimator's pre-finalizer runs cleanup() and stops the > > > timer. Thus the timer should not be triggered after the ScrollableArea gets > > > destructed. > > > > > > In conclusion, I think that we don't need to call cleanup() here. Am I > missing > > > something? > > > > When ScrollableArea::clearAnimators() is called or ScrollableArea becomes > > unreachable, > > they are dead in Oilpan context, but they are actually alive (not released). > > > > It means that the timer can be triggered by the next GC collects them, and it > > can > > access other objects via RetainPtr<> in ScrollAnimatorMac. > > Yes, it can happen (i.e., the timer is triggered before the next GC hits) but it > should not be a problem because both of the ScrollableArea and > ScrollableAnimator are not yet destructed, right? > > The scenario we should avoid is: > > - ScrollableArea gets destructed. > - ScrollableAnimator is not yet destructed. > - The timer is triggered. The timer touches the ScrollableArea, which has > already been destructed. > > This scenario is already avoided by the pre-finalizer of ScrollableAnimator. > > So if the test still fails, it would mean that we're seeing another issue. Ah, I'm getting to understand the situation. The problem is NOT that: - the timer touches the ScrollableArea got destructed but that: - the timer touches the ScrollableArea after the ScrollableArea called clearAnimators() If that is the case, we need to make sure that: (a) the timer does not fire after the ScrollableArea called clearAnimators(); and (b) the timer does not fire after the ScrollableArea got destructed (a) is realized by the explicit cleanup() call. (b) is realized by the pre-finalizer. If my understanding is correct, the patch set 12 LGTM. Nit: Rename "cleanup" to "dispose". We normally use "dispose" for promptly cleaning up something.
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1477113003/#ps390001 (title: "rename")
On 2015/12/03 15:36:03, haraken wrote: > On 2015/12/03 15:28:01, haraken wrote: > > On 2015/12/03 15:08:13, peria wrote: > > > On 2015/12/03 14:48:47, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > > > File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1477113003/diff/370001/third_party/WebKit/Sou... > > > > third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:96: #if > > > OS(MACOSX) > > > > && ENABLE(OILPAN) > > > > On 2015/12/03 13:42:08, peria wrote: > > > > > This clearing is added to make the probability to kick the timer in > > > > > ScrollAnimatorMac small. > > > > > > > > Add something to make the likelihood small sounds bad, since it means that > > we > > > > have a bug... > > > > > > Strongly agree. > > > But without this, webkit_unit_tests fails. Before this CL, clearing > OwnPtr<> > > > did the same thing. > > > > > > > > > > In oilpan, why do we need to explicitly call cleanup() here? Given the > > > following > > > > facts, it should not happen that the ScrollAnimator's timer is invoked > after > > > the > > > > ScrollableArea dies. > > > > > > > > - Both ScrollAnimator and ScrollableArea are on oilpan's heap. > > > > - The ScrollAnimator has a strong reference to the ScrollableArea. Thus it > > > > should not happen that the ScrollableArea dies before the ScrollAnimator. > > > > - If the ScrollableArea and the ScrollAnimator die in the same GC cycle > > (this > > > > can happen), the ScrollAnimator's pre-finalizer runs cleanup() and stops > the > > > > timer. Thus the timer should not be triggered after the ScrollableArea > gets > > > > destructed. > > > > > > > > In conclusion, I think that we don't need to call cleanup() here. Am I > > missing > > > > something? > > > > > > When ScrollableArea::clearAnimators() is called or ScrollableArea becomes > > > unreachable, > > > they are dead in Oilpan context, but they are actually alive (not released). > > > > > > It means that the timer can be triggered by the next GC collects them, and > it > > > can > > > access other objects via RetainPtr<> in ScrollAnimatorMac. > > > > Yes, it can happen (i.e., the timer is triggered before the next GC hits) but > it > > should not be a problem because both of the ScrollableArea and > > ScrollableAnimator are not yet destructed, right? > > > > The scenario we should avoid is: > > > > - ScrollableArea gets destructed. > > - ScrollableAnimator is not yet destructed. > > - The timer is triggered. The timer touches the ScrollableArea, which has > > already been destructed. > > > > This scenario is already avoided by the pre-finalizer of ScrollableAnimator. > > > > So if the test still fails, it would mean that we're seeing another issue. > > Ah, I'm getting to understand the situation. > > The problem is NOT that: > > - the timer touches the ScrollableArea got destructed > > but that: > > - the timer touches the ScrollableArea after the ScrollableArea called > clearAnimators() > > If that is the case, we need to make sure that: > > (a) the timer does not fire after the ScrollableArea called clearAnimators(); > and > (b) the timer does not fire after the ScrollableArea got destructed > > (a) is realized by the explicit cleanup() call. (b) is realized by the > pre-finalizer. > > If my understanding is correct, the patch set 12 LGTM. > > Nit: Rename "cleanup" to "dispose". We normally use "dispose" for promptly > cleaning up something. Thank you for the explanation! Correct.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/390001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by peria@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477113003/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477113003/390001
Message was sent while issue was closed.
Description was changed from ========== Move ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap. Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea. BUG=550325 ========== to ========== Move ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap. Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea. BUG=550325 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:390001)
Message was sent while issue was closed.
Description was changed from ========== Move ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap. Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea. BUG=550325 ========== to ========== Move ScrollAnimatorBase and ProgrammaticScrollAnimator onto oilpan heap. Beside it, I removed a redundant struct ScrollableAreaAnimators in ScrollableArea. BUG=550325 Committed: https://crrev.com/8e25ffb821fc66cbca271b2ca5b7b1908811f977 Cr-Commit-Position: refs/heads/master@{#363117} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/8e25ffb821fc66cbca271b2ca5b7b1908811f977 Cr-Commit-Position: refs/heads/master@{#363117} |
