Changeset 183788 in webkit

Timestamp:

May 4, 2015 8:22:35 PM (9 years ago)

Author:

Alan Bujtas

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/FrameView.cpp (modified) (1 diff)
• rendering/RenderFrameBase.cpp (modified) (2 diffs)
• rendering/RenderObject.cpp (modified) (2 diffs)
• rendering/RenderView.h (modified) (3 diffs)
• rendering/RenderWidget.cpp (modified) (1 diff)
• rendering/RenderWidget.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/FrameView.cpp

@@ -1362 +1362 @@
         root->view().repaintRootContents();
 
+    root->view().releaseProtectedRenderWidgets();
+
     ASSERT(!root->needsLayout());
 

trunk/Source/WebCore/rendering/RenderFrameBase.cpp

@@ -55 +55 @@
 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;
@@ -84 +84 @@
         // 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());

trunk/Source/WebCore/rendering/RenderObject.cpp

@@ -61 +61 @@
 #include "RenderTheme.h"
 #include "RenderView.h"
+#include "RenderWidget.h"
 #include "SVGRenderSupport.h"
 #include "Settings.h"
@@ -2025 +2026 @@
 
     willBeDestroyed();
+    if (is<RenderWidget>(*this)) {
+        downcast<RenderWidget>(*this).deref();
+        return;
+    }
     delete this;
 }

trunk/Source/WebCore/rendering/RenderView.h

@@ -27 +27 @@
 #include "Region.h"
 #include "RenderBlockFlow.h"
+#include "RenderWidget.h"
 #include "SelectionSubtreeRoot.h"
 #include <memory>
@@ -245 +246 @@
     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;
@@ -363 +367 @@
 
     HashSet<RenderElement*> m_renderersWithPausedImageAnimation;
+    Vector<RefPtr<RenderWidget>> m_protectedRenderWidgets;
 
 #if ENABLE(SERVICE_CONTROLS)

trunk/Source/WebCore/rendering/RenderWidget.cpp

@@ -107 +107 @@
 RenderWidget::~RenderWidget()
 {
+    ASSERT(!m_refCount);
 }
 

trunk/Source/WebCore/rendering/RenderWidget.h

@@ -75 +75 @@
     WeakPtr<RenderWidget> createWeakPtr() { return m_weakPtrFactory.createWeakPtr(); }
 
+    void ref() { ++m_refCount; }
+    void deref();
+
 protected:
     RenderWidget(HTMLFrameOwnerElement&, Ref<RenderStyle>&&);
@@ -103 +106 @@
     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