- Revision
- 88982
- Author
- [email protected]
- Date
- 2011-06-15 16:13:23 -0700 (Wed, 15 Jun 2011)
Log Message
https://bugs.webkit.org/show_bug.cgi?id=62746
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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (88981 => 88982)
--- trunk/Source/WebCore/ChangeLog 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/ChangeLog 2011-06-15 23:13:23 UTC (rev 88982)
@@ -1,3 +1,68 @@
+2011-06-15 Beth Dakin <[email protected]>
+
+ 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 <[email protected]>
Reviewed by Dan Bernstein.
Modified: trunk/Source/WebCore/dom/Document.cpp (88981 => 88982)
--- trunk/Source/WebCore/dom/Document.cpp 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/dom/Document.cpp 2011-06-15 23:13:23 UTC (rev 88982)
@@ -3820,9 +3820,6 @@
if (childNeedsStyleRecalc())
scheduleStyleRecalc();
}
-
- if (v)
- v->setAnimatorsAreActive(!m_inPageCache);
}
void Document::documentWillBecomeInactive()
@@ -3848,6 +3845,9 @@
renderView()->didMoveOnscreen();
#endif
+ if (FrameView* frameView = view())
+ frameView->setAnimatorsAreActive();
+
ASSERT(m_frame);
m_frame->loader()->client()->dispatchDidBecomeFrameset(isFrameSet());
}
Modified: trunk/Source/WebCore/page/FrameView.cpp (88981 => 88982)
--- trunk/Source/WebCore/page/FrameView.cpp 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/page/FrameView.cpp 2011-06-15 23:13:23 UTC (rev 88982)
@@ -2226,12 +2226,19 @@
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 @@
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
Modified: trunk/Source/WebCore/page/FrameView.h (88981 => 88982)
--- trunk/Source/WebCore/page/FrameView.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/page/FrameView.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -277,7 +277,7 @@
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 @@
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;
Modified: trunk/Source/WebCore/platform/ScrollAnimator.cpp (88981 => 88982)
--- trunk/Source/WebCore/platform/ScrollAnimator.cpp 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/platform/ScrollAnimator.cpp 2011-06-15 23:13:23 UTC (rev 88982)
@@ -52,7 +52,6 @@
: m_scrollableArea(scrollableArea)
, m_currentPosX(0)
, m_currentPosY(0)
- , m_isActive(true)
{
}
Modified: trunk/Source/WebCore/platform/ScrollAnimator.h (88981 => 88982)
--- trunk/Source/WebCore/platform/ScrollAnimator.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/platform/ScrollAnimator.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -61,8 +61,7 @@
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 @@
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
Modified: trunk/Source/WebCore/platform/ScrollableArea.h (88981 => 88982)
--- trunk/Source/WebCore/platform/ScrollableArea.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/platform/ScrollableArea.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -136,6 +136,8 @@
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(); }
Modified: trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.h (88981 => 88982)
--- trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -83,7 +83,7 @@
bool haveScrolledSincePageLoad() const { return m_haveScrolledSincePageLoad; }
- virtual void setIsActive(bool);
+ virtual void setIsActive();
#if USE(WK_SCROLLBAR_PAINTER)
void updateScrollerStyle();
Modified: trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm (88981 => 88982)
--- trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/platform/mac/ScrollAnimatorMac.mm 2011-06-15 23:13:23 UTC (rev 88982)
@@ -1216,10 +1216,8 @@
}
#endif
-void ScrollAnimatorMac::setIsActive(bool active)
+void ScrollAnimatorMac::setIsActive()
{
- ScrollAnimator::setIsActive(active);
-
#if USE(WK_SCROLLBAR_PAINTER)
if (needsScrollerStyleUpdate())
updateScrollerStyle();
@@ -1229,7 +1227,7 @@
#if USE(WK_SCROLLBAR_PAINTER)
void ScrollAnimatorMac::updateScrollerStyle()
{
- if (!isActive()) {
+ if (!scrollableArea()->isOnActivePage()) {
setNeedsScrollerStyleUpdate(true);
return;
}
Modified: trunk/Source/WebCore/rendering/RenderLayer.cpp (88981 => 88982)
--- trunk/Source/WebCore/rendering/RenderLayer.cpp 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/rendering/RenderLayer.cpp 2011-06-15 23:13:23 UTC (rev 88982)
@@ -1817,6 +1817,11 @@
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();
Modified: trunk/Source/WebCore/rendering/RenderLayer.h (88981 => 88982)
--- trunk/Source/WebCore/rendering/RenderLayer.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/rendering/RenderLayer.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -546,6 +546,7 @@
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;
Modified: trunk/Source/WebCore/rendering/RenderListBox.cpp (88981 => 88982)
--- trunk/Source/WebCore/rendering/RenderListBox.cpp 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/rendering/RenderListBox.cpp 2011-06-15 23:13:23 UTC (rev 88982)
@@ -801,6 +801,11 @@
return view->frameView()->shouldSuspendScrollAnimations();
}
+bool RenderListBox::isOnActivePage() const
+{
+ return !document()->inPageCache();
+}
+
PassRefPtr<Scrollbar> RenderListBox::createScrollbar()
{
RefPtr<Scrollbar> widget;
Modified: trunk/Source/WebCore/rendering/RenderListBox.h (88981 => 88982)
--- trunk/Source/WebCore/rendering/RenderListBox.h 2011-06-15 23:08:56 UTC (rev 88981)
+++ trunk/Source/WebCore/rendering/RenderListBox.h 2011-06-15 23:13:23 UTC (rev 88982)
@@ -117,6 +117,7 @@
virtual int visibleWidth() const;
virtual IntPoint currentMousePosition() const;
virtual bool shouldSuspendScrollAnimations() const;
+ virtual bool isOnActivePage() const;
virtual void disconnectFromPage() { m_page = 0; }
