Crash under RenderLayer::scrollTo() with marquee
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 7 Jan 2018 05:48:47 +0000 (05:48 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 7 Jan 2018 05:48:47 +0000 (05:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181349
rdar://problem/36190168

Reviewed by Zalan Bujtas.

Source/WebCore:

Don't call updateWidgetPositions() synchonously during RenderLayer scrolling, because it
can run arbitrary script which may trigger destruction of this RenderLayer.

Instead, queue up updateWidgetPositions() on a zero-delay timer.

Under some circumstances this may allow a paint to occur before the widgets have been
updated (which could be fixed with a more invasive change), but in practice I saw no
painting issues with plug-ins or iframes inside overflow scroll, in WebKit or LegacyWebKit.

Test: fast/scrolling/marquee-scroll-crash.html

* page/FrameView.cpp:
(WebCore::FrameView::FrameView):
(WebCore::FrameView::updateWidgetPositions):
(WebCore::FrameView::scheduleUpdateWidgetPositions):
(WebCore::FrameView::updateWidgetPositionsTimerFired):
* page/FrameView.h:
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::scrollTo):

LayoutTests:

* fast/scrolling/marquee-scroll-crash-expected.txt: Added.
* fast/scrolling/marquee-scroll-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/scrolling/marquee-scroll-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/marquee-scroll-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderLayer.cpp

index 1e6f6ce..f56c2c8 100644 (file)
@@ -1,3 +1,14 @@
+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
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/scrolling/marquee-scroll-crash-expected.txt: Added.
+        * fast/scrolling/marquee-scroll-crash.html: Added.
+
 2018-01-05  Dean Jackson  <dino@apple.com>
 
         Accurately clip copyTexImage2D and copyTexSubImage2D
diff --git a/LayoutTests/fast/scrolling/marquee-scroll-crash-expected.txt b/LayoutTests/fast/scrolling/marquee-scroll-crash-expected.txt
new file mode 100644 (file)
index 0000000..0383162
--- /dev/null
@@ -0,0 +1 @@
+Test passes if it does not crash.
diff --git a/LayoutTests/fast/scrolling/marquee-scroll-crash.html b/LayoutTests/fast/scrolling/marquee-scroll-crash.html
new file mode 100644 (file)
index 0000000..05c9109
--- /dev/null
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html>
+    <head>
+    <script>
+        if (window.testRunner)
+            testRunner.dumpAsText();
+
+        function testcase()
+        {
+            var iframe = document.getElementById('iframe');
+            var frameDocElement = iframe.contentDocument.documentElement;
+            frameDocElement.innerHTML = '<object>cc</object>';
+            frameDocElement.addEventListener('beforeload', frameBeforeLoad, true);
+            window.getSelection().selectAllChildren(document.body);
+        }
+
+        function frameBeforeLoad(event)
+        {
+            document.write('Test passes if it does not crash.');
+        }
+    </script>
+    </head>
+    <body onload='testcase();'>
+        <iframe id='iframe'></iframe>
+        <marquee>marquee</marquee>
+    </body>
+</html>
index 2d01d99..d253c41 100644 (file)
@@ -1,3 +1,31 @@
+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
+
+        Reviewed by Zalan Bujtas.
+
+        Don't call updateWidgetPositions() synchonously during RenderLayer scrolling, because it
+        can run arbitrary script which may trigger destruction of this RenderLayer.
+
+        Instead, queue up updateWidgetPositions() on a zero-delay timer.
+
+        Under some circumstances this may allow a paint to occur before the widgets have been
+        updated (which could be fixed with a more invasive change), but in practice I saw no
+        painting issues with plug-ins or iframes inside overflow scroll, in WebKit or LegacyWebKit.
+
+        Test: fast/scrolling/marquee-scroll-crash.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::FrameView):
+        (WebCore::FrameView::updateWidgetPositions):
+        (WebCore::FrameView::scheduleUpdateWidgetPositions):
+        (WebCore::FrameView::updateWidgetPositionsTimerFired):
+        * page/FrameView.h:
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::scrollTo):
+
 2018-01-05  Dean Jackson  <dino@apple.com>
 
         Accurately clip copyTexImage2D and copyTexSubImage2D
index df66909..e4c1dca 100644 (file)
@@ -171,6 +171,7 @@ FrameView::FrameView(Frame& frame)
     : m_frame(frame)
     , m_canHaveScrollbars(true)
     , m_updateEmbeddedObjectsTimer(*this, &FrameView::updateEmbeddedObjectsTimerFired)
+    , m_updateWidgetPositionsTimer(*this, &FrameView::updateWidgetPositionsTimerFired)
     , m_isTransparent(false)
     , m_baseBackgroundColor(Color::white)
     , m_mediaType("screen")
@@ -4908,6 +4909,7 @@ static Vector<RefPtr<Widget>> collectAndProtectWidgets(const HashSet<Widget*>& s
 
 void FrameView::updateWidgetPositions()
 {
+    m_updateWidgetPositionsTimer.stop();
     // updateWidgetPosition() can possibly cause layout to be re-entered (via plug-ins running
     // scripts in response to NPP_SetWindow, for example), so we need to keep the Widgets
     // alive during enumeration.
@@ -4919,6 +4921,17 @@ void FrameView::updateWidgetPositions()
     }
 }
 
+void FrameView::scheduleUpdateWidgetPositions()
+{
+    if (!m_updateWidgetPositionsTimer.isActive())
+        m_updateWidgetPositionsTimer.startOneShot(0_s);
+}
+
+void FrameView::updateWidgetPositionsTimerFired()
+{
+    updateWidgetPositions();
+}
+
 void FrameView::notifyWidgets(WidgetNotification notification)
 {
     for (auto& widget : collectAndProtectWidgets(m_widgetsInRenderTree))
index ea459fd..630aee4 100644 (file)
@@ -595,6 +595,8 @@ public:
     void setHasFlippedBlockRenderers(bool b) { m_hasFlippedBlockRenderers = b; }
 
     void updateWidgetPositions();
+    void scheduleUpdateWidgetPositions();
+
     void didAddWidgetToRenderTree(Widget&);
     void willRemoveWidgetFromRenderTree(Widget&);
 
@@ -742,6 +744,9 @@ private:
     void updateEmbeddedObjectsTimerFired();
     bool updateEmbeddedObjects();
     void updateEmbeddedObject(RenderEmbeddedObject&);
+
+    void updateWidgetPositionsTimerFired();
+
     void scrollToAnchor();
     void scrollPositionChanged(const ScrollPosition& oldPosition, const ScrollPosition& newPosition);
     void scrollableAreaSetChanged();
@@ -792,6 +797,8 @@ private:
     bool m_contentIsOpaque;
 
     Timer m_updateEmbeddedObjectsTimer;
+    Timer m_updateWidgetPositionsTimer;
+
     bool m_firstLayoutCallbackPending;
 
     bool m_isTransparent;
index 25e13b4..92c5429 100644 (file)
@@ -2461,7 +2461,7 @@ void RenderLayer::scrollTo(const ScrollPosition& position)
 #if ENABLE(DASHBOARD_SUPPORT)
         view.frameView().updateAnnotatedRegions();
 #endif
-        view.frameView().updateWidgetPositions();
+        view.frameView().scheduleUpdateWidgetPositions();
 
         if (!m_updatingMarqueePosition) {
             // Avoid updating compositing layers if, higher on the stack, we're already updating layer