Do not crash when the descendant frame tree is destroyed during layout.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Jun 2015 02:49:12 +0000 (02:49 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Jun 2015 02:49:12 +0000 (02:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144540
rdar://problem/20793184

Reviewed by Andreas Kling.

Source/WebCore:

Widget::setFrameRect(), through WebHTMLView layout, could trigger a style recalc, which in turn
could initiate an onBeforeLoad callback.
If javascript happens to destroy the current iframe in the onBeforeLoad callback, we lose the descendant
render tree, including the child FrameView (the iframe element's view). However the RenderIFrame
object stays protected until after the layout is done. (see protectRenderWidgetUntilLayoutIsDone())

Climbing back on the callstack, we need to make sure that
1. the root widget of the descendant render tree (FrameView) stays valid as long as it is needed.
2. RenderFrameBase::layoutWithFlattening() can handle the case when the associated widget (child FrameView) is set to nullptr.
(see RenderWidget::willBeDestroyed() -> setWidget(nullptr))

(and later, when layout is finished this (RenderIFrame) object gets destroyed too.)

Covered by fast/frames/flattening/crash-remove-iframe-during-object-beforeload.html.

* page/FrameView.cpp:
(WebCore::FrameView::setFrameRect):
(WebCore::FrameView::updateEmbeddedObject):
(WebCore::FrameView::updateWidgetPositions):
* platform/ScrollView.cpp:
(WebCore::ScrollView::setFrameRect):
* platform/mac/WidgetMac.mm:
(WebCore::Widget::setFrameRect):
* rendering/RenderFrameBase.cpp:
(WebCore::RenderFrameBase::layoutWithFlattening):
(WebCore::RenderFrameBase::childRenderView):
(WebCore::RenderFrameBase::peformLayoutWithFlattening):
* rendering/RenderFrameBase.h:
* rendering/RenderWidget.cpp:
(WebCore::RenderWidget::updateWidgetPosition):
* rendering/RenderWidget.h:

LayoutTests:

Unskip fast/frames/flattening/crash-remove-iframe-during-object-beforeload.html.

* TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/platform/ScrollView.cpp
Source/WebCore/platform/mac/WidgetMac.mm
Source/WebCore/rendering/RenderFrameBase.cpp
Source/WebCore/rendering/RenderFrameBase.h
Source/WebCore/rendering/RenderWidget.cpp
Source/WebCore/rendering/RenderWidget.h

index 7f9bb25..1e7224f 100644 (file)
@@ -1,3 +1,15 @@
+2015-06-11  Zalan Bujtas  <zalan@apple.com>
+
+        Do not crash when the descendant frame tree is destroyed during layout.
+        https://bugs.webkit.org/show_bug.cgi?id=144540
+        rdar://problem/20793184
+
+        Reviewed by Andreas Kling.
+
+        Unskip fast/frames/flattening/crash-remove-iframe-during-object-beforeload.html.
+
+        * TestExpectations:
+
 2015-06-11  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r185470.
index fa2d262..058b323 100644 (file)
@@ -519,8 +519,6 @@ webkit.org/b/142937 ietestcenter/Javascript/15.2.3.14-1-3.html [ Failure ]
 
 webkit.org/b/144258 [ Debug ] js/class-syntax-semicolon.html [ Skip ]
 
-webkit.org/b/144540 fast/frames/flattening/crash-remove-iframe-during-object-beforeload.html [ Skip ]
-
 webkit.org/b/145007 js/regress-141098.html [ Skip ]
 
 webkit.org/b/145390 storage/indexeddb/deleteIndex-bug110792.html [ Pass Failure ]
index 491ef0c..0ce2ff5 100644 (file)
@@ -1,3 +1,43 @@
+2015-06-11  Zalan Bujtas  <zalan@apple.com>
+
+        Do not crash when the descendant frame tree is destroyed during layout.
+        https://bugs.webkit.org/show_bug.cgi?id=144540
+        rdar://problem/20793184
+
+        Reviewed by Andreas Kling.
+
+        Widget::setFrameRect(), through WebHTMLView layout, could trigger a style recalc, which in turn
+        could initiate an onBeforeLoad callback.
+        If javascript happens to destroy the current iframe in the onBeforeLoad callback, we lose the descendant
+        render tree, including the child FrameView (the iframe element's view). However the RenderIFrame
+        object stays protected until after the layout is done. (see protectRenderWidgetUntilLayoutIsDone())
+
+        Climbing back on the callstack, we need to make sure that
+        1. the root widget of the descendant render tree (FrameView) stays valid as long as it is needed.
+        2. RenderFrameBase::layoutWithFlattening() can handle the case when the associated widget (child FrameView) is set to nullptr.
+        (see RenderWidget::willBeDestroyed() -> setWidget(nullptr))
+
+        (and later, when layout is finished this (RenderIFrame) object gets destroyed too.)
+
+        Covered by fast/frames/flattening/crash-remove-iframe-during-object-beforeload.html.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::setFrameRect):
+        (WebCore::FrameView::updateEmbeddedObject):
+        (WebCore::FrameView::updateWidgetPositions):
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::setFrameRect):
+        * platform/mac/WidgetMac.mm:
+        (WebCore::Widget::setFrameRect):
+        * rendering/RenderFrameBase.cpp:
+        (WebCore::RenderFrameBase::layoutWithFlattening):
+        (WebCore::RenderFrameBase::childRenderView):
+        (WebCore::RenderFrameBase::peformLayoutWithFlattening):
+        * rendering/RenderFrameBase.h:
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::updateWidgetPosition):
+        * rendering/RenderWidget.h:
+
 2015-06-11  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r185470.
index 32afa44..2a89880 100644 (file)
@@ -452,6 +452,7 @@ void FrameView::invalidateRect(const IntRect& rect)
 
 void FrameView::setFrameRect(const IntRect& newRect)
 {
+    Ref<FrameView> protect(*this);
     IntRect oldRect = frameRect();
     if (newRect == oldRect)
         return;
@@ -2935,7 +2936,8 @@ void FrameView::updateEmbeddedObject(RenderEmbeddedObject& embeddedObject)
     if (!weakRenderer)
         return;
 
-    embeddedObject.updateWidgetPosition();
+    auto ignoreWidgetState = embeddedObject.updateWidgetPosition();
+    UNUSED_PARAM(ignoreWidgetState);
 }
 
 bool FrameView::updateEmbeddedObjects()
@@ -4714,8 +4716,10 @@ void FrameView::updateWidgetPositions()
     // scripts in response to NPP_SetWindow, for example), so we need to keep the Widgets
     // alive during enumeration.
     for (auto& widget : collectAndProtectWidgets(m_widgetsInRenderTree)) {
-        if (RenderWidget* renderWidget = RenderWidget::find(widget.get()))
-            renderWidget->updateWidgetPosition();
+        if (RenderWidget* renderWidget = RenderWidget::find(widget.get())) {
+            auto ignoreWidgetState = renderWidget->updateWidgetPosition();
+            UNUSED_PARAM(ignoreWidgetState);
+        }
     }
 }
 
index 67ea7d2..3cc6701 100644 (file)
@@ -1056,6 +1056,7 @@ void ScrollView::setScrollbarOverlayStyle(ScrollbarOverlayStyle overlayStyle)
 
 void ScrollView::setFrameRect(const IntRect& newRect)
 {
+    Ref<ScrollView> protect(*this);
     IntRect oldRect = frameRect();
     
     if (newRect == oldRect)
index 1a98271..54d9c85 100644 (file)
@@ -159,7 +159,7 @@ void Widget::setFrameRect(const IntRect& rect)
         return;
 
     // Take a reference to this Widget, because sending messages to outerView can invoke arbitrary
-    // code, which can deref it.
+    // code including recalc style/layout, which can deref it.
     Ref<Widget> protect(*this);
 
     NSRect frame = rect;
index 194e78f..cdbccdf 100644 (file)
@@ -55,23 +55,35 @@ inline bool shouldExpandFrame(LayoutUnit width, LayoutUnit height, bool hasFixed
 void RenderFrameBase::layoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight)
 {
     view().protectRenderWidgetUntilLayoutIsDone(*this);
-    RenderView* childRoot = childView() ? childView()->frame().contentRenderer() : 0;
 
-    if (!childRoot || !shouldExpandFrame(width(), height(), hasFixedWidth, hasFixedHeight)) {
-        updateWidgetPosition();
-        if (childView())
-            childView()->layout();
-        clearNeedsLayout();
+    peformLayoutWithFlattening(hasFixedWidth, hasFixedHeight);
+
+    clearNeedsLayout();
+}
+
+RenderView* RenderFrameBase::childRenderView() const
+{
+    if (!childView())
+        return nullptr;
+    return childView()->renderView();
+}
+
+void RenderFrameBase::peformLayoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight)
+{
+    if (!childRenderView() || !shouldExpandFrame(width(), height(), hasFixedWidth, hasFixedHeight)) {
+        if (updateWidgetPosition() == ChildWidgetState::ChildWidgetIsDestroyed)
+            return;
+        childView()->layout();
         return;
     }
 
     // need to update to calculate min/max correctly
-    updateWidgetPosition();
-
+    if (updateWidgetPosition() == ChildWidgetState::ChildWidgetIsDestroyed)
+        return;
+    
     // if scrollbars are off, and the width or height are fixed
     // we obey them and do not expand. With frame flattening
     // no subframe much ever become scrollable.
-
     bool isScrollable = frameOwnerElement().scrollingMode() != ScrollbarAlwaysOff;
 
     // consider iframe inset border
@@ -80,27 +92,27 @@ void RenderFrameBase::layoutWithFlattening(bool hasFixedWidth, bool hasFixedHeig
 
     // make sure minimum preferred width is enforced
     if (isScrollable || !hasFixedWidth) {
-        setWidth(std::max(width(), childRoot->minPreferredLogicalWidth() + hBorder));
+        ASSERT(childRenderView());
+        setWidth(std::max(width(), childRenderView()->minPreferredLogicalWidth() + hBorder));
         // update again to pass the new width to the child frame
-        updateWidgetPosition();
-        if (childView())
-            childView()->layout();
+        if (updateWidgetPosition() == ChildWidgetState::ChildWidgetIsDestroyed)
+            return;
+        childView()->layout();
     }
 
-    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(childView());
+    // expand the frame by setting frame height = content height
+    if (isScrollable || !hasFixedHeight || childRenderView()->isFrameSet())
+        setHeight(std::max<LayoutUnit>(height(), childView()->contentsHeight() + vBorder));
+    if (isScrollable || !hasFixedWidth || childRenderView()->isFrameSet())
+        setWidth(std::max<LayoutUnit>(width(), childView()->contentsWidth() + hBorder));
 
-    ASSERT(!childView()->layoutPending());
-    ASSERT(!childRoot->needsLayout());
-    ASSERT(!childRoot->firstChild() || !childRoot->firstChild()->firstChildSlow() || !childRoot->firstChild()->firstChildSlow()->needsLayout());
+    if (updateWidgetPosition() == ChildWidgetState::ChildWidgetIsDestroyed)
+        return;
 
-    clearNeedsLayout();
+    ASSERT(!childView()->layoutPending());
+    ASSERT(!childRenderView()->needsLayout());
+    ASSERT(!childRenderView()->firstChild() || !childRenderView()->firstChild()->firstChildSlow() || !childRenderView()->firstChild()->firstChildSlow()->needsLayout());
 }
 
 }
index e7dd6bf..7f2f636 100644 (file)
@@ -32,6 +32,7 @@
 namespace WebCore {
 
 class HTMLFrameElementBase;
+class RenderView;
 
 // Base class for RenderFrame and RenderIFrame
 class RenderFrameBase : public RenderWidget {
@@ -44,6 +45,8 @@ public:
     void layoutWithFlattening(bool fixedWidth, bool fixedHeight);
 
 private:
+    void peformLayoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight);
+    RenderView* childRenderView() const;
     void widget() const = delete;
 };
 
index 7b1972a..d75b0fd 100644 (file)
@@ -303,15 +303,15 @@ void RenderWidget::setOverlapTestResult(bool isOverlapped)
     downcast<FrameView>(*m_widget).setIsOverlapped(isOverlapped);
 }
 
-void RenderWidget::updateWidgetPosition()
+RenderWidget::ChildWidgetState RenderWidget::updateWidgetPosition()
 {
     if (!m_widget)
-        return;
+        return ChildWidgetState::ChildWidgetIsDestroyed;
 
     WeakPtr<RenderWidget> weakThis = createWeakPtr();
     bool widgetSizeChanged = updateWidgetGeometry();
-    if (!weakThis)
-        return;
+    if (!weakThis || !m_widget)
+        return ChildWidgetState::ChildWidgetIsDestroyed;
 
     // if the frame size got changed, or if view needs layout (possibly indicating
     // content size is wrong) we have to do a layout to set the right widget size.
@@ -321,6 +321,7 @@ void RenderWidget::updateWidgetPosition()
         if ((widgetSizeChanged || frameView.needsLayout()) && frameView.frame().page())
             frameView.layout();
     }
+    return ChildWidgetState::ChildWidgetIsValid;
 }
 
 IntRect RenderWidget::windowClipRect() const
index 6f5776d..8a1a013 100644 (file)
@@ -67,7 +67,8 @@ public:
 
     static RenderWidget* find(const Widget*);
 
-    void updateWidgetPosition();
+    enum class ChildWidgetState { ChildWidgetIsValid, ChildWidgetIsDestroyed };
+    ChildWidgetState updateWidgetPosition() WARN_UNUSED_RETURN;
     WEBCORE_EXPORT IntRect windowClipRect() const;
 
     bool requiresAcceleratedCompositing() const;