- Revision
- 186165
- Author
- [email protected]
- Date
- 2015-06-30 21:30:52 -0700 (Tue, 30 Jun 2015)
Log Message
Frame flattening: Hit-testing an iframe could end up destroying the associated inline tree context.
https://bugs.webkit.org/show_bug.cgi?id=146447
rdar://problem/20613501
Reviewed by Simon Fraser.
This patch ensures that the render tree associated with the document on which
the hit-test is initiated does not get laid out, unless it was directly mutated prior to the hittest.
Hit-test requirements:
1. A clean the render tree before hit-testing gets propagated to the renderers.
Document::updateLayout() ensures it by calling both updateStyleIfNeeded() and layout() not only on the current tree, but also
on the ancestors if needed.
2. No render tree mutation while hit-testing the renderers.
When an iframe is being hit-tested, this hit-test could bubble down to the child frame's render view.
In order to ensure #1, we call Document::updateLayout() on the current (subframe) document.
If updateStyleIfNeeded() mutates the render tree, we mark it dirty for layout(). However frame flattening also
marks the parent renderer (RenderIFrame) dirty.
While calling layout() to clean the current render tree, we end up laying out the parent tree too.
Laying out the parent tree could end up destroying the inline tree context from where the
hittest just bubbled down. (InlineFlowBox -> RenderWidget -> RenderView).
This patch protects the render tree from such unintentional inline tree mutation during hittesting.
After the initial layout we set a layout disallow flag on the frame view to defer subsequent layouts.
This patch only changes behavior when frame flattening is enabled, but in future we may always want to enable this.
Source/WebCore:
Test: fast/frames/flattening/hittest-iframe-while-style-changes-crash.html
* page/FrameView.cpp:
(WebCore::FrameView::layout):
(WebCore::FrameView::startLayoutAtMainFrameViewIfNeeded): Deleted. -> Assertion in no longer valid.
* page/FrameView.h:
* rendering/RenderView.cpp:
(WebCore::FrameFlatteningLayoutDisallower::FrameFlatteningLayoutDisallower):
(WebCore::FrameFlatteningLayoutDisallower::~FrameFlatteningLayoutDisallower):
(WebCore::RenderView::hitTest): Protect the render tree from subsequent layouts.
LayoutTests:
* fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt: Added.
* fast/frames/flattening/hittest-iframe-while-style-changes-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (186164 => 186165)
--- trunk/LayoutTests/ChangeLog 2015-07-01 02:39:50 UTC (rev 186164)
+++ trunk/LayoutTests/ChangeLog 2015-07-01 04:30:52 UTC (rev 186165)
@@ -1,3 +1,36 @@
+2015-06-30 Zalan Bujtas <[email protected]>
+
+ Frame flattening: Hit-testing an iframe could end up destroying the associated inline tree context.
+ https://bugs.webkit.org/show_bug.cgi?id=146447
+ rdar://problem/20613501
+
+ Reviewed by Simon Fraser.
+
+ This patch ensures that the render tree associated with the document on which
+ the hit-test is initiated does not get laid out, unless it was directly mutated prior to the hittest.
+
+ Hit-test requirements:
+ 1. A clean the render tree before hit-testing gets propagated to the renderers.
+ Document::updateLayout() ensures it by calling both updateStyleIfNeeded() and layout() not only on the current tree, but also
+ on the ancestors if needed.
+
+ 2. No render tree mutation while hit-testing the renderers.
+
+ When an iframe is being hit-tested, this hit-test could bubble down to the child frame's render view.
+ In order to ensure #1, we call Document::updateLayout() on the current (subframe) document.
+ If updateStyleIfNeeded() mutates the render tree, we mark it dirty for layout(). However frame flattening also
+ marks the parent renderer (RenderIFrame) dirty.
+ While calling layout() to clean the current render tree, we end up laying out the parent tree too.
+ Laying out the parent tree could end up destroying the inline tree context from where the
+ hittest just bubbled down. (InlineFlowBox -> RenderWidget -> RenderView).
+
+ This patch protects the render tree from such unintentional inline tree mutation during hittesting.
+ After the initial layout we set a layout disallow flag on the frame view to defer subsequent layouts.
+ This patch only changes behavior when frame flattening is enabled, but in future we may always want to enable this.
+
+ * fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt: Added.
+ * fast/frames/flattening/hittest-iframe-while-style-changes-crash.html: Added.
+
2015-06-30 Andy VanWagoner <[email protected]>
Implement ECMAScript Internationalization API
Added: trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt (0 => 186165)
--- trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt 2015-07-01 04:30:52 UTC (rev 186165)
@@ -0,0 +1 @@
+Pass if no crash or assert in debug.
Added: trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html (0 => 186165)
--- trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html (rev 0)
+++ trunk/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html 2015-07-01 04:30:52 UTC (rev 186165)
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+<head>
+<title>This tests that hittesting an iframe when frame flattening is on does not crash.</title>
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+if (window.internals)
+ internals.settings.setFrameFlatteningEnabled(true);
+
+function runTest() {
+ setTimeout(function() {
+ document.getElementById('clickonthis').contentDocument.getElementById('foo').style.display = "none";
+ if (window.internals)
+ internals.nodesFromRect(document, 100, 100, 0, 0, 0, 0, false, false, true);
+ if (window.testRunner)
+ testRunner.notifyDone();
+ }, 0);
+}
+</script>
+<body>
+Pass if no crash or assert in debug.
+<iframe _onload_="runTest()" id=clickonthis src="" id=foo>foobar</div>">
+</body>
Modified: trunk/Source/WebCore/ChangeLog (186164 => 186165)
--- trunk/Source/WebCore/ChangeLog 2015-07-01 02:39:50 UTC (rev 186164)
+++ trunk/Source/WebCore/ChangeLog 2015-07-01 04:30:52 UTC (rev 186165)
@@ -1,3 +1,44 @@
+2015-06-30 Zalan Bujtas <[email protected]>
+
+ Frame flattening: Hit-testing an iframe could end up destroying the associated inline tree context.
+ https://bugs.webkit.org/show_bug.cgi?id=146447
+ rdar://problem/20613501
+
+ Reviewed by Simon Fraser.
+
+ This patch ensures that the render tree associated with the document on which
+ the hit-test is initiated does not get laid out, unless it was directly mutated prior to the hittest.
+
+ Hit-test requirements:
+ 1. A clean the render tree before hit-testing gets propagated to the renderers.
+ Document::updateLayout() ensures it by calling both updateStyleIfNeeded() and layout() not only on the current tree, but also
+ on the ancestors if needed.
+
+ 2. No render tree mutation while hit-testing the renderers.
+
+ When an iframe is being hit-tested, this hit-test could bubble down to the child frame's render view.
+ In order to ensure #1, we call Document::updateLayout() on the current (subframe) document.
+ If updateStyleIfNeeded() mutates the render tree, we mark it dirty for layout(). However frame flattening also
+ marks the parent renderer (RenderIFrame) dirty.
+ While calling layout() to clean the current render tree, we end up laying out the parent tree too.
+ Laying out the parent tree could end up destroying the inline tree context from where the
+ hittest just bubbled down. (InlineFlowBox -> RenderWidget -> RenderView).
+
+ This patch protects the render tree from such unintentional inline tree mutation during hittesting.
+ After the initial layout we set a layout disallow flag on the frame view to defer subsequent layouts.
+ This patch only changes behavior when frame flattening is enabled, but in future we may always want to enable this.
+
+ Test: fast/frames/flattening/hittest-iframe-while-style-changes-crash.html
+
+ * page/FrameView.cpp:
+ (WebCore::FrameView::layout):
+ (WebCore::FrameView::startLayoutAtMainFrameViewIfNeeded): Deleted. -> Assertion in no longer valid.
+ * page/FrameView.h:
+ * rendering/RenderView.cpp:
+ (WebCore::FrameFlatteningLayoutDisallower::FrameFlatteningLayoutDisallower):
+ (WebCore::FrameFlatteningLayoutDisallower::~FrameFlatteningLayoutDisallower):
+ (WebCore::RenderView::hitTest): Protect the render tree from subsequent layouts.
+
2015-06-30 Andy VanWagoner <[email protected]>
Implement ECMAScript Internationalization API
Modified: trunk/Source/WebCore/page/FrameView.cpp (186164 => 186165)
--- trunk/Source/WebCore/page/FrameView.cpp 2015-07-01 02:39:50 UTC (rev 186164)
+++ trunk/Source/WebCore/page/FrameView.cpp 2015-07-01 04:30:52 UTC (rev 186165)
@@ -1140,6 +1140,9 @@
if (isInLayout())
return;
+ if (layoutDisallowed())
+ return;
+
// Protect the view from being deleted during layout (in recalcStyle).
Ref<FrameView> protect(*this);
@@ -3763,9 +3766,6 @@
parentView = parentView->parentFrameView();
parentView->layout(allowSubtree);
-
- RenderElement* root = m_layoutRoot ? m_layoutRoot : frame().document()->renderView();
- ASSERT_UNUSED(root, !root->needsLayout());
}
void FrameView::updateControlTints()
Modified: trunk/Source/WebCore/page/FrameView.h (186164 => 186165)
--- trunk/Source/WebCore/page/FrameView.h 2015-07-01 02:39:50 UTC (rev 186164)
+++ trunk/Source/WebCore/page/FrameView.h 2015-07-01 04:30:52 UTC (rev 186165)
@@ -365,6 +365,10 @@
bool isInChildFrameWithFrameFlattening() const;
+ void startDisallowingLayout() { ++m_layoutDisallowed; }
+ void endDisallowingLayout() { ASSERT(m_layoutDisallowed > 0); --m_layoutDisallowed; }
+ bool layoutDisallowed() const { return m_layoutDisallowed; }
+
static double currentPaintTimeStamp() { return sCurrentPaintTimeStamp; } // returns 0 if not painting
WEBCORE_EXPORT void updateLayoutAndStyleIfNeededRecursive();
@@ -736,6 +740,7 @@
unsigned m_deferSetNeedsLayoutCount;
bool m_setNeedsLayoutWasDeferred;
+ int m_layoutDisallowed { 0 };
RefPtr<Node> m_nodeToDraw;
PaintBehavior m_paintBehavior;
Modified: trunk/Source/WebCore/rendering/RenderView.cpp (186164 => 186165)
--- trunk/Source/WebCore/rendering/RenderView.cpp 2015-07-01 02:39:50 UTC (rev 186164)
+++ trunk/Source/WebCore/rendering/RenderView.cpp 2015-07-01 04:30:52 UTC (rev 186165)
@@ -54,6 +54,26 @@
namespace WebCore {
+struct FrameFlatteningLayoutDisallower {
+ FrameFlatteningLayoutDisallower(FrameView& frameView)
+ : m_frameView(frameView)
+ , m_disallowLayout(frameView.frame().settings().frameFlatteningEnabled())
+ {
+ if (m_disallowLayout)
+ m_frameView.startDisallowingLayout();
+ }
+
+ ~FrameFlatteningLayoutDisallower()
+ {
+ if (m_disallowLayout)
+ m_frameView.endDisallowingLayout();
+ }
+
+private:
+ FrameView& m_frameView;
+ bool m_disallowLayout { false };
+};
+
struct SelectionIterator {
RenderObject* m_current;
Vector<RenderMultiColumnSpannerPlaceholder*> m_spannerStack;
@@ -176,6 +196,8 @@
{
document().updateLayout();
+ FrameFlatteningLayoutDisallower disallower(frameView());
+
if (layer()->hitTest(request, location, result))
return true;
