Changeset 188298 in webkit

Timestamp:

Aug 11, 2015 2:50:32 PM (9 years ago)

Author:

Alan Bujtas

Message:

Invalid FrameView::m_viewportRenderer after layout is finished.
​https://bugs.webkit.org/show_bug.cgi?id=147848
rdar://problem/22205197

Reviewed by Simon Fraser.

We cache the current viewport renderer (FrameView::m_viewportRenderer) right before layout.
It gets dereferenced later when layout is finished to update the overflow status.
If the viewport renderer gets destroyed during layout, we end up with a dangling pointer.
This patch replaces the pointer caching with type caching (none, body, document).

Unable to construct a test case.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/FrameView.cpp (modified) (10 diffs)
• page/FrameView.h (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-08-11  Zalan Bujtas  <zalan@apple.com>
+
+        Invalid FrameView::m_viewportRenderer after layout is finished.
+        https://bugs.webkit.org/show_bug.cgi?id=147848
+        rdar://problem/22205197
+
+        Reviewed by Simon Fraser.
+
+        We cache the current viewport renderer (FrameView::m_viewportRenderer) right before layout.
+        It gets dereferenced later when layout is finished to update the overflow status.
+        If the viewport renderer gets destroyed during layout, we end up with a dangling pointer.
+        This patch replaces the pointer caching with type caching (none, body, document).
+
+        Unable to construct a test case.
+
 2015-08-11  Brent Fulgham  <bfulgham@apple.com>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -173 +173 @@
     , m_mediaType("screen")
     , m_overflowStatusDirty(true)
-    , m_viewportRenderer(nullptr)
     , m_wasScrolledByUser(false)
     , m_inProgrammaticScroll(false)
@@ -622 +621 @@
 {
     RenderView* renderView = this->renderView();
-    if (!renderView || !m_viewportRenderer || !is<RenderBox>(m_viewportRenderer) || !frame().isMainFrame())
+    auto* viewportRenderer = this->viewportRenderer();
+    if (!renderView || !is<RenderBox>(viewportRenderer) || !frame().isMainFrame())
         return contentsSize();
 
@@ -628 +628 @@
 
     FloatRect contentRect = renderView->unscaledDocumentRect();
-    RenderBox& viewportRendererBox = downcast<RenderBox>(*m_viewportRenderer);
-
-    if (m_viewportRenderer->style().overflowX() == OHIDDEN)
+    RenderBox& viewportRendererBox = downcast<RenderBox>(*viewportRenderer);
+
+    if (viewportRendererBox.style().overflowX() == OHIDDEN)
         contentRect.setWidth(std::min<float>(contentRect.width(), viewportRendererBox.frameRect().width()));
 
-    if (m_viewportRenderer->style().overflowY() == OHIDDEN)
+    if (viewportRendererBox.style().overflowY() == OHIDDEN)
         contentRect.setHeight(std::min<float>(contentRect.height(), viewportRendererBox.frameRect().height()));
 
@@ -642 +642 @@
 }
 
-void FrameView::applyOverflowToViewport(RenderElement* renderer, ScrollbarMode& hMode, ScrollbarMode& vMode)
+void FrameView::applyOverflowToViewport(const RenderElement& renderer, ScrollbarMode& hMode, ScrollbarMode& vMode)
 {
     // Handle the overflow:hidden/scroll case for the body/html elements.  WinIE treats
@@ -655 +655 @@
     bool overrideHidden = frame().isMainFrame() && ((frame().frameScaleFactor() > 1) || headerHeight() || footerHeight());
 
-    EOverflow overflowX = renderer->style().overflowX();
-    EOverflow overflowY = renderer->style().overflowY();
-
-    if (is<RenderSVGRoot>(*renderer)) {
+    EOverflow overflowX = renderer.style().overflowX();
+    EOverflow overflowY = renderer.style().overflowY();
+
+    if (is<RenderSVGRoot>(renderer)) {
         // FIXME: evaluate if we can allow overflow for these cases too.
         // Overflow is always hidden when stand-alone SVG documents are embedded.
-        if (downcast<RenderSVGRoot>(*renderer).isEmbeddedThroughFrameContainingSVGDocument()) {
+        if (downcast<RenderSVGRoot>(renderer).isEmbeddedThroughFrameContainingSVGDocument()) {
             overflowX = OHIDDEN;
             overflowY = OHIDDEN;
@@ -702 +702 @@
             ;
     }
-
-    m_viewportRenderer = renderer;
 }
 
@@ -734 +732 @@
 void FrameView::calculateScrollbarModesForLayout(ScrollbarMode& hMode, ScrollbarMode& vMode, ScrollbarModesCalculationStrategy strategy)
 {
-    m_viewportRenderer = nullptr;
+    m_viewportRendererType = ViewportRendererType::None;
 
     const HTMLFrameOwnerElement* owner = frame().ownerElement();
@@ -751 +749 @@
     }
     
-    if (!m_layoutRoot) {
-        Document* document = frame().document();
-        auto documentElement = document->documentElement();
-        RenderElement* rootRenderer = documentElement ? documentElement->renderer() : nullptr;
-        auto* body = document->bodyOrFrameset();
-        if (body && body->renderer()) {
-            if (is<HTMLFrameSetElement>(*body) && !frameFlatteningEnabled()) {
-                vMode = ScrollbarAlwaysOff;
-                hMode = ScrollbarAlwaysOff;
-            } else if (is<HTMLBodyElement>(*body)) {
-                // It's sufficient to just check the X overflow,
-                // since it's illegal to have visible in only one direction.
-                RenderElement* o = rootRenderer->style().overflowX() == OVISIBLE && is<HTMLHtmlElement>(*document->documentElement()) ? body->renderer() : rootRenderer;
-                applyOverflowToViewport(o, hMode, vMode);
+    if (m_layoutRoot)
+        return;
+    
+    auto* document = frame().document();
+    if (!document)
+        return;
+
+    auto documentElement = document->documentElement();
+    if (!documentElement)
+        return;
+
+    auto* bodyOrFrameset = document->bodyOrFrameset();
+    auto* rootRenderer = documentElement->renderer();
+    if (!bodyOrFrameset || !bodyOrFrameset->renderer()) {
+        if (rootRenderer) {
+            applyOverflowToViewport(*rootRenderer, hMode, vMode);
+            m_viewportRendererType = ViewportRendererType::Document;
+        }
+        return;
+    }
+    
+    if (is<HTMLFrameSetElement>(*bodyOrFrameset) && !frameFlatteningEnabled()) {
+        vMode = ScrollbarAlwaysOff;
+        hMode = ScrollbarAlwaysOff;
+        return;
+    }
+
+    if (is<HTMLBodyElement>(*bodyOrFrameset) && rootRenderer) {
+        // It's sufficient to just check the X overflow,
+        // since it's illegal to have visible in only one direction.
+        if (rootRenderer->style().overflowX() == OVISIBLE && is<HTMLHtmlElement>(documentElement)) {
+            auto* bodyRenderer = bodyOrFrameset->renderer();
+            if (bodyRenderer) {
+                applyOverflowToViewport(*bodyRenderer, hMode, vMode);
+                m_viewportRendererType = ViewportRendererType::Body;
             }
-        } else if (rootRenderer)
-            applyOverflowToViewport(rootRenderer, hMode, vMode);
-    }    
+        } else {
+            applyOverflowToViewport(*rootRenderer, hMode, vMode);
+            m_viewportRendererType = ViewportRendererType::Document;
+        }
+    }
 }
 
@@ -3264 +3285 @@
 }
 
+RenderElement* FrameView::viewportRenderer() const
+{
+    if (m_viewportRendererType == ViewportRendererType::None)
+        return nullptr;
+
+    auto* document = frame().document();
+    if (!document)
+        return nullptr;
+
+    if (m_viewportRendererType == ViewportRendererType::Document) {
+        auto* documentElement = document->documentElement();
+        if (!documentElement)
+            return nullptr;
+        return documentElement->renderer();
+    }
+
+    if (m_viewportRendererType == ViewportRendererType::Body) {
+        auto* body = document->body();
+        if (!body)
+            return nullptr;
+        return body->renderer();
+    }
+
+    ASSERT_NOT_REACHED();
+    return nullptr;
+}
+
 void FrameView::updateOverflowStatus(bool horizontalOverflow, bool verticalOverflow)
 {
-    if (!m_viewportRenderer)
+    auto* viewportRenderer = this->viewportRenderer();
+    if (!viewportRenderer)
         return;
     
@@ -3285 +3334 @@
         RefPtr<OverflowEvent> overflowEvent = OverflowEvent::create(horizontalOverflowChanged, horizontalOverflow,
             verticalOverflowChanged, verticalOverflow);
-        overflowEvent->setTarget(m_viewportRenderer->element());
+        overflowEvent->setTarget(viewportRenderer->element());
 
         frame().document()->enqueueOverflowEvent(overflowEvent.release());

trunk/Source/WebCore/page/FrameView.h

@@ -595 +595 @@
     virtual void scrollPositionChangedViaPlatformWidgetImpl(const IntPoint& oldPosition, const IntPoint& newPosition) override;
 
-    void applyOverflowToViewport(RenderElement*, ScrollbarMode& hMode, ScrollbarMode& vMode);
+    void applyOverflowToViewport(const RenderElement&, ScrollbarMode& hMode, ScrollbarMode& vMode);
     void applyPaginationToViewport();
 
@@ -676 +676 @@
     void notifyWidgets(WidgetNotification);
 
+    RenderElement* viewportRenderer() const;
+
     HashSet<Widget*> m_widgetsInRenderTree;
 
@@ -719 +721 @@
     bool m_overflowStatusDirty;
     bool m_horizontalOverflow;
-    bool m_verticalOverflow;    
-    RenderElement* m_viewportRenderer;
+    bool m_verticalOverflow;
+    enum class ViewportRendererType { None, Document, Body };
+    ViewportRendererType m_viewportRendererType { ViewportRendererType::None };
 
     Pagination m_pagination;