Frame flattening: Hit-testing an iframe could end up destroying the associated inline...
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Jul 2015 04:30:52 +0000 (04:30 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 1 Jul 2015 04:30:52 +0000 (04:30 +0000)
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderView.cpp

index c0374ae..4a045fc 100644 (file)
@@ -1,3 +1,36 @@
+2015-06-30  Zalan Bujtas  <zalan@apple.com>
+
+        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  <thetalecrafter@gmail.com>
 
         Implement ECMAScript Internationalization API
diff --git a/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt b/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash-expected.txt
new file mode 100644 (file)
index 0000000..d177235
--- /dev/null
@@ -0,0 +1 @@
+Pass if no crash or assert in debug. 
diff --git a/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html b/LayoutTests/fast/frames/flattening/hittest-iframe-while-style-changes-crash.html
new file mode 100644 (file)
index 0000000..b0caeab
--- /dev/null
@@ -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="data:text/html,<div id=foo>foobar</div>">
+</body>
index 39734cc..712177a 100644 (file)
@@ -1,3 +1,44 @@
+2015-06-30  Zalan Bujtas  <zalan@apple.com>
+
+        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  <thetalecrafter@gmail.com>
 
         Implement ECMAScript Internationalization API
index 21b754e..875d6ef 100644 (file)
@@ -1140,6 +1140,9 @@ void FrameView::layout(bool allowSubtree)
     if (isInLayout())
         return;
 
+    if (layoutDisallowed())
+        return;
+
     // Protect the view from being deleted during layout (in recalcStyle).
     Ref<FrameView> protect(*this);
 
@@ -3763,9 +3766,6 @@ void FrameView::startLayoutAtMainFrameViewIfNeeded(bool allowSubtree)
         parentView = parentView->parentFrameView();
 
     parentView->layout(allowSubtree);
-
-    RenderElement* root = m_layoutRoot ? m_layoutRoot : frame().document()->renderView();
-    ASSERT_UNUSED(root, !root->needsLayout());
 }
 
 void FrameView::updateControlTints()
index c2c4cf2..2ae1674 100644 (file)
@@ -365,6 +365,10 @@ public:
 
     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 @@ private:
 
     unsigned m_deferSetNeedsLayoutCount;
     bool m_setNeedsLayoutWasDeferred;
+    int m_layoutDisallowed { 0 };
 
     RefPtr<Node> m_nodeToDraw;
     PaintBehavior m_paintBehavior;
index eed495e..9d03c02 100644 (file)
 
 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 @@ bool RenderView::hitTest(const HitTestRequest& request, const HitTestLocation& l
 {
     document().updateLayout();
 
+    FrameFlatteningLayoutDisallower disallower(frameView());
+
     if (layer()->hitTest(request, location, result))
         return true;