- Revision
- 226492
- Author
- [email protected]
- Date
- 2018-01-06 22:31:14 -0800 (Sat, 06 Jan 2018)
Log Message
Possible crash computing event regions
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (226491 => 226492)
--- trunk/LayoutTests/ChangeLog 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/LayoutTests/ChangeLog 2018-01-07 06:31:14 UTC (rev 226492)
@@ -1,5 +1,16 @@
2018-01-06 Simon Fraser <[email protected]>
+ 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 <[email protected]>
+
Crash under RenderLayer::scrollTo() with marquee
https://bugs.webkit.org/show_bug.cgi?id=181349
rdar://problem/36190168
Added: trunk/LayoutTests/fast/events/event-handler-regions-layout-expected.txt (0 => 226492)
--- trunk/LayoutTests/fast/events/event-handler-regions-layout-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/events/event-handler-regions-layout-expected.txt 2018-01-07 06:31:14 UTC (rev 226492)
@@ -0,0 +1,3 @@
+This test should not assert in debug builds.
+
+
Added: trunk/LayoutTests/fast/events/event-handler-regions-layout.html (0 => 226492)
--- trunk/LayoutTests/fast/events/event-handler-regions-layout.html (rev 0)
+++ trunk/LayoutTests/fast/events/event-handler-regions-layout.html 2018-01-07 06:31:14 UTC (rev 226492)
@@ -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>
Modified: trunk/Source/WebCore/ChangeLog (226491 => 226492)
--- trunk/Source/WebCore/ChangeLog 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/Source/WebCore/ChangeLog 2018-01-07 06:31:14 UTC (rev 226492)
@@ -1,5 +1,37 @@
2018-01-06 Simon Fraser <[email protected]>
+ 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 <[email protected]>
+
Crash under RenderLayer::scrollTo() with marquee
https://bugs.webkit.org/show_bug.cgi?id=181349
rdar://problem/36190168
Modified: trunk/Source/WebCore/dom/Document.cpp (226491 => 226492)
--- trunk/Source/WebCore/dom/Document.cpp 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/Source/WebCore/dom/Document.cpp 2018-01-07 06:31:14 UTC (rev 226492)
@@ -1969,7 +1969,6 @@
void Document::updateLayout()
{
ASSERT(isMainThread());
- ASSERT(LayoutDisallowedScope::isLayoutAllowed());
RefPtr<FrameView> frameView = view();
if (frameView && frameView->layoutContext().isInRenderTreeLayout()) {
@@ -6645,6 +6644,8 @@
Document::RegionFixedPair Document::absoluteRegionForEventTargets(const EventTargetSet* targets)
{
+ LayoutDisallowedScope layoutDisallowedScope(LayoutDisallowedScope::Reason::ReentrancyAvoidance);
+
if (!targets)
return RegionFixedPair(Region(), false);
Modified: trunk/Source/WebCore/dom/Element.cpp (226491 => 226492)
--- trunk/Source/WebCore/dom/Element.cpp 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/Source/WebCore/dom/Element.cpp 2018-01-07 06:31:14 UTC (rev 226492)
@@ -1141,9 +1141,6 @@
if (!frameView)
return LayoutRect();
- if (frameView->needsLayout())
- frameView->layoutContext().layout();
-
return absoluteEventBoundsOfElementAndDescendants(includesFixedPositionElements);
}
Modified: trunk/Source/WebCore/page/LayoutContext.cpp (226491 => 226492)
--- trunk/Source/WebCore/page/LayoutContext.cpp 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/Source/WebCore/page/LayoutContext.cpp 2018-01-07 06:31:14 UTC (rev 226492)
@@ -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 @@
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());
Modified: trunk/Source/WebCore/rendering/LayoutDisallowedScope.h (226491 => 226492)
--- trunk/Source/WebCore/rendering/LayoutDisallowedScope.h 2018-01-07 05:48:47 UTC (rev 226491)
+++ trunk/Source/WebCore/rendering/LayoutDisallowedScope.h 2018-01-07 06:31:14 UTC (rev 226492)
@@ -27,11 +27,14 @@
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 @@
private:
LayoutDisallowedScope* m_previousAssertion;
static LayoutDisallowedScope* s_currentAssertion;
+#endif
};
-#else
-
-class LayoutDisallowedScope {
-public:
- enum class Reason { PerformanceOptimization };
- LayoutDisallowedScope(Reason) { }
- static bool isLayoutAllowed() { return true; }
-};
-
-#endif
-
}
