Changeset 208003 in webkit

Timestamp:

Oct 27, 2016 1:56:17 PM (7 years ago)

Author:

Brent Fulgham

Message:

Prevent hit tests from being performed on an invalid render tree
​https://bugs.webkit.org/show_bug.cgi?id=163877
<rdar://problem/28675761>

Reviewed by Simon Fraser.

Source/WebCore:

Changeset r200971 added code to ensure that layout is up-to-date before hit testing, but did
so only for the main frame. It was still possible to enter cross-frame hit testing with a
subframe needing style recalc. In that situation, the subframe's updateLayout() would get
called, which could trigger a compositing change that marked the parent frame as needing style
recalc. A subsequent layout on the parent frame (for example by hit testing traversing into
a second subframe) could then mutate the parent frame's layer tree while hit testing was
traversing it.

This patch modifies the hit test logic to ensure that a recursive layout is performed so that
we always perform hit tests on a clean set of frames. It also adds some assertions to warn
us if we encounter this invalid state.

Tested by fast/layers/prevent-hit-test-during-layout.html.

• dom/Document.cpp:

(WebCore::Document::scheduleStyleRecalc): Assert that we are not hit testing
during style recalculation.

• page/EventHandler.cpp:

(WebCore::EventHandler::hitTestResultAtPoint): Ensure that we have a clean render tree
when hit testing.

• page/FrameView.cpp:

(WebCore::FrameView::setNeedsLayout): Assert that we are not in the process of hit testing
when we schedule a layout.

• rendering/RenderView.cpp:

(WebCore::RenderView::hitTest): Mark RenderView as in an active hit test.

• rendering/RenderView.h:

LayoutTests:

• fast/layers/prevent-hit-test-during-layout-expected.txt: Added.
• fast/layers/prevent-hit-test-during-layout.html: Added.
• platform/efl/TestExpectations: Skip on this platform.
• platform/gtk/TestExpectations: Skip on this platform.
• platform/ios-simulator/TestExpectations: Skip on this platform.
• platform/win/TestExpectations: Skip on this platform.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/layers/prevent-hit-test-during-layout-expected.txt (added)
• LayoutTests/fast/layers/prevent-hit-test-during-layout.html (added)
• LayoutTests/platform/efl/TestExpectations (modified) (1 diff)
• LayoutTests/platform/gtk/TestExpectations (modified) (1 diff)
• LayoutTests/platform/ios-simulator/TestExpectations (modified) (1 diff)
• LayoutTests/platform/win/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Document.cpp (modified) (1 diff)
• Source/WebCore/page/EventHandler.cpp (modified) (1 diff)
• Source/WebCore/page/FrameView.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderView.cpp (modified) (2 diffs)
• Source/WebCore/rendering/RenderView.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2016-10-25  Brent Fulgham  <bfulgham@apple.com>
+
+        Prevent hit tests from being performed on an invalid render tree
+        https://bugs.webkit.org/show_bug.cgi?id=163877
+        <rdar://problem/28675761>
+
+        Reviewed by Simon Fraser.
+
+        * fast/layers/prevent-hit-test-during-layout-expected.txt: Added.
+        * fast/layers/prevent-hit-test-during-layout.html: Added.
+        * platform/efl/TestExpectations: Skip on this platform.
+        * platform/gtk/TestExpectations: Skip on this platform.
+        * platform/ios-simulator/TestExpectations: Skip on this platform.
+        * platform/win/TestExpectations: Skip on this platform.
+
 2016-10-27  Chris Dumez  <cdumez@apple.com>
 

trunk/LayoutTests/platform/efl/TestExpectations

@@ -596 +596 @@
 security/contentSecurityPolicy/plugins-types-blocks-youtube-plugin-replacement-without-mime-type.html [ Skip ]
 security/contentSecurityPolicy/plugins-types-blocks-youtube-plugin-replacement.html [ Skip ]
+
+# Only Mac has implemented DictionaryLookup
+fast/layers/prevent-hit-test-during-layout.html [ Skip ]
 
 #////////////////////////////////////////////////////////////////////////////////////////

trunk/LayoutTests/platform/gtk/TestExpectations

@@ -1704 +1704 @@
 webkit.org/b/152247 fast/forms/input-appearance-spinbutton.html [ Skip ]
 webkit.org/b/152247 fast/forms/listbox-scrollbar-hit-test.html [ Skip ]
+
+# Only Mac has implemented DictionaryLookup
+fast/layers/prevent-hit-test-during-layout.html [ Skip ]
 
 #////////////////////////////////////////////////////////////////////////////////////////

trunk/LayoutTests/platform/ios-simulator/TestExpectations

@@ -2732 +2732 @@
 
 webkit.org/b/158836 imported/w3c/web-platform-tests/encrypted-media [ Skip ]
+
+# Only Mac has implemented DictionaryLookup
+fast/layers/prevent-hit-test-during-layout.html [ Skip ]

trunk/LayoutTests/platform/win/TestExpectations

@@ -3623 +3623 @@
 # html/syntax web platform tests are failing on Windows.
 webkit.org/b/162415 imported/w3c/web-platform-tests/html/syntax [ Skip ]
+
+# Only Mac has implemented DictionaryLookup
+fast/layers/prevent-hit-test-during-layout.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-10-27  Brent Fulgham  <bfulgham@apple.com>
+
+        Prevent hit tests from being performed on an invalid render tree
+        https://bugs.webkit.org/show_bug.cgi?id=163877
+        <rdar://problem/28675761>
+
+        Reviewed by Simon Fraser.
+
+        Changeset r200971 added code to ensure that layout is up-to-date before hit testing, but did
+        so only for the main frame. It was still possible to enter cross-frame hit testing with a
+        subframe needing style recalc. In that situation, the subframe's updateLayout() would get
+        called, which could trigger a compositing change that marked the parent frame as needing style
+        recalc. A subsequent layout on the parent frame (for example by hit testing traversing into
+        a second subframe) could then mutate the parent frame's layer tree while hit testing was
+        traversing it.
+        
+        This patch modifies the hit test logic to ensure that a recursive layout is performed so that
+        we always perform hit tests on a clean set of frames. It also adds some assertions to warn
+        us if we encounter this invalid state.
+
+        Tested by fast/layers/prevent-hit-test-during-layout.html.
+
+        * dom/Document.cpp:
+        (WebCore::Document::scheduleStyleRecalc): Assert that we are not hit testing
+        during style recalculation.
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::hitTestResultAtPoint): Ensure that we have a clean render tree
+        when hit testing.
+        * page/FrameView.cpp:
+        (WebCore::FrameView::setNeedsLayout): Assert that we are not in the process of hit testing
+        when we schedule a layout.
+        * rendering/RenderView.cpp:
+        (WebCore::RenderView::hitTest): Mark RenderView as in an active hit test.
+        * rendering/RenderView.h:
+
 2016-10-27  Zan Dobersek  <zdobersek@igalia.com>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -1756 +1756 @@
 void Document::scheduleStyleRecalc()
 {
+    ASSERT(!m_renderView || !m_renderView->inHitTesting());
+
     if (m_styleRecalcTimer.isActive() || pageCacheState() != NotInPageCache)
         return;

trunk/Source/WebCore/page/EventHandler.cpp

@@ -1146 +1146 @@
     unsigned nonNegativePaddingWidth = std::max<LayoutUnit>(0, padding.width()).toUnsigned();
     unsigned nonNegativePaddingHeight = std::max<LayoutUnit>(0, padding.height()).toUnsigned();
+
     // We should always start hit testing a clean tree.
-    if (m_frame.document())
-        m_frame.document()->updateLayoutIgnorePendingStylesheets();
+    if (auto* frameView = m_frame.view())
+        frameView->updateLayoutAndStyleIfNeededRecursive();
+
     HitTestResult result(point, nonNegativePaddingHeight, nonNegativePaddingWidth, nonNegativePaddingHeight, nonNegativePaddingWidth);
     RenderView* renderView = m_frame.contentRenderer();

trunk/Source/WebCore/page/FrameView.cpp

@@ -2846 +2846 @@
     }
 
-    if (RenderView* renderView = this->renderView())
+    if (auto* renderView = this->renderView()) {
+        ASSERT(!renderView->inHitTesting());
         renderView->setNeedsLayout();
+    }
 }
 

trunk/Source/WebCore/rendering/RenderView.cpp

@@ -53 +53 @@
 #include "TransformState.h"
 #include <wtf/StackStats.h>
+#include <wtf/TemporaryChange.h>
 
 namespace WebCore {
@@ -184 +185 @@
 {
     document().updateLayout();
+    
+#if !ASSERT_DISABLED
+    TemporaryChange<bool> hitTestRestorer { m_inHitTesting, true };
+#endif
 
     FrameFlatteningLayoutDisallower disallower(frameView());

trunk/Source/WebCore/rendering/RenderView.h

@@ -1 +1 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
- * Copyright (C) 2006, 2015 Apple Inc.
+ * Copyright (C) 2006, 2015-2016 Apple Inc.
  *
  * This library is free software; you can redistribute it and/or
@@ -251 +251 @@
 #endif
 
+#if !ASSERT_DISABLED
+    bool inHitTesting() const { return m_inHitTesting; }
+#endif
+
 protected:
     void mapLocalToContainer(const RenderLayerModelObject* repaintContainer, TransformState&, MapCoordinatesFlags, bool* wasFixed) const override;
@@ -361 +365 @@
     bool m_usesFirstLineRules { false };
     bool m_usesFirstLetterRules { false };
+#if !ASSERT_DISABLED
+    bool m_inHitTesting { false };
+#endif
 
     HashSet<RenderElement*> m_renderersWithPausedImageAnimation;