REGRESSION (r182215): Feedly crashes when closing article
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Apr 2015 22:39:29 +0000 (22:39 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Apr 2015 22:39:29 +0000 (22:39 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182346 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash-expected.txt [new file with mode: 0644]
LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h
Source/WebCore/page/scrolling/ScrollingCoordinator.cpp
Source/WebCore/page/scrolling/ScrollingCoordinator.h
Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/Scrolling/RemoteScrollingCoordinator.mm
Source/WebKit2/WebProcess/WebPage/mac/TiledCoreAnimationDrawingArea.mm

index 27779e5..d7bfda5 100644 (file)
@@ -1,5 +1,18 @@
 2015-04-04  Simon Fraser  <simon.fraser@apple.com>
 
+        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.
+        
+        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.
+
+2015-04-04  Simon Fraser  <simon.fraser@apple.com>
+
         Differentiate between composited scrolling, and async scrolling
         https://bugs.webkit.org/show_bug.cgi?id=143291
 
diff --git a/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash-expected.txt b/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash-expected.txt
new file mode 100644 (file)
index 0000000..7e24c80
--- /dev/null
@@ -0,0 +1,3 @@
+Test passes if it does not crash.
+
+
diff --git a/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html b/LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/non-fast-region/compute-region-inside-delete-renderer-crash.html
new file mode 100644 (file)
index 0000000..eb2ca69
--- /dev/null
@@ -0,0 +1,41 @@
+<html>
+<head>
+    <style>
+        .scroller {
+            height: 10px;
+            width: 10px;
+            overflow: scroll;
+            border: 1px solid black;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            setTimeout(function() {
+                var scroller = document.getElementById('scoller');
+                var newNode = document.createElement('div');
+                var container = document.getElementById('container');
+                container.insertBefore(newNode, scroller);
+                scroller.parentNode.removeChild(scroller);
+
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 0);
+        }
+        
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+<p>Test passes if it does not crash.</p>
+<div><div id="container"><div id="scoller" class="scroller">foo</div></div></div>
+<script>
+    document.getElementById("scoller").addEventListener("mousewheel", function() { }, false);
+</script>
+</body>
+</html>
index 96da256..2cb5542 100644 (file)
@@ -1,5 +1,46 @@
 2015-04-04  Simon Fraser  <simon.fraser@apple.com>
 
+        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.
+        
+        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):
+
+2015-04-04  Simon Fraser  <simon.fraser@apple.com>
+
         Differentiate between composited scrolling, and async scrolling
         https://bugs.webkit.org/show_bug.cgi?id=143291
 
index 6c75fb2..1209049 100644 (file)
@@ -73,6 +73,27 @@ static inline void setStateScrollingNodeSnapOffsetsAsFloat(ScrollingStateScrolli
         node.setVerticalSnapOffsets(snapOffsetsAsFloat);
 }
 
+void AsyncScrollingCoordinator::setNonFastScrollableRegionDirty()
+{
+    m_nonFastScrollableRegionDirty = true;
+    // We have to schedule a commit, but the computed non-fast region may not have actually changed.
+    scheduleTreeStateCommit();
+}
+
+void AsyncScrollingCoordinator::willCommitTree()
+{
+    updateNonFastScrollableRegion();
+}
+
+void AsyncScrollingCoordinator::updateNonFastScrollableRegion()
+{
+    if (!m_nonFastScrollableRegionDirty)
+        return;
+
+    m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
+    m_nonFastScrollableRegionDirty = false;
+}
+
 void AsyncScrollingCoordinator::frameViewLayoutUpdated(FrameView& frameView)
 {
     ASSERT(isMainThread());
@@ -88,6 +109,7 @@ void AsyncScrollingCoordinator::frameViewLayoutUpdated(FrameView& frameView)
     // In the future, we may want to have the ability to set non-fast scrolling regions for more than
     // just the root node. But right now, this concept only applies to the root.
     m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
+    m_nonFastScrollableRegionDirty = false;
 
     if (!coordinatesScrollingForFrameView(frameView))
         return;
@@ -135,8 +157,7 @@ void AsyncScrollingCoordinator::frameViewNonFastScrollableRegionChanged(FrameVie
     if (!m_scrollingStateTree->rootStateNode())
         return;
 
-    // FIXME: computeNonFastScrollableRegion lazily.
-    m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
+    setNonFastScrollableRegionDirty();
 }
 
 void AsyncScrollingCoordinator::frameViewRootLayerDidChange(FrameView& frameView)
@@ -500,8 +521,11 @@ void AsyncScrollingCoordinator::setScrollPinningBehavior(ScrollPinningBehavior p
 
 String AsyncScrollingCoordinator::scrollingStateTreeAsText() const
 {
-    if (m_scrollingStateTree->rootStateNode())
+    if (m_scrollingStateTree->rootStateNode()) {
+        if (m_nonFastScrollableRegionDirty)
+            m_scrollingStateTree->rootStateNode()->setNonFastScrollableRegion(computeNonFastScrollableRegion(m_page->mainFrame(), IntPoint()));
         return m_scrollingStateTree->rootStateNode()->scrollingStateTreeAsText();
+    }
 
     return String();
 }
index 0226c5e..d9a4c53 100644 (file)
@@ -68,6 +68,9 @@ protected:
     void updateScrollPositionAfterAsyncScroll(ScrollingNodeID, const FloatPoint&, bool programmaticScroll, SetOrSyncScrollingLayerPosition);
 
     WEBCORE_EXPORT virtual String scrollingStateTreeAsText() const override;
+    WEBCORE_EXPORT virtual void willCommitTree() override;
+
+    bool nonFastScrollableRegionDirty() const { return m_nonFastScrollableRegionDirty; }
 
 private:
     virtual bool isAsyncScrollingCoordinator() const override { return true; }
@@ -104,6 +107,8 @@ private:
     void updateMainFrameScrollLayerPosition();
 
     void updateScrollPositionAfterAsyncScrollTimerFired();
+    void setNonFastScrollableRegionDirty();
+    void updateNonFastScrollableRegion();
     
     FrameView* frameViewForScrollingNode(ScrollingNodeID) const;
 
@@ -140,6 +145,8 @@ private:
 
     std::unique_ptr<ScrollingStateTree> m_scrollingStateTree;
     RefPtr<ScrollingTree> m_scrollingTree;
+
+    bool m_nonFastScrollableRegionDirty { false };
 };
 
 } // namespace WebCore
index 3a10c3e..b28ee73 100644 (file)
@@ -67,7 +67,6 @@ PassRefPtr<ScrollingCoordinator> ScrollingCoordinator::create(Page* page)
 
 ScrollingCoordinator::ScrollingCoordinator(Page* page)
     : m_page(page)
-    , m_forceSynchronousScrollLayerPositionUpdates(false)
 {
 }
 
@@ -126,6 +125,9 @@ Region ScrollingCoordinator::computeNonFastScrollableRegion(const Frame& frame,
     if (!frameView)
         return nonFastScrollableRegion;
 
+    // FIXME: should ASSERT(!frameView->needsLayout()) here, but need to fix DebugPageOverlays
+    // to not ask for regions at bad times.
+
     IntPoint offset = frameLocation;
     offset.moveBy(frameView->frameRect().location());
     offset.move(0, frameView->topContentInset());
index 5148d5e..27b9bcd 100644 (file)
@@ -214,6 +214,8 @@ protected:
     GraphicsLayer* headerLayerForFrameView(FrameView&);
     GraphicsLayer* footerLayerForFrameView(FrameView&);
 
+    virtual void willCommitTree() { }
+
     Page* m_page; // FIXME: ideally this would be a reference but it gets nulled on async teardown.
 
 private:
@@ -222,7 +224,7 @@ private:
     virtual bool hasVisibleSlowRepaintViewportConstrainedObjects(const FrameView&) const;
     void updateSynchronousScrollingReasons(FrameView&);
     
-    bool m_forceSynchronousScrollLayerPositionUpdates;
+    bool m_forceSynchronousScrollLayerPositionUpdates { false };
 };
 
 } // namespace WebCore
index b4b20db..111bdac 100644 (file)
@@ -79,9 +79,6 @@ void ScrollingCoordinatorMac::pageDestroyed()
 
 void ScrollingCoordinatorMac::commitTreeStateIfNeeded()
 {
-    if (!scrollingStateTree()->hasChangedProperties())
-        return;
-
     commitTreeState();
     m_scrollingStateTreeCommitterTimer.stop();
 }
@@ -103,7 +100,7 @@ bool ScrollingCoordinatorMac::handleWheelEvent(FrameView&, const PlatformWheelEv
 
 void ScrollingCoordinatorMac::scheduleTreeStateCommit()
 {
-    ASSERT(scrollingStateTree()->hasChangedProperties());
+    ASSERT(scrollingStateTree()->hasChangedProperties() || nonFastScrollableRegionDirty());
 
     if (m_scrollingStateTreeCommitterTimer.isActive())
         return;
@@ -118,7 +115,9 @@ void ScrollingCoordinatorMac::scrollingStateTreeCommitterTimerFired()
 
 void ScrollingCoordinatorMac::commitTreeState()
 {
-    ASSERT(scrollingStateTree()->hasChangedProperties());
+    willCommitTree();
+    if (!scrollingStateTree()->hasChangedProperties())
+        return;
 
     RefPtr<ThreadedScrollingTree> threadedScrollingTree = downcast<ThreadedScrollingTree>(scrollingTree());
     ScrollingStateTree* unprotectedTreeState = scrollingStateTree()->commit(LayerRepresentation::PlatformLayerRepresentation).release();
index 5f07ce9..a701c0b 100644 (file)
@@ -1,3 +1,27 @@
+2015-04-04  Simon Fraser  <simon.fraser@apple.com>
+
+        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.
+        
+        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.
+
 2015-04-03  Beth Dakin  <bdakin@apple.com>
 
         Attempted build fix.
index f777398..59fe5b9 100644 (file)
@@ -86,6 +86,7 @@ void RemoteScrollingCoordinator::setScrollPinningBehavior(ScrollPinningBehavior)
 
 void RemoteScrollingCoordinator::buildTransaction(RemoteScrollingCoordinatorTransaction& transaction)
 {
+    willCommitTree();
     transaction.setStateTreeToEncode(scrollingStateTree()->commit(LayerRepresentation::PlatformLayerIDRepresentation));
 }
 
index 6783797..f0759f2 100644 (file)
@@ -318,10 +318,10 @@ bool TiledCoreAnimationDrawingArea::flushLayers()
             m_viewOverlayRootLayer->flushCompositingState(visibleRect);
 
         bool returnValue = m_webPage.mainFrameView()->flushCompositingStateIncludingSubframes();
-    #if ENABLE(ASYNC_SCROLLING)
+#if ENABLE(ASYNC_SCROLLING)
         if (ScrollingCoordinator* scrollingCoordinator = m_webPage.corePage()->scrollingCoordinator())
             scrollingCoordinator->commitTreeStateIfNeeded();
-    #endif
+#endif
 
         // If we have an active transient zoom, we want the zoom to win over any changes
         // that WebCore makes to the relevant layers, so re-apply our changes after flushing.