https://bugs.webkit.org/show_bug.cgi?id=62746
authorbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Jun 2011 23:13:23 +0000 (23:13 +0000)
committerbdakin@apple.com <bdakin@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Jun 2011 23:13:23 +0000 (23:13 +0000)
Crash possible when switching scrollbar appearance preference on Mac
-and corresponding-
<rdar://problem/9323983>

Reviewed by Simon Fraser.

This crash happens because the current mechanism that is intended to flag
ScrollAnimators as being in the page cache or not does not work correctly.
Long-term the fix for this is to move the ScrollableArea HashSet to a more
appropriate place. In the meantime, this patch addresses the crash by getting
rid of the m_isActive bool on ScrollAnimator that was intended to represent
whether or not the ScrollableArea is in the page cache. Instead, ScrollableArea
implementations now have their own functions to compute whether they are in
active pages. ScrollAnimator::setIsActive() needs to be kept around even though
there is no bool to flip anymore because scrollbars may need to be properly
updated if the appearance was switched while the document was in the page cache.

No longer call FrameView::setAnimatorsAreActive() from
Document::setIsInPageCache(), instead call it in
Document::documentDidBecomeActive()
* dom/Document.cpp:
(WebCore::Document::setInPageCache):
(WebCore::Document::documentDidBecomeActive):

ScrollableAreas can now assess whether or not they are on active pages (ie, not
in the page cache).
* platform/ScrollableArea.h:
(WebCore::ScrollableArea::isOnActivePage):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::isOnActivePage):
* rendering/RenderLayer.h:
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::isOnActivePage):
* rendering/RenderListBox.h:

A FrameView cannot access its Document when it's in the page cache, so it
usually determines whether it's in the page cache by checking if its frame
points to a FrameView other than itself.
* page/FrameView.cpp:
(WebCore::FrameView::isOnActivePage):

Make sure ScrollableAreas are on active pages before setting them as
active. This will not be necessary when the HashSet become a per-web page
HashSet.
(WebCore::FrameView::setAnimatorsAreActive):
* page/FrameView.h:

ScrollAnimator no longer tracks the m_isActive bool.
* platform/ScrollAnimator.cpp:
(WebCore::ScrollAnimator::ScrollAnimator):
* platform/ScrollAnimator.h:
(WebCore::ScrollAnimator::setIsActive):

setIsActive() now exclusively calls updateScrollStyle() if there is a pending
need to do so.
* platform/mac/ScrollAnimatorMac.h:
* platform/mac/ScrollAnimatorMac.mm:
(WebCore::ScrollAnimatorMac::setIsActive):

Return early if the ScrollableArea is in the page cache.
(WebCore::ScrollAnimatorMac::updateScrollerStyle):

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

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/platform/ScrollAnimator.cpp
Source/WebCore/platform/ScrollAnimator.h
Source/WebCore/platform/ScrollableArea.h
Source/WebCore/platform/mac/ScrollAnimatorMac.h
Source/WebCore/platform/mac/ScrollAnimatorMac.mm
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h
Source/WebCore/rendering/RenderListBox.cpp
Source/WebCore/rendering/RenderListBox.h

index bb4e396..ef4f342 100644 (file)
@@ -1,3 +1,68 @@
+2011-06-15  Beth Dakin  <bdakin@apple.com>
+
+        Reviewed by Simon Fraser.
+
+        https://bugs.webkit.org/show_bug.cgi?id=62746
+        Crash possible when switching scrollbar appearance preference on Mac
+        -and corresponding-
+        <rdar://problem/9323983>
+        
+        This crash happens because the current mechanism that is intended to flag 
+        ScrollAnimators as being in the page cache or not does not work correctly. 
+        Long-term the fix for this is to move the ScrollableArea HashSet to a more 
+        appropriate place. In the meantime, this patch addresses the crash by getting 
+        rid of the m_isActive bool on ScrollAnimator that was intended to represent 
+        whether or not the ScrollableArea is in the page cache. Instead, ScrollableArea 
+        implementations now have their own functions to compute whether they are in 
+        active pages. ScrollAnimator::setIsActive() needs to be kept around even though 
+        there is no bool to flip anymore because scrollbars may need to be properly 
+        updated if the appearance was switched while the document was in the page cache.
+
+        No longer call FrameView::setAnimatorsAreActive() from 
+        Document::setIsInPageCache(), instead call it in 
+        Document::documentDidBecomeActive()
+        * dom/Document.cpp:
+        (WebCore::Document::setInPageCache):
+        (WebCore::Document::documentDidBecomeActive):
+
+        ScrollableAreas can now assess whether or not they are on active pages (ie, not 
+        in the page cache).
+        * platform/ScrollableArea.h:
+        (WebCore::ScrollableArea::isOnActivePage):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::isOnActivePage):
+        * rendering/RenderLayer.h:
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::isOnActivePage):
+        * rendering/RenderListBox.h:
+
+        A FrameView cannot access its Document when it's in the page cache, so it 
+        usually determines whether it's in the page cache by checking if its frame 
+        points to a FrameView other than itself.
+        * page/FrameView.cpp:
+        (WebCore::FrameView::isOnActivePage):
+        
+        Make sure ScrollableAreas are on active pages before setting them as 
+        active. This will not be necessary when the HashSet become a per-web page 
+        HashSet.
+        (WebCore::FrameView::setAnimatorsAreActive):
+        * page/FrameView.h:
+        
+        ScrollAnimator no longer tracks the m_isActive bool. 
+        * platform/ScrollAnimator.cpp:
+        (WebCore::ScrollAnimator::ScrollAnimator):
+        * platform/ScrollAnimator.h:
+        (WebCore::ScrollAnimator::setIsActive):
+        
+        setIsActive() now exclusively calls updateScrollStyle() if there is a pending 
+        need to do so.
+        * platform/mac/ScrollAnimatorMac.h:
+        * platform/mac/ScrollAnimatorMac.mm:
+        (WebCore::ScrollAnimatorMac::setIsActive):
+        
+        Return early if the ScrollableArea is in the page cache. 
+        (WebCore::ScrollAnimatorMac::updateScrollerStyle):
+
 2011-06-15  Simon Fraser  <simon.fraser@apple.com>
 
         Reviewed by Dan Bernstein.
index c962a32..6bdb6c1 100644 (file)
@@ -3820,9 +3820,6 @@ void Document::setInPageCache(bool flag)
         if (childNeedsStyleRecalc())
             scheduleStyleRecalc();
     }
-
-    if (v)
-        v->setAnimatorsAreActive(!m_inPageCache);
 }
 
 void Document::documentWillBecomeInactive() 
@@ -3848,6 +3845,9 @@ void Document::documentDidBecomeActive()
         renderView()->didMoveOnscreen();
 #endif
 
+    if (FrameView* frameView = view())
+        frameView->setAnimatorsAreActive();
+
     ASSERT(m_frame);
     m_frame->loader()->client()->dispatchDidBecomeFrameset(isFrameSet());
 }
index 48837a8..1865018 100644 (file)
@@ -2226,12 +2226,19 @@ void FrameView::setVisibleScrollerThumbRect(const IntRect& scrollerThumb)
     return page->chrome()->client()->notifyScrollerThumbIsVisibleInRect(scrollerThumb);
 }
 
+bool FrameView::isOnActivePage() const
+{
+    if (m_frame->view() != this)
+        return false;
+    return !m_frame->document()->inPageCache();
+}
+
 bool FrameView::shouldSuspendScrollAnimations() const
 {
     return m_frame->loader()->state() != FrameStateComplete;
 }
 
-void FrameView::setAnimatorsAreActive(bool active)
+void FrameView::setAnimatorsAreActive()
 {
     Page* page = m_frame->page();
     if (!page)
@@ -2242,8 +2249,14 @@ void FrameView::setAnimatorsAreActive(bool active)
         return;
 
     HashSet<ScrollableArea*>::const_iterator end = scrollableAreas->end(); 
-    for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it)
-        (*it)->scrollAnimator()->setIsActive(active);
+    for (HashSet<ScrollableArea*>::const_iterator it = scrollableAreas->begin(); it != end; ++it) {
+        // FIXME: This extra check to make sure the ScrollableArea is on an active page needs 
+        // to be here as long as the ScrollableArea HashSet lives on Page. But it should really be
+        // moved to the top-level Document or a similar class that really represents a single 
+        // web page. https://bugs.webkit.org/show_bug.cgi?id=62762
+        if ((*it)->isOnActivePage())
+            (*it)->scrollAnimator()->setIsActive();
+    }
 }
 
 void FrameView::notifyPageThatContentAreaWillPaint() const
index c530cef..04f51b7 100644 (file)
@@ -277,7 +277,7 @@ public:
 
     virtual bool shouldSuspendScrollAnimations() const;
 
-    void setAnimatorsAreActive(bool);
+    void setAnimatorsAreActive();
 
 protected:
     virtual bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect);
@@ -328,6 +328,7 @@ private:
     virtual void didStartAnimatedScroll() const;
     virtual void didCompleteAnimatedScroll() const;
     virtual void setVisibleScrollerThumbRect(const IntRect&);
+    virtual bool isOnActivePage() const;
 #if USE(ACCELERATED_COMPOSITING)
     virtual GraphicsLayer* layerForHorizontalScrollbar() const;
     virtual GraphicsLayer* layerForVerticalScrollbar() const;
index b7b0a41..45cce39 100644 (file)
@@ -52,7 +52,6 @@ ScrollAnimator::ScrollAnimator(ScrollableArea* scrollableArea)
     : m_scrollableArea(scrollableArea)
     , m_currentPosX(0)
     , m_currentPosY(0)
-    , m_isActive(true)
 {
 }
 
index 483698b..1c44191 100644 (file)
@@ -61,8 +61,7 @@ public:
 
     ScrollableArea* scrollableArea() const { return m_scrollableArea; }
 
-    virtual void setIsActive(bool active) { m_isActive = active; }
-    bool isActive() const { return m_isActive; }
+    virtual void setIsActive() { }
 
     virtual void handleWheelEvent(PlatformWheelEvent&);
 #if ENABLE(GESTURE_EVENTS)
@@ -96,7 +95,6 @@ protected:
     ScrollableArea* m_scrollableArea;
     float m_currentPosX; // We avoid using a FloatPoint in order to reduce
     float m_currentPosY; // subclass code complexity.
-    bool m_isActive;
 };
 
 } // namespace WebCore
index e919cee..36131fd 100644 (file)
@@ -136,6 +136,8 @@ public:
     virtual bool shouldSuspendScrollAnimations() const { return true; }
     virtual void scrollbarStyleChanged() { }
     virtual void setVisibleScrollerThumbRect(const IntRect&) { }
+
+    virtual bool isOnActivePage() const { ASSERT_NOT_REACHED(); return true; }
     
     bool isHorizontalScrollerPinnedToMinimumPosition() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) <= minimumScrollPosition().x(); }
     bool isHorizontalScrollerPinnedToMaximumPosition() const { return !horizontalScrollbar() || scrollPosition(horizontalScrollbar()) >= maximumScrollPosition().x(); }
index 09fd333..f5f137e 100644 (file)
@@ -83,7 +83,7 @@ public:
 
     bool haveScrolledSincePageLoad() const { return m_haveScrolledSincePageLoad; }
 
-    virtual void setIsActive(bool);
+    virtual void setIsActive();
 
 #if USE(WK_SCROLLBAR_PAINTER)
     void updateScrollerStyle();
index 80e0943..85b0f7c 100644 (file)
@@ -1216,10 +1216,8 @@ void ScrollAnimatorMac::snapRubberBandTimerFired(Timer<ScrollAnimatorMac>*)
 }
 #endif
 
-void ScrollAnimatorMac::setIsActive(bool active)
+void ScrollAnimatorMac::setIsActive()
 {
-    ScrollAnimator::setIsActive(active);
-
 #if USE(WK_SCROLLBAR_PAINTER)
     if (needsScrollerStyleUpdate())
         updateScrollerStyle();
@@ -1229,7 +1227,7 @@ void ScrollAnimatorMac::setIsActive(bool active)
 #if USE(WK_SCROLLBAR_PAINTER)
 void ScrollAnimatorMac::updateScrollerStyle()
 {
-    if (!isActive()) {
+    if (!scrollableArea()->isOnActivePage()) {
         setNeedsScrollerStyleUpdate(true);
         return;
     }
index a5679a1..f7f2481 100644 (file)
@@ -1817,6 +1817,11 @@ bool RenderLayer::shouldSuspendScrollAnimations() const
     return view->frameView()->shouldSuspendScrollAnimations();
 }
 
+bool RenderLayer::isOnActivePage() const
+{
+    return !m_renderer->document()->inPageCache();
+}
+
 IntPoint RenderLayer::currentMousePosition() const
 {
     return renderer()->frame() ? renderer()->frame()->eventHandler()->currentMousePosition() : IntPoint();
index 7a8298f..8138ece 100644 (file)
@@ -546,6 +546,7 @@ private:
     virtual IntPoint currentMousePosition() const;
     virtual void didCompleteRubberBand(const IntSize&) const;
     virtual bool shouldSuspendScrollAnimations() const;
+    virtual bool isOnActivePage() const;
 
     // Rectangle encompassing the scroll corner and resizer rect.
     IntRect scrollCornerAndResizerRect() const;
index 735b641..5621967 100644 (file)
@@ -801,6 +801,11 @@ bool RenderListBox::shouldSuspendScrollAnimations() const
     return view->frameView()->shouldSuspendScrollAnimations();
 }
 
+bool RenderListBox::isOnActivePage() const
+{
+    return !document()->inPageCache();
+}
+
 PassRefPtr<Scrollbar> RenderListBox::createScrollbar()
 {
     RefPtr<Scrollbar> widget;
index 0d0828e..6772858 100644 (file)
@@ -117,6 +117,7 @@ private:
     virtual int visibleWidth() const;
     virtual IntPoint currentMousePosition() const;
     virtual bool shouldSuspendScrollAnimations() const;
+    virtual bool isOnActivePage() const;
 
     virtual void disconnectFromPage() { m_page = 0; }