- Revision
- 185484
- Author
- [email protected]
- Date
- 2015-06-11 19:49:12 -0700 (Thu, 11 Jun 2015)
Log Message
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.
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:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (185483 => 185484)
--- trunk/LayoutTests/ChangeLog 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/LayoutTests/ChangeLog 2015-06-12 02:49:12 UTC (rev 185484)
@@ -1,3 +1,15 @@
+2015-06-11 Zalan Bujtas <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r185470.
Modified: trunk/LayoutTests/TestExpectations (185483 => 185484)
--- trunk/LayoutTests/TestExpectations 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/LayoutTests/TestExpectations 2015-06-12 02:49:12 UTC (rev 185484)
@@ -519,8 +519,6 @@
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 ]
Modified: trunk/Source/WebCore/ChangeLog (185483 => 185484)
--- trunk/Source/WebCore/ChangeLog 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/ChangeLog 2015-06-12 02:49:12 UTC (rev 185484)
@@ -1,3 +1,43 @@
+2015-06-11 Zalan Bujtas <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r185470.
Modified: trunk/Source/WebCore/page/FrameView.cpp (185483 => 185484)
--- trunk/Source/WebCore/page/FrameView.cpp 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/page/FrameView.cpp 2015-06-12 02:49:12 UTC (rev 185484)
@@ -452,6 +452,7 @@
void FrameView::setFrameRect(const IntRect& newRect)
{
+ Ref<FrameView> protect(*this);
IntRect oldRect = frameRect();
if (newRect == oldRect)
return;
@@ -2935,7 +2936,8 @@
if (!weakRenderer)
return;
- embeddedObject.updateWidgetPosition();
+ auto ignoreWidgetState = embeddedObject.updateWidgetPosition();
+ UNUSED_PARAM(ignoreWidgetState);
}
bool FrameView::updateEmbeddedObjects()
@@ -4714,8 +4716,10 @@
// 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);
+ }
}
}
Modified: trunk/Source/WebCore/platform/ScrollView.cpp (185483 => 185484)
--- trunk/Source/WebCore/platform/ScrollView.cpp 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/platform/ScrollView.cpp 2015-06-12 02:49:12 UTC (rev 185484)
@@ -1056,6 +1056,7 @@
void ScrollView::setFrameRect(const IntRect& newRect)
{
+ Ref<ScrollView> protect(*this);
IntRect oldRect = frameRect();
if (newRect == oldRect)
Modified: trunk/Source/WebCore/platform/mac/WidgetMac.mm (185483 => 185484)
--- trunk/Source/WebCore/platform/mac/WidgetMac.mm 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/platform/mac/WidgetMac.mm 2015-06-12 02:49:12 UTC (rev 185484)
@@ -159,7 +159,7 @@
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;
Modified: trunk/Source/WebCore/rendering/RenderFrameBase.cpp (185483 => 185484)
--- trunk/Source/WebCore/rendering/RenderFrameBase.cpp 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/rendering/RenderFrameBase.cpp 2015-06-12 02:49:12 UTC (rev 185484)
@@ -55,23 +55,35 @@
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 @@
// 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));
+ if (updateWidgetPosition() == ChildWidgetState::ChildWidgetIsDestroyed)
+ return;
+
ASSERT(!childView()->layoutPending());
- ASSERT(!childRoot->needsLayout());
- ASSERT(!childRoot->firstChild() || !childRoot->firstChild()->firstChildSlow() || !childRoot->firstChild()->firstChildSlow()->needsLayout());
-
- clearNeedsLayout();
+ ASSERT(!childRenderView()->needsLayout());
+ ASSERT(!childRenderView()->firstChild() || !childRenderView()->firstChild()->firstChildSlow() || !childRenderView()->firstChild()->firstChildSlow()->needsLayout());
}
}
Modified: trunk/Source/WebCore/rendering/RenderFrameBase.h (185483 => 185484)
--- trunk/Source/WebCore/rendering/RenderFrameBase.h 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/rendering/RenderFrameBase.h 2015-06-12 02:49:12 UTC (rev 185484)
@@ -32,6 +32,7 @@
namespace WebCore {
class HTMLFrameElementBase;
+class RenderView;
// Base class for RenderFrame and RenderIFrame
class RenderFrameBase : public RenderWidget {
@@ -44,6 +45,8 @@
void layoutWithFlattening(bool fixedWidth, bool fixedHeight);
private:
+ void peformLayoutWithFlattening(bool hasFixedWidth, bool hasFixedHeight);
+ RenderView* childRenderView() const;
void widget() const = delete;
};
Modified: trunk/Source/WebCore/rendering/RenderWidget.cpp (185483 => 185484)
--- trunk/Source/WebCore/rendering/RenderWidget.cpp 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/rendering/RenderWidget.cpp 2015-06-12 02:49:12 UTC (rev 185484)
@@ -303,15 +303,15 @@
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 @@
if ((widgetSizeChanged || frameView.needsLayout()) && frameView.frame().page())
frameView.layout();
}
+ return ChildWidgetState::ChildWidgetIsValid;
}
IntRect RenderWidget::windowClipRect() const
Modified: trunk/Source/WebCore/rendering/RenderWidget.h (185483 => 185484)
--- trunk/Source/WebCore/rendering/RenderWidget.h 2015-06-12 02:16:28 UTC (rev 185483)
+++ trunk/Source/WebCore/rendering/RenderWidget.h 2015-06-12 02:49:12 UTC (rev 185484)
@@ -67,7 +67,8 @@
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;
