Changeset 182346 in webkit


Ignore:
Timestamp:
Apr 4, 2015, 3:39:29 PM (11 years ago)
Author:
Simon Fraser
Message:

REGRESSION (r182215): Feedly crashes when closing article
​https://bugs.webkit.org/show_bug.cgi?id=143405
rdar://problem/20382734, rdar://problem/20395497

Reviewed by Tim Horton.

Source/WebCore:

Calling computeNonFastScrollableRegion() eagerly when scrollable areas come and go
is bad, because it can cause FrameView::layout() to get called in the middle of
RenderObject destruction, which leaves the render tree in a bad state.

Fix by calling computeNonFastScrollableRegion() lazily, just before scrolling tree commit.

AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged() now just sets
a flag to say that the non-fast region needs to be recomputed, and that schedules
a scrolling tree commit. When the commit happens, we recompute the region. If the
region didn't change, and no other changes are pending, there's no need to commit.

Test: platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html

  • page/scrolling/AsyncScrollingCoordinator.cpp:

(WebCore::AsyncScrollingCoordinator::setNonFastScrollableRegionDirty):
(WebCore::AsyncScrollingCoordinator::willCommitTree):
(WebCore::AsyncScrollingCoordinator::updateNonFastScrollableRegion):
(WebCore::AsyncScrollingCoordinator::frameViewLayoutUpdated):
(WebCore::AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged):
(WebCore::AsyncScrollingCoordinator::scrollingStateTreeAsText): Need to eagerly update
the non-fast scrollable region.

  • page/scrolling/AsyncScrollingCoordinator.h:

(WebCore::AsyncScrollingCoordinator::nonFastScrollableRegionDirty):

  • page/scrolling/ScrollingCoordinator.cpp:

(WebCore::ScrollingCoordinator::ScrollingCoordinator):
(WebCore::ScrollingCoordinator::computeNonFastScrollableRegion):

  • page/scrolling/ScrollingCoordinator.h:

(WebCore::ScrollingCoordinator::willCommitTree):

  • page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::commitTreeStateIfNeeded):
(WebCore::ScrollingCoordinatorMac::scheduleTreeStateCommit):
(WebCore::ScrollingCoordinatorMac::commitTreeState):

Source/WebKit2:

Calling computeNonFastScrollableRegion() eagerly when scrollable areas come and go
is bad, because it can cause FrameView::layout() to get called in the middle of
RenderObject destruction, which leaves the render tree in a bad state.

Fix by calling computeNonFastScrollableRegion() lazily, just before scrolling tree commit.

AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged() now just sets
a flag to say that the non-fast region needs to be recomputed, and that schedules
a scrolling tree commit. When the commit happens, we recompute the region. If the
region didn't change, and no other changes are pending, there's no need to commit.

  • WebProcess/Scrolling/RemoteScrollingCoordinator.mm:

(WebKit::RemoteScrollingCoordinator::buildTransaction):

  • WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:

(WebKit::TiledCoreAnimationDrawingArea::flushLayers): Outdent #ifdefs.

LayoutTests:

Test that triggers a crash without the fix (thanks to Zalan for the test).

  • platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash-expected.txt: Added.
  • platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html: Added.
Location:
trunk
Files:
2 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r182345 r182346  
     12015-04-04  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r182215): Feedly crashes when closing article
     4        https://bugs.webkit.org/show_bug.cgi?id=143405
     5        rdar://problem/20382734, rdar://problem/20395497
     6
     7        Reviewed by Tim Horton.
     8       
     9        Test that triggers a crash without the fix (thanks to Zalan for the test).
     10
     11        * platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash-expected.txt: Added.
     12        * platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html: Added.
     13
    1142015-04-04  Simon Fraser  <simon.fraser@apple.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r182345 r182346  
     12015-04-04  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r182215): Feedly crashes when closing article
     4        https://bugs.webkit.org/show_bug.cgi?id=143405
     5        rdar://problem/20382734, rdar://problem/20395497
     6
     7        Reviewed by Tim Horton.
     8       
     9        Calling computeNonFastScrollableRegion() eagerly when scrollable areas come and go
     10        is bad, because it can cause FrameView::layout() to get called in the middle of
     11        RenderObject destruction, which leaves the render tree in a bad state.
     12       
     13        Fix by calling computeNonFastScrollableRegion() lazily, just before scrolling tree commit.
     14       
     15        AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged() now just sets
     16        a flag to say that the non-fast region needs to be recomputed, and that schedules
     17        a scrolling tree commit. When the commit happens, we recompute the region. If the
     18        region didn't change, and no other changes are pending, there's no need to commit.
     19
     20        Test: platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html
     21
     22        * page/scrolling/AsyncScrollingCoordinator.cpp:
     23        (WebCore::AsyncScrollingCoordinator::setNonFastScrollableRegionDirty):
     24        (WebCore::AsyncScrollingCoordinator::willCommitTree):
     25        (WebCore::AsyncScrollingCoordinator::updateNonFastScrollableRegion):
     26        (WebCore::AsyncScrollingCoordinator::frameViewLayoutUpdated):
     27        (WebCore::AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged):
     28        (WebCore::AsyncScrollingCoordinator::scrollingStateTreeAsText): Need to eagerly update
     29        the non-fast scrollable region.
     30        * page/scrolling/AsyncScrollingCoordinator.h:
     31        (WebCore::AsyncScrollingCoordinator::nonFastScrollableRegionDirty):
     32        * page/scrolling/ScrollingCoordinator.cpp:
     33        (WebCore::ScrollingCoordinator::ScrollingCoordinator):
     34        (WebCore::ScrollingCoordinator::computeNonFastScrollableRegion):
     35        * page/scrolling/ScrollingCoordinator.h:
     36        (WebCore::ScrollingCoordinator::willCommitTree):
     37        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
     38        (WebCore::ScrollingCoordinatorMac::commitTreeStateIfNeeded):
     39        (WebCore::ScrollingCoordinatorMac::scheduleTreeStateCommit):
     40        (WebCore::ScrollingCoordinatorMac::commitTreeState):
     41
    1422015-04-04  Simon Fraser  <simon.fraser@apple.com>
    243

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp

    r182242 r182346  
    7474}
    7575
     76void AsyncScrollingCoordinator::setNonFastScrollableRegionDirty()
     77{
     78    m_nonFastScrollableRegionDirty = true;
     79    // We have to schedule a commit, but the computed non-fast region may not have actually changed.
     80    scheduleTreeStateCommit();
     81}
     82
     83void AsyncScrollingCoordinator::willCommitTree()
     84{
     85    updateNonFastScrollableRegion();
     86}
     87
     88void AsyncScrollingCoordinator::updateNonFastScrollableRegion()
     89{
     90    if (!m_nonFastScrollableRegionDirty)
     91        return;
     92
     93    m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
     94    m_nonFastScrollableRegionDirty = false;
     95}
     96
    7697void AsyncScrollingCoordinator::frameViewLayoutUpdated(FrameView& frameView)
    7798{
    … …  
    89110    // just the root node. But right now, this concept only applies to the root.
    90111    m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
     112    m_nonFastScrollableRegionDirty = false;
    91113
    92114    if (!coordinatesScrollingForFrameView(frameView))
    … …  
    136158        return;
    137159
    138     // FIXME: computeNonFastScrollableRegion lazily.
    139     m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
     160    setNonFastScrollableRegionDirty();
    140161}
    141162
    … …  
    501522String AsyncScrollingCoordinator::scrollingStateTreeAsText() const
    502523{
    503     if (m_scrollingStateTree->rootStateNode())
     524    if (m_scrollingStateTree->rootStateNode()) {
     525        if (m_nonFastScrollableRegionDirty)
     526            m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
    504527        return m_scrollingStateTree->rootStateNode()->scrollingStateTreeAsText();
     528    }
    505529
    506530    return String();

  • trunk/Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h

    r182242 r182346  
    6969
    7070    WEBCORE_EXPORT virtual String scrollingStateTreeAsText() const override;
     71    WEBCORE_EXPORT virtual void willCommitTree() override;
     72
     73    bool nonFastScrollableRegionDirty() const { return m_nonFastScrollableRegionDirty; }
    7174
    7275private:
    … …  
    105108
    106109    void updateScrollPositionAfterAsyncScrollTimerFired();
     110    void setNonFastScrollableRegionDirty();
     111    void updateNonFastScrollableRegion();
    107112   
    108113    FrameView* frameViewForScrollingNode(ScrollingNodeID) const;
    … …  
    141146    std::unique_ptr<ScrollingStateTree> m_scrollingStateTree;
    142147    RefPtr<ScrollingTree> m_scrollingTree;
     148
     149    bool m_nonFastScrollableRegionDirty { false };
    143150};
    144151

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.cpp

    r182345 r182346  
    6868ScrollingCoordinator::ScrollingCoordinator(Page* page)
    6969    : m_page(page)
    70     , m_forceSynchronousScrollLayerPositionUpdates(false)
    7170{
    7271}
    … …  
    126125    if (!frameView)
    127126        return nonFastScrollableRegion;
     127
     128    // FIXME: should ASSERT(!frameView->needsLayout()) here, but need to fix DebugPageOverlays
     129    // to not ask for regions at bad times.
    128130
    129131    IntPoint offset = frameLocation;

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.h

    r182242 r182346  
    215215    GraphicsLayer* footerLayerForFrameView(FrameView&);
    216216
     217    virtual void willCommitTree() { }
     218
    217219    Page* m_page; // FIXME: ideally this would be a reference but it gets nulled on async teardown.
    218220
    … …  
    223225    void updateSynchronousScrollingReasons(FrameView&);
    224226   
    225     bool m_forceSynchronousScrollLayerPositionUpdates;
     227    bool m_forceSynchronousScrollLayerPositionUpdates { false };
    226228};
    227229

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

    r182242 r182346  
    8080void ScrollingCoordinatorMac::commitTreeStateIfNeeded()
    8181{
    82     if (!scrollingStateTree()->hasChangedProperties())
    83         return;
    84 
    8582    commitTreeState();
    8683    m_scrollingStateTreeCommitterTimer.stop();
    … …  
    104101void ScrollingCoordinatorMac::scheduleTreeStateCommit()
    105102{
    106     ASSERT(scrollingStateTree()->hasChangedProperties());
     103    ASSERT(scrollingStateTree()->hasChangedProperties() || nonFastScrollableRegionDirty());
    107104
    108105    if (m_scrollingStateTreeCommitterTimer.isActive())
    … …  
    119116void ScrollingCoordinatorMac::commitTreeState()
    120117{
    121     ASSERT(scrollingStateTree()->hasChangedProperties());
     118    willCommitTree();
     119    if (!scrollingStateTree()->hasChangedProperties())
     120        return;
    122121
    123122    RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());

  • trunk/Source/WebKit2/ChangeLog

    r182341 r182346  
     12015-04-04  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r182215): Feedly crashes when closing article
     4        https://bugs.webkit.org/show_bug.cgi?id=143405
     5        rdar://problem/20382734, rdar://problem/20395497
     6
     7        Reviewed by Tim Horton.
     8       
     9        Calling computeNonFastScrollableRegion() eagerly when scrollable areas come and go
     10        is bad, because it can cause FrameView::layout() to get called in the middle of
     11        RenderObject destruction, which leaves the render tree in a bad state.
     12       
     13        Fix by calling computeNonFastScrollableRegion() lazily, just before scrolling tree commit.
     14       
     15        AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged() now just sets
     16        a flag to say that the non-fast region needs to be recomputed, and that schedules
     17        a scrolling tree commit. When the commit happens, we recompute the region. If the
     18        region didn't change, and no other changes are pending, there's no need to commit.
     19
     20        * WebProcess/Scrolling/RemoteScrollingCoordinator.mm:
     21        (WebKit::RemoteScrollingCoordinator::buildTransaction):
     22        * WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm:
     23        (WebKit::TiledCoreAnimationDrawingArea::flushLayers): Outdent #ifdefs.
     24
    1252015-04-03  Beth Dakin  <bdakin@apple.com>
    226

  • trunk/Source/WebKit2/WebProcess/Scrolling/RemoteScrollingCoordinator.mm

    r182132 r182346  
    8787void RemoteScrollingCoordinator::buildTransaction(RemoteScrollingCoordinatorTransaction& transaction)
    8888{
     89    willCommitTree();
    8990    transaction.setStateTreeToEncode(scrollingStateTree()->commit(LayerRepresentation::PlatformLayerIDRepresentation));
    9091}

  • trunk/Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm

    r179409 r182346  
    319319
    320320        bool returnValue = m_webPage.mainFrameView()->flushCompositingStateIncludingSubframes();
    321     #if ENABLE(ASYNC_SCROLLING)
     321#if ENABLE(ASYNC_SCROLLING)
    322322        if (ScrollingCoordinator* scrollingCoordinator = m_webPage.corePage()->scrollingCoordinator())
    323323            scrollingCoordinator->commitTreeStateIfNeeded();
    324     #endif
     324#endif
    325325
    326326        // If we have an active transient zoom, we want the zoom to win over any changes
Note: See TracChangeset for help on using the changeset viewer.