RenderWidget::setWidgetGeometry() can end up destroying *this*.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 May 2015 03:22:35 +0000 (03:22 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 May 2015 03:22:35 +0000 (03:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144601

Reviewed by Andreas Kling.

This is a speculative fix to ensure we don't crash on an invalid *this* renderer
while flattening the current iframe.
Calling RenderWidget::setWidgetGeometry() can result in destroying the current renderer.
While it is not a issue in case of normal layout flow as widget positions are updated at post layout,
frame flattening initiates this action in the middle of layout.
This patch re-introduces refcount model for RenderWidgets so that the renderer is protected during layout
when frame flattening is in use.

* rendering/RenderFrameBase.cpp:
(WebCore::RenderFrameBase::layoutWithFlattening): Let's be paranoid about child view.
* rendering/RenderObject.cpp:
(WebCore::RenderObject::destroy):
* rendering/FrameView.cpp:
(WebCore::FrameView::layout):
* rendering/RenderView.h:
* rendering/RenderWidget.cpp:
(WebCore::RenderWidget::~RenderWidget):
* rendering/RenderWidget.h:
(WebCore::RenderWidget::ref):
(WebCore::RenderWidget::deref):

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

Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/rendering/RenderFrameBase.cpp
Source/WebCore/rendering/RenderObject.cpp
Source/WebCore/rendering/RenderView.h
Source/WebCore/rendering/RenderWidget.cpp
Source/WebCore/rendering/RenderWidget.h

index 54d8331..c49df99 100644 (file)
@@ -1,3 +1,31 @@
+2015-05-04  Zalan Bujtas  <zalan@apple.com>
+
+        RenderWidget::setWidgetGeometry() can end up destroying *this*.
+        https://bugs.webkit.org/show_bug.cgi?id=144601
+
+        Reviewed by Andreas Kling.
+
+        This is a speculative fix to ensure we don't crash on an invalid *this* renderer
+        while flattening the current iframe.
+        Calling RenderWidget::setWidgetGeometry() can result in destroying the current renderer.
+        While it is not a issue in case of normal layout flow as widget positions are updated at post layout,
+        frame flattening initiates this action in the middle of layout.
+        This patch re-introduces refcount model for RenderWidgets so that the renderer is protected during layout
+        when frame flattening is in use.
+
+        * rendering/RenderFrameBase.cpp:
+        (WebCore::RenderFrameBase::layoutWithFlattening): Let's be paranoid about child view.
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::destroy):
+        * rendering/FrameView.cpp:
+        (WebCore::FrameView::layout):
+        * rendering/RenderView.h:
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::~RenderWidget):
+        * rendering/RenderWidget.h:
+        (WebCore::RenderWidget::ref):
+        (WebCore::RenderWidget::deref):
+
 2015-05-04  Doug Russell  <d_russell@apple.com>
 
         AX: setting focus via accessibility object needs to set isSynchronizing in resulting selection intent
index 0da0757..1258636 100644 (file)
@@ -1361,6 +1361,8 @@ void FrameView::layout(bool allowSubtree)
     if (m_needsFullRepaint)
         root->view().repaintRootContents();
 
+    root->view().releaseProtectedRenderWidgets();
+
     ASSERT(!root->needsLayout());
 
     layer->updateLayerPositionsAfterLayout(renderView()->layer(), updateLayerPositionFlags(layer, subtree, m_needsFullRepaint));
index 925a073..9232ccd 100644 (file)
@@ -54,13 +54,13 @@ inline bool shouldExpandFrame(LayoutUnit width, LayoutUnit height, bool hasFixed
 
 void RenderFrameBase::layoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight)
 {
-    FrameView* childFrameView = childView();
-    RenderView* childRoot = childFrameView ? childFrameView->frame().contentRenderer() : 0;
+    view().protectRenderWidgetUntilLayoutIsDone(*this);
+    RenderView* childRoot = childView() ? childView()->frame().contentRenderer() : 0;
 
     if (!childRoot || !shouldExpandFrame(width(), height(), hasFixedWidth, hasFixedHeight)) {
         updateWidgetPosition();
-        if (childFrameView)
-            childFrameView->layout();
+        if (childView())
+            childView()->layout();
         clearNeedsLayout();
         return;
     }
@@ -83,18 +83,20 @@ void RenderFrameBase::layoutWithFlattening(bool hasFixedWidth, bool hasFixedHeig
         setWidth(std::max(width(), childRoot->minPreferredLogicalWidth() + hBorder));
         // update again to pass the new width to the child frame
         updateWidgetPosition();
-        childFrameView->layout();
+        if (childView())
+            childView()->layout();
     }
 
-    // expand the frame by setting frame height = content height
-    if (isScrollable || !hasFixedHeight || childRoot->isFrameSet())
-        setHeight(std::max<LayoutUnit>(height(), childFrameView->contentsHeight() + vBorder));
-    if (isScrollable || !hasFixedWidth || childRoot->isFrameSet())
-        setWidth(std::max<LayoutUnit>(width(), childFrameView->contentsWidth() + hBorder));
-
+    if (childView()) {
+        // expand the frame by setting frame height = content height
+        if (isScrollable || !hasFixedHeight || childRoot->isFrameSet())
+            setHeight(std::max<LayoutUnit>(height(), childView()->contentsHeight() + vBorder));
+        if (isScrollable || !hasFixedWidth || childRoot->isFrameSet())
+            setWidth(std::max<LayoutUnit>(width(), childView()->contentsWidth() + hBorder));
+    }
     updateWidgetPosition();
 
-    ASSERT(!childFrameView->layoutPending());
+    ASSERT(!childView()->layoutPending());
     ASSERT(!childRoot->needsLayout());
     ASSERT(!childRoot->firstChild() || !childRoot->firstChild()->firstChildSlow() || !childRoot->firstChild()->firstChildSlow()->needsLayout());
 
index fbb5d78..f576ec5 100644 (file)
@@ -60,6 +60,7 @@
 #include "RenderScrollbarPart.h"
 #include "RenderTheme.h"
 #include "RenderView.h"
+#include "RenderWidget.h"
 #include "SVGRenderSupport.h"
 #include "Settings.h"
 #include "StyleResolver.h"
@@ -2024,6 +2025,10 @@ void RenderObject::destroy()
 #endif
 
     willBeDestroyed();
+    if (is<RenderWidget>(*this)) {
+        downcast<RenderWidget>(*this).deref();
+        return;
+    }
     delete this;
 }
 
index c1aa9fa..2eb34fd 100644 (file)
@@ -26,6 +26,7 @@
 #include "LayoutState.h"
 #include "Region.h"
 #include "RenderBlockFlow.h"
+#include "RenderWidget.h"
 #include "SelectionSubtreeRoot.h"
 #include <memory>
 #include <wtf/HashSet.h>
@@ -244,6 +245,9 @@ public:
     void scheduleLazyRepaint(RenderBox&);
     void unscheduleLazyRepaint(RenderBox&);
 
+    void protectRenderWidgetUntilLayoutIsDone(RenderWidget& widget) { m_protectedRenderWidgets.append(&widget); }
+    void releaseProtectedRenderWidgets() { m_protectedRenderWidgets.clear(); }
+
 protected:
     virtual void mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags, bool* wasFixed) const override;
     virtual const RenderObject* pushMappingToContainer(const RenderLayerModelObject* ancestorToStopAt, RenderGeometryMap&) const override;
@@ -362,6 +366,7 @@ private:
     bool m_hasFlippedBlockDescendants;
 
     HashSet<RenderElement*> m_renderersWithPausedImageAnimation;
+    Vector<RefPtr<RenderWidget>> m_protectedRenderWidgets;
 
 #if ENABLE(SERVICE_CONTROLS)
     SelectionRectGatherer m_selectionRectGatherer;
index c476512..5edc1a3 100644 (file)
@@ -106,6 +106,7 @@ void RenderWidget::willBeDestroyed()
 
 RenderWidget::~RenderWidget()
 {
+    ASSERT(!m_refCount);
 }
 
 // Widgets are always placed on integer boundaries, so rounding the size is actually
index e848fc5..6f5776d 100644 (file)
@@ -74,6 +74,9 @@ public:
 
     WeakPtr<RenderWidget> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }
 
+    void ref() { ++m_refCount; }
+    void deref();
+
 protected:
     RenderWidget(HTMLFrameOwnerElement&, Ref<RenderStyle>&&);
 
@@ -102,8 +105,16 @@ private:
     WeakPtrFactory<RenderWidget> m_weakPtrFactory;
     RefPtr<Widget> m_widget;
     IntRect m_clipRect; // The rectangle needs to remain correct after scrolling, so it is stored in content view coordinates, and not clipped to window.
+    unsigned m_refCount { 1 };
 };
 
+inline void RenderWidget::deref()
+{
+    ASSERT(m_refCount);
+    if (!--m_refCount)
+        delete this;
+}
+
 } // namespace WebCore
 
 SPECIALIZE_TYPE_TRAITS_RENDER_OBJECT(RenderWidget, isWidget())