Possible crash computing event regions
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 7 Jan 2018 06:31:14 +0000 (06:31 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 7 Jan 2018 06:31:14 +0000 (06:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181368
rdar://problem/34847081

Reviewed by Zalan Bujtas.

Source/WebCore:

Don't trigger layout in Element::absoluteEventHandlerBounds(), since this can run arbirary script
which might delete elements or re-enter Document::absoluteRegionForEventTargets().

It's OK to not trigger layout, because if layout is dirty, the next layout will update event regions again.

Add a LayoutDisallowedScope to check that Document::absoluteRegionForEventTargets() doesn't
trigger layout, and move the check for LayoutDisallowedScope::isLayoutAllowed() from Document::updateLayout()
to LayoutContext::layout(), since some layouts don't happen via the former (e.g. the one being removed here).

The test checks that the assertion does not fire. I was not able to get a reliable test for any crash.

Test: fast/events/event-handler-regions-layout.html

* dom/Document.cpp:
(WebCore::Document::updateLayout):
(WebCore::Document::absoluteRegionForEventTargets):
* dom/Element.cpp:
(WebCore::Element::absoluteEventHandlerBounds):
* page/LayoutContext.cpp:
(WebCore::LayoutContext::layout):
* rendering/LayoutDisallowedScope.h: Move the #ifdefs around to avoid defining the enum twice.
(WebCore::LayoutDisallowedScope::LayoutDisallowedScope):
(WebCore::LayoutDisallowedScope::isLayoutAllowed):

LayoutTests:

* fast/events/event-handler-regions-layout-expected.txt: Added.
* fast/events/event-handler-regions-layout.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/events/event-handler-regions-layout-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/event-handler-regions-layout.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Element.cpp
Source/WebCore/page/LayoutContext.cpp
Source/WebCore/rendering/LayoutDisallowedScope.h

index f56c2c8..774fa83 100644 (file)
@@ -1,5 +1,16 @@
 2018-01-06  Simon Fraser  <simon.fraser@apple.com>
 
+        Possible crash computing event regions
+        https://bugs.webkit.org/show_bug.cgi?id=181368
+        rdar://problem/34847081
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/events/event-handler-regions-layout-expected.txt: Added.
+        * fast/events/event-handler-regions-layout.html: Added.
+
+2018-01-06  Simon Fraser  <simon.fraser@apple.com>
+
         Crash under RenderLayer::scrollTo() with marquee
         https://bugs.webkit.org/show_bug.cgi?id=181349
         rdar://problem/36190168
diff --git a/LayoutTests/fast/events/event-handler-regions-layout-expected.txt b/LayoutTests/fast/events/event-handler-regions-layout-expected.txt
new file mode 100644 (file)
index 0000000..efe8f10
--- /dev/null
@@ -0,0 +1,3 @@
+This test should not assert in debug builds.
+
+  
diff --git a/LayoutTests/fast/events/event-handler-regions-layout.html b/LayoutTests/fast/events/event-handler-regions-layout.html
new file mode 100644 (file)
index 0000000..01ddd5c
--- /dev/null
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script>
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    function doTest()
+    {
+        var iframe = document.getElementById('iframe');
+        var frameDocElement = iframe.contentDocument.documentElement;
+        frameDocElement.innerHTML = '<object></object>';
+        frameDocElement.addEventListener('beforeload', frameBeforeLoad, true);
+        frameDocElement.offsetWidth;
+    }
+    
+    function frameBeforeLoad()
+    {
+        var wrapper = document.getElementById('wrapper');
+        document.getElementById('destination_frame').contentDocument.body.appendChild(wrapper);
+
+        if (window.testRunner)
+            testRunner.notifyDone();
+    }
+
+    window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    <p>This test should not assert in debug builds.</p>
+    <iframe id='iframe'></iframe>
+    <iframe id='destination_frame' srcdoc="<body>Test</body>"></iframe>
+    <div id="wrapper">
+        <div id="wheelie">wheel handler</div>
+    </div>
+</div>
+<script>
+    document.getElementById('wheelie').addEventListener('mousewheel', function(e) { });
+</script>
+</body>
+</html>
index d253c41..3cdf793 100644 (file)
@@ -1,5 +1,37 @@
 2018-01-06  Simon Fraser  <simon.fraser@apple.com>
 
+        Possible crash computing event regions
+        https://bugs.webkit.org/show_bug.cgi?id=181368
+        rdar://problem/34847081
+
+        Reviewed by Zalan Bujtas.
+
+        Don't trigger layout in Element::absoluteEventHandlerBounds(), since this can run arbirary script
+        which might delete elements or re-enter Document::absoluteRegionForEventTargets().
+
+        It's OK to not trigger layout, because if layout is dirty, the next layout will update event regions again.
+
+        Add a LayoutDisallowedScope to check that Document::absoluteRegionForEventTargets() doesn't
+        trigger layout, and move the check for LayoutDisallowedScope::isLayoutAllowed() from Document::updateLayout()
+        to LayoutContext::layout(), since some layouts don't happen via the former (e.g. the one being removed here).
+
+        The test checks that the assertion does not fire. I was not able to get a reliable test for any crash.
+
+        Test: fast/events/event-handler-regions-layout.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::updateLayout):
+        (WebCore::Document::absoluteRegionForEventTargets):
+        * dom/Element.cpp:
+        (WebCore::Element::absoluteEventHandlerBounds):
+        * page/LayoutContext.cpp:
+        (WebCore::LayoutContext::layout):
+        * rendering/LayoutDisallowedScope.h: Move the #ifdefs around to avoid defining the enum twice.
+        (WebCore::LayoutDisallowedScope::LayoutDisallowedScope):
+        (WebCore::LayoutDisallowedScope::isLayoutAllowed):
+
+2018-01-06  Simon Fraser  <simon.fraser@apple.com>
+
         Crash under RenderLayer::scrollTo() with marquee
         https://bugs.webkit.org/show_bug.cgi?id=181349
         rdar://problem/36190168
index db04736..7d2f9d2 100644 (file)
@@ -1969,7 +1969,6 @@ bool Document::updateStyleIfNeeded()
 void Document::updateLayout()
 {
     ASSERT(isMainThread());
-    ASSERT(LayoutDisallowedScope::isLayoutAllowed());
 
     RefPtr<FrameView> frameView = view();
     if (frameView && frameView->layoutContext().isInRenderTreeLayout()) {
@@ -6645,6 +6644,8 @@ LayoutRect Document::absoluteEventHandlerBounds(bool& includesFixedPositionEleme
 
 Document::RegionFixedPair Document::absoluteRegionForEventTargets(const EventTargetSet* targets)
 {
+    LayoutDisallowedScope layoutDisallowedScope(LayoutDisallowedScope::Reason::ReentrancyAvoidance);
+
     if (!targets)
         return RegionFixedPair(Region(), false);
 
index 30d4bd2..18b85bd 100644 (file)
@@ -1141,9 +1141,6 @@ LayoutRect Element::absoluteEventHandlerBounds(bool& includesFixedPositionElemen
     if (!frameView)
         return LayoutRect();
 
-    if (frameView->needsLayout())
-        frameView->layoutContext().layout();
-
     return absoluteEventBoundsOfElementAndDescendants(includesFixedPositionElements);
 }
 
index 5de7d06..ce7c3b3 100644 (file)
@@ -31,6 +31,7 @@
 #include "Document.h"
 #include "FrameView.h"
 #include "InspectorInstrumentation.h"
+#include "LayoutDisallowedScope.h"
 #include "LayoutState.h"
 #include "Logging.h"
 #include "RenderElement.h"
@@ -122,6 +123,7 @@ void LayoutContext::layout()
     LOG_WITH_STREAM(Layout, stream << "FrameView " << &view() << " LayoutContext::layout() with size " << view().layoutSize());
 
     RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!frame().document()->inRenderTreeUpdate());
+    ASSERT(LayoutDisallowedScope::isLayoutAllowed());
     ASSERT(!view().isPainting());
     ASSERT(frame().view() == &view());
     ASSERT(frame().document());
index bd162f5..7d8739f 100644 (file)
 
 namespace WebCore {
 
-#if !ASSERT_DISABLED
-
 class LayoutDisallowedScope {
 public:
-    enum class Reason { PerformanceOptimization };
+    enum class Reason { PerformanceOptimization, ReentrancyAvoidance };
+
+#if ASSERT_DISABLED
+    LayoutDisallowedScope(Reason) { }
+    static bool isLayoutAllowed() { return true; }
+#else
     LayoutDisallowedScope(Reason)
         : m_previousAssertion(s_currentAssertion)
     {
@@ -48,17 +51,7 @@ public:
 private:
     LayoutDisallowedScope* m_previousAssertion;
     static LayoutDisallowedScope* s_currentAssertion;
-};
-
-#else
-
-class LayoutDisallowedScope {
-public:
-    enum class Reason { PerformanceOptimization };
-    LayoutDisallowedScope(Reason) { }
-    static bool isLayoutAllowed() { return true; }
-};
-
 #endif
+};
 
 }