Remove slow repaint object from FrameView when style changes.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 Nov 2017 17:18:57 +0000 (17:18 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 Nov 2017 17:18:57 +0000 (17:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=179871

Reviewed by Antti Koivisto.

Source/WebCore:

The "oldStyleSlowScroll" value does not need to be computed. We already know its value
by checking the HashSet. This code is also unnecessarily complicated and error prone
(could lead to UAF errors by leaving stale renderers in the slow paint list).

Test: fast/repaint/slow-repaint-object-crash.html

* rendering/RenderElement.cpp:
(WebCore::RenderElement::styleWillChange):

LayoutTests:

* fast/repaint/slow-repaint-object-crash-expected.txt: Added.
* fast/repaint/slow-repaint-object-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/repaint/slow-repaint-object-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/repaint/slow-repaint-object-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderElement.cpp

index a677da2..329ca42 100644 (file)
@@ -1,3 +1,13 @@
+2017-11-20  Zalan Bujtas  <zalan@apple.com>
+
+        Remove slow repaint object from FrameView when style changes.
+        https://bugs.webkit.org/show_bug.cgi?id=179871
+
+        Reviewed by Antti Koivisto.
+
+        * fast/repaint/slow-repaint-object-crash-expected.txt: Added.
+        * fast/repaint/slow-repaint-object-crash.html: Added.
+
 2017-11-19  Ms2ger  <Ms2ger@igalia.com>
 
         [WPE] Enable the XMLHttpRequest/ directory of web-platform-tests.
diff --git a/LayoutTests/fast/repaint/slow-repaint-object-crash-expected.txt b/LayoutTests/fast/repaint/slow-repaint-object-crash-expected.txt
new file mode 100644 (file)
index 0000000..8b13789
--- /dev/null
@@ -0,0 +1 @@
+
diff --git a/LayoutTests/fast/repaint/slow-repaint-object-crash.html b/LayoutTests/fast/repaint/slow-repaint-object-crash.html
new file mode 100644 (file)
index 0000000..3ac9581
--- /dev/null
@@ -0,0 +1,24 @@
+<html>
+<head>
+<style>
+html, body { 
+    background: url() fixed;
+}
+</style>
+<script>
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+function runTest() {
+    var range = document.createRange();
+    range.setEnd(span, 1);
+    range.extractContents();
+    window.scrollBy(0, 100);
+    document.body.style.display = "none";
+}
+</script>
+</head>
+<body onload=runTest()>
+<span id=span>Pass if no crash or assert.</span>
+</body>
+</html>
index 54e159a..9201ca1 100644 (file)
@@ -1,3 +1,19 @@
+2017-11-20  Zalan Bujtas  <zalan@apple.com>
+
+        Remove slow repaint object from FrameView when style changes.
+        https://bugs.webkit.org/show_bug.cgi?id=179871
+
+        Reviewed by Antti Koivisto.
+
+        The "oldStyleSlowScroll" value does not need to be computed. We already know its value
+        by checking the HashSet. This code is also unnecessarily complicated and error prone
+        (could lead to UAF errors by leaving stale renderers in the slow paint list).
+
+        Test: fast/repaint/slow-repaint-object-crash.html
+
+        * rendering/RenderElement.cpp:
+        (WebCore::RenderElement::styleWillChange):
+
 2017-11-20  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [WPE] GLContextEGLWPE.cpp:44:96: error: invalid cast from type ‘GLNativeWindowType {aka long long unsigned int}’ to type ‘EGLNativeWindowType {aka unsigned int}
index 4219cec..c0c6977 100644 (file)
@@ -1448,39 +1448,40 @@ void FrameView::setCannotBlitToWindow()
     updateCanBlitOnScrollRecursively();
 }
 
-void FrameView::addSlowRepaintObject(RenderElement* o)
+void FrameView::addSlowRepaintObject(RenderElement& renderer)
 {
     bool hadSlowRepaintObjects = hasSlowRepaintObjects();
 
     if (!m_slowRepaintObjects)
         m_slowRepaintObjects = std::make_unique<HashSet<const RenderElement*>>();
 
-    m_slowRepaintObjects->add(o);
+    m_slowRepaintObjects->add(&renderer);
+    if (hadSlowRepaintObjects)
+        return;
 
-    if (!hadSlowRepaintObjects) {
-        updateCanBlitOnScrollRecursively();
+    updateCanBlitOnScrollRecursively();
 
-        if (Page* page = frame().page()) {
-            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
-                scrollingCoordinator->frameViewHasSlowRepaintObjectsDidChange(*this);
-        }
+    if (auto* page = frame().page()) {
+        if (auto* scrollingCoordinator = page->scrollingCoordinator())
+            scrollingCoordinator->frameViewHasSlowRepaintObjectsDidChange(*this);
     }
 }
 
-void FrameView::removeSlowRepaintObject(RenderElement* o)
+void FrameView::removeSlowRepaintObject(RenderElement& renderer)
 {
     if (!m_slowRepaintObjects)
         return;
 
-    m_slowRepaintObjects->remove(o);
-    if (m_slowRepaintObjects->isEmpty()) {
-        m_slowRepaintObjects = nullptr;
-        updateCanBlitOnScrollRecursively();
+    m_slowRepaintObjects->remove(&renderer);
+    if (!m_slowRepaintObjects->isEmpty())
+        return;
 
-        if (Page* page = frame().page()) {
-            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
-                scrollingCoordinator->frameViewHasSlowRepaintObjectsDidChange(*this);
-        }
+    m_slowRepaintObjects = nullptr;
+    updateCanBlitOnScrollRecursively();
+
+    if (auto* page = frame().page()) {
+        if (auto* scrollingCoordinator = page->scrollingCoordinator())
+            scrollingCoordinator->frameViewHasSlowRepaintObjectsDidChange(*this);
     }
 }
 
index 0ee7118..80fc296 100644 (file)
@@ -273,8 +273,8 @@ public:
     void setIsOverlapped(bool);
     void setContentIsOpaque(bool);
 
-    void addSlowRepaintObject(RenderElement*);
-    void removeSlowRepaintObject(RenderElement*);
+    void addSlowRepaintObject(RenderElement&);
+    void removeSlowRepaintObject(RenderElement&);
     bool hasSlowRepaintObject(const RenderElement& renderer) const { return m_slowRepaintObjects && m_slowRepaintObjects->contains(&renderer); }
     bool hasSlowRepaintObjects() const { return m_slowRepaintObjects && m_slowRepaintObjects->size(); }
 
index 1f5f66d..c382680 100644 (file)
@@ -913,32 +913,20 @@ void RenderElement::styleWillChange(StyleDifference diff, const RenderStyle& new
         s_noLongerAffectsParentBlock = false;
     }
 
-    bool newStyleUsesFixedBackgrounds = newStyle.hasFixedBackgroundImage();
-    bool oldStyleUsesFixedBackgrounds = m_style.hasFixedBackgroundImage();
-    if (newStyleUsesFixedBackgrounds || oldStyleUsesFixedBackgrounds) {
-        bool repaintFixedBackgroundsOnScroll = !settings().fixedBackgroundsPaintRelativeToDocument();
-        bool newStyleSlowScroll = repaintFixedBackgroundsOnScroll && newStyleUsesFixedBackgrounds;
-        bool oldStyleSlowScroll = oldStyle && repaintFixedBackgroundsOnScroll && oldStyleUsesFixedBackgrounds;
+    bool newStyleSlowScroll = false;
+    if (newStyle.hasFixedBackgroundImage() && !settings().fixedBackgroundsPaintRelativeToDocument()) {
+        newStyleSlowScroll = true;
         bool drawsRootBackground = isDocumentElementRenderer() || (isBody() && !rendererHasBackground(document().documentElement()->renderer()));
-        if (drawsRootBackground && repaintFixedBackgroundsOnScroll) {
-            if (view().compositor().supportsFixedRootBackgroundCompositing()) {
-                if (newStyleSlowScroll && newStyle.hasEntirelyFixedBackground())
-                    newStyleSlowScroll = false;
-
-                if (oldStyleSlowScroll && m_style.hasEntirelyFixedBackground())
-                    oldStyleSlowScroll = false;
-            }
-        }
-
-        if (oldStyleSlowScroll != newStyleSlowScroll) {
-            if (oldStyleSlowScroll)
-                view().frameView().removeSlowRepaintObject(this);
-
-            if (newStyleSlowScroll)
-                view().frameView().addSlowRepaintObject(this);
-        }
+        if (drawsRootBackground && newStyle.hasEntirelyFixedBackground() && view().compositor().supportsFixedRootBackgroundCompositing())
+            newStyleSlowScroll = false;
     }
 
+    if (view().frameView().hasSlowRepaintObject(*this)) {
+        if (!newStyleSlowScroll)
+            view().frameView().removeSlowRepaintObject(*this);
+    } else if (newStyleSlowScroll)
+        view().frameView().addSlowRepaintObject(*this);
+
     if (isDocumentElementRenderer() || isBody())
         view().frameView().updateExtendBackgroundIfNecessary();
 }
@@ -1124,7 +1112,7 @@ inline void RenderElement::clearSubtreeLayoutRootIfNeeded() const
 void RenderElement::willBeDestroyed()
 {
     if (m_style.hasFixedBackgroundImage() && !settings().fixedBackgroundsPaintRelativeToDocument())
-        view().frameView().removeSlowRepaintObject(this);
+        view().frameView().removeSlowRepaintObject(*this);
 
     unregisterForVisibleInViewportCallback();