Changeset 45710 in webkit

Timestamp:

Jul 10, 2009 9:54:14 AM (15 years ago)

Author:

mitz@apple.com

Message:

WebCore:

Reviewed by Simon Fraser.

• fix ​https://bugs.webkit.org/show_bug.cgi?id=27137 <rdar://problem/7043124> REGRESSION (r44311): Reproducible crash due to infinite recursion into FrameLoader::gotoAnchor() -> FrameView::layout()

Test: fast/loader/goto-anchor-infinite-layout.html

• loader/FrameLoader.cpp: (WebCore::FrameLoader::gotoAnchor): Moved the code to update layout, find the renderer to scroll to, and scroll from here to methods on FrameView, and replaced it with a call to FrameView::maintainScrollPositionAtAnchor(). (WebCore::FrameLoader::completed): Call maintainScrollPositionAtAnchor() instead of setLockedToAnchor().

• page/FrameView.cpp: (WebCore::FrameView::FrameView): Removed initialization of m_lockedToAnchor. (WebCore::FrameView::reset): Reset m_maintainScrollPositionAnchor instead of m_lockedToAnchor. (WebCore::FrameView::layout): Removed the code related to scrolling to the anchor from here, because scrolling can trigger events which invalidate the layout, and as such, belongs with the post-layout tasks. (WebCore::FrameView::maintainScrollPositionAtAnchor): Added. When called with a node scrolls the view to the top of that node and maintains it scrolled to the top of the node during subsequent layouts, until this function is called with 0 or other things trigger scrolling. (WebCore::FrameView::scrollRectIntoViewRecursively): Reset m_maintainScrollPositionAnchor. (WebCore::FrameView::setScrollPosition): Ditto. (WebCore::FrameView::scrollToAnchor): Added. Scrolls to the top of m_maintainScrollPositionAnchor, if it is set. (WebCore::FrameView::performPostLayoutTasks): Call scrollToAnchor(). (WebCore::FrameView::setWasScrolledByUser): Reset m_maintainScrollPositionAnchor.

• page/FrameView.h: Removed lockedToAnchor(), setLockedToAnchor(), and m_lockedToAnchor. Added maintainScrollPositionAtAnchor() and m_maintainScrollPositionAnchor.

LayoutTests:

Reviewed by Simon Fraser.

• test for ​https://bugs.webkit.org/show_bug.cgi?id=27137 <rdar://problem/7043124> REGRESSION (r44311): Reproducible crash due to infinite recursion into FrameLoader::gotoAnchor() -> FrameView::layout()

• fast/loader/goto-anchor-infinite-layout-expected.txt: Added.
• fast/loader/goto-anchor-infinite-layout.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/loader/goto-anchor-infinite-layout-expected.txt (added)
• LayoutTests/fast/loader/goto-anchor-infinite-layout.html (added)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/loader/FrameLoader.cpp (modified) (2 diffs)
• WebCore/page/FrameView.cpp (modified) (8 diffs)
• WebCore/page/FrameView.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2009-07-10  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Simon Fraser.
+
+        - test for https://bugs.webkit.org/show_bug.cgi?id=27137
+          <rdar://problem/7043124> REGRESSION (r44311): Reproducible crash due
+          to infinite recursion into FrameLoader::gotoAnchor() ->
+          FrameView::layout()
+
+        * fast/loader/goto-anchor-infinite-layout-expected.txt: Added.
+        * fast/loader/goto-anchor-infinite-layout.html: Added.
+
 2009-07-09  Simon Hausmann  <hausmann@webkit.org>
 

trunk/WebCore/ChangeLog

@@ +1 @@
+2009-07-10  Dan Bernstein  <mitz@apple.com>
+
+        Reviewed by Simon Fraser.
+
+        - fix https://bugs.webkit.org/show_bug.cgi?id=27137
+          <rdar://problem/7043124> REGRESSION (r44311): Reproducible crash due
+          to infinite recursion into FrameLoader::gotoAnchor() ->
+          FrameView::layout()
+
+        Test: fast/loader/goto-anchor-infinite-layout.html
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::gotoAnchor): Moved the code to update layout,
+        find the renderer to scroll to, and scroll from here to methods on
+        FrameView, and replaced it with a call to
+        FrameView::maintainScrollPositionAtAnchor().
+        (WebCore::FrameLoader::completed): Call maintainScrollPositionAtAnchor()
+        instead of setLockedToAnchor().
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::FrameView): Removed initialization of
+        m_lockedToAnchor.
+        (WebCore::FrameView::reset): Reset m_maintainScrollPositionAnchor instead
+        of m_lockedToAnchor.
+        (WebCore::FrameView::layout): Removed the code related to scrolling to
+        the anchor from here, because scrolling can trigger events which
+        invalidate the layout, and as such, belongs with the post-layout tasks.
+        (WebCore::FrameView::maintainScrollPositionAtAnchor): Added. When called
+        with a node scrolls the view to the top of that node and maintains it
+        scrolled to the top of the node during subsequent layouts, until
+        this function is called with 0 or other things trigger scrolling.
+        (WebCore::FrameView::scrollRectIntoViewRecursively): Reset
+        m_maintainScrollPositionAnchor.
+        (WebCore::FrameView::setScrollPosition): Ditto.
+        (WebCore::FrameView::scrollToAnchor): Added. Scrolls to the top of
+        m_maintainScrollPositionAnchor, if it is set.
+        (WebCore::FrameView::performPostLayoutTasks): Call scrollToAnchor().
+        (WebCore::FrameView::setWasScrolledByUser): Reset
+        m_maintainScrollPositionAnchor.
+
+        * page/FrameView.h: Removed lockedToAnchor(), setLockedToAnchor(),
+        and m_lockedToAnchor. Added maintainScrollPositionAtAnchor() and
+        m_maintainScrollPositionAnchor.
+
 2009-07-04  Sriram Yadavalli  <sriram.yadavalli@nokia.com>
 

trunk/WebCore/loader/FrameLoader.cpp

@@ -1590 +1590 @@
         return false;
 
-    // We need to update the layout before scrolling, otherwise we could
-    // really mess things up if an anchor scroll comes at a bad moment.
-    m_frame->document()->updateStyleIfNeeded();
-    // Only do a layout if changes have occurred that make it necessary.
-    if (m_frame->view() && m_frame->contentRenderer() && m_frame->contentRenderer()->needsLayout())
-        m_frame->view()->layout();
-  
-    // Scroll nested layers and frames to reveal the anchor.
-    // Align to the top and to the closest side (this matches other browsers).
-    RenderObject* renderer;
-    IntRect rect;
-    if (!anchorNode)
-        renderer = m_frame->document()->renderer(); // top of document
-    else {
-        renderer = anchorNode->renderer();
-        rect = anchorNode->getRect();
-    }
-    if (renderer) {
-        renderer->enclosingLayer()->scrollRectToVisible(rect, true, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);
-        if (m_frame->view())
-            m_frame->view()->setLockedToAnchor(true);
-    }
+    if (FrameView* view = m_frame->view())
+        view->maintainScrollPositionAtAnchor(anchorNode ? static_cast<Node*>(anchorNode) : m_frame->document());
 
     return true;
@@ -2115 +2095 @@
         parent->loader()->checkCompleted();
     if (m_frame->view())
-        m_frame->view()->setLockedToAnchor(false);
+        m_frame->view()->maintainScrollPositionAtAnchor(0);
 }
 

trunk/WebCore/page/FrameView.cpp

@@ -112 +112 @@
     , m_deferSetNeedsLayouts(0)
     , m_setNeedsLayoutWasDeferred(false)
-    , m_lockedToAnchor(false)
 {
     init();
@@ -186 +185 @@
     m_isVisuallyNonEmpty = false;
     m_firstVisuallyNonEmptyLayoutCallbackPending = true;
-    m_lockedToAnchor = false;
+    m_maintainScrollPositionAnchor = 0;
 }
 
@@ -667 +666 @@
     }
 
-    if (lockedToAnchor())
-        m_frame->loader()->gotoAnchor();
-
     m_nestedLayoutCount--;
 }
@@ -752 +748 @@
 }
 
+void FrameView::maintainScrollPositionAtAnchor(Node* anchorNode)
+{
+    m_maintainScrollPositionAnchor = anchorNode;
+    if (!m_maintainScrollPositionAnchor)
+        return;
+
+    // We need to update the layout before scrolling, otherwise we could
+    // really mess things up if an anchor scroll comes at a bad moment.
+    m_frame->document()->updateStyleIfNeeded();
+    // Only do a layout if changes have occurred that make it necessary.
+    if (m_frame->contentRenderer() && m_frame->contentRenderer()->needsLayout())
+        layout();
+    else
+        scrollToAnchor();
+}
+
 void FrameView::scrollRectIntoViewRecursively(const IntRect& r)
 {
     bool wasInProgrammaticScroll = m_inProgrammaticScroll;
     m_inProgrammaticScroll = true;
-    setLockedToAnchor(false);
+    m_maintainScrollPositionAnchor = 0;
     ScrollView::scrollRectIntoViewRecursively(r);
     m_inProgrammaticScroll = wasInProgrammaticScroll;
@@ -765 +777 @@
     bool wasInProgrammaticScroll = m_inProgrammaticScroll;
     m_inProgrammaticScroll = true;
-    setLockedToAnchor(false);
+    m_maintainScrollPositionAnchor = 0;
     ScrollView::setScrollPosition(scrollPoint);
     m_inProgrammaticScroll = wasInProgrammaticScroll;
@@ -1119 +1131 @@
 }
 
+void FrameView::scrollToAnchor()
+{
+    RefPtr<Node> anchorNode = m_maintainScrollPositionAnchor;
+    if (!anchorNode)
+        return;
+
+    if (!anchorNode->renderer())
+        return;
+
+    IntRect rect;
+    if (anchorNode != m_frame->document())
+        rect = anchorNode->getRect();
+
+    // Scroll nested layers and frames to reveal the anchor.
+    // Align to the top and to the closest side (this matches other browsers).
+    anchorNode->renderer()->enclosingLayer()->scrollRectToVisible(rect, true, ScrollAlignment::alignToEdgeIfNeeded, ScrollAlignment::alignTopAlways);
+
+    // scrollRectToVisible can call into scrollRectIntoViewRecursively(), which resets m_maintainScrollPositionAnchor.
+    m_maintainScrollPositionAnchor = anchorNode;
+}
+
 bool FrameView::updateWidgets()
 {
@@ -1162 +1195 @@
             break;
     }
-    
+
+    scrollToAnchor();
+
     resumeScheduledEvents();
 
@@ -1352 +1387 @@
     if (m_inProgrammaticScroll)
         return;
-    setLockedToAnchor(false);
+    m_maintainScrollPositionAnchor = 0;
     m_wasScrolledByUser = wasScrolledByUser;
 }

trunk/WebCore/page/FrameView.h

@@ -180 +180 @@
     void adjustPageHeight(float* newBottom, float oldTop, float oldBottom, float bottomLimit);
 
-    bool lockedToAnchor() { return m_lockedToAnchor; }
-    void setLockedToAnchor(bool lockedToAnchor) { m_lockedToAnchor = lockedToAnchor; }
+    void maintainScrollPositionAtAnchor(Node*);
 
     // Methods to convert points and rects between the coordinate space of the renderer, and this view.
@@ -234 +233 @@
 
     bool updateWidgets();
+    void scrollToAnchor();
     
     static double sCurrentPaintTimeStamp; // used for detecting decoded resource thrash in the cache
@@ -303 +303 @@
     bool m_firstVisuallyNonEmptyLayoutCallbackPending;
 
-    bool m_lockedToAnchor;
+    RefPtr<Node> m_maintainScrollPositionAnchor;
 };