Assert in RenderGeometryMap::mapToContainer
authorachicu@adobe.com <achicu@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Jan 2013 02:57:50 +0000 (02:57 +0000)
committerachicu@adobe.com <achicu@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 9 Jan 2013 02:57:50 +0000 (02:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=106068

Reviewed by Simon Fraser.

Source/WebCore:

The assert was due to a pending layout, so the values used to compute the layer bounding boxes were incorrect.
That was because of the Document::setVisualUpdatesAllowed mechanism, which triggers a compositor update
and a repaint, but before this patch didn't check whether a layout was pending or not.

Added a check in Document::setVisualUpdatesAllowed for pending layouts and bailed when such case happened.
A layout will come anyway and trigger the correct updates. Couldn't not force an inline layout at that time
as this function is sometimes called really soon, when the WebKit parts are not fully created yet and updates were
calling back into some client callbacks that were not ready.

Also added an assert in RenderLayerCompositor::updateCompositingLayers to check for other cases that might
try to update the layers with a layout pending. That one led to finding an issue in the RenderMarquee, which
was updating on a timer callback. It might happen that a layout is pending while this timer fires and it
tries to update the scroll position of the layers while a layout is still due.

There was already a protection to bail if a layout is pending in RenderMarquee::timerFired, so I've just broadened the scope
to the whole RenderView to catch all the layout requests.

Tests: compositing/geometry/assert-layout-not-done.html
       compositing/geometry/assert-marquee-timer.html

* dom/Document.cpp:
(WebCore::Document::setVisualUpdatesAllowed):
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::updateCompositingLayers):
* rendering/RenderMarquee.cpp:
(WebCore::RenderMarquee::timerFired):

LayoutTests:

* compositing/geometry/assert-layout-not-done-expected.txt: Added.
* compositing/geometry/assert-layout-not-done.html: Added. Testing for the case when compositor is triggered before the first layout.
* compositing/geometry/assert-marquee-timer-expected.txt: Added.
* compositing/geometry/assert-marquee-timer.html: Added. Tested for the case when the marquee might trigger compositor updates, while a layout is still pending.

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

LayoutTests/ChangeLog
LayoutTests/compositing/geometry/assert-layout-not-done-expected.txt [new file with mode: 0644]
LayoutTests/compositing/geometry/assert-layout-not-done.html [new file with mode: 0644]
LayoutTests/compositing/geometry/assert-marquee-timer-expected.txt [new file with mode: 0644]
LayoutTests/compositing/geometry/assert-marquee-timer.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp
Source/WebCore/rendering/RenderMarquee.cpp

index 78cb156..92312f9 100644 (file)
@@ -1,3 +1,15 @@
+2013-01-08  Alexandru Chiculita  <achicu@adobe.com>
+
+        Assert in RenderGeometryMap::mapToContainer
+        https://bugs.webkit.org/show_bug.cgi?id=106068
+
+        Reviewed by Simon Fraser.
+
+        * compositing/geometry/assert-layout-not-done-expected.txt: Added.
+        * compositing/geometry/assert-layout-not-done.html: Added. Testing for the case when compositor is triggered before the first layout.
+        * compositing/geometry/assert-marquee-timer-expected.txt: Added.
+        * compositing/geometry/assert-marquee-timer.html: Added. Tested for the case when the marquee might trigger compositor updates, while a layout is still pending.
+
 2013-01-08  Brandon Jones  <bajones@chromium.org>
 
         Make WebGLRenderingContext inherit from ActiveDOMObject
diff --git a/LayoutTests/compositing/geometry/assert-layout-not-done-expected.txt b/LayoutTests/compositing/geometry/assert-layout-not-done-expected.txt
new file mode 100644 (file)
index 0000000..8b13789
--- /dev/null
@@ -0,0 +1 @@
+
diff --git a/LayoutTests/compositing/geometry/assert-layout-not-done.html b/LayoutTests/compositing/geometry/assert-layout-not-done.html
new file mode 100644 (file)
index 0000000..55ebf9c
--- /dev/null
@@ -0,0 +1,27 @@
+<!doctype html>
+<html>
+    <style>
+    .container {
+        position: relative;
+        z-index: 0;
+        top: 123px; /* easy to detect values  */
+        left: 123px; /* easy to detect values */
+    }
+    #child {
+        width: 100px;
+        height: 100px;
+        background-color: gray;
+        -webkit-transform: translateZ(0px);
+    }
+    </style>
+    <script>
+        if (window.testRunner)
+            testRunner.dumpAsText(false);
+    </script>
+    <body>
+        <!-- This test passes if there's no ASSERT in debug mode. There should be a gray rectangle on screen. -->
+        <div class="container">
+            <div id="child"></div>
+        </div>
+    </body>
+</html>
\ No newline at end of file
diff --git a/LayoutTests/compositing/geometry/assert-marquee-timer-expected.txt b/LayoutTests/compositing/geometry/assert-marquee-timer-expected.txt
new file mode 100644 (file)
index 0000000..8110c03
--- /dev/null
@@ -0,0 +1 @@
+Counter:
diff --git a/LayoutTests/compositing/geometry/assert-marquee-timer.html b/LayoutTests/compositing/geometry/assert-marquee-timer.html
new file mode 100644 (file)
index 0000000..b0c4212
--- /dev/null
@@ -0,0 +1,54 @@
+<!doctype html>
+<html>
+    <head>
+        <style>
+        .content {
+            -webkit-transform: translateZ(1px);
+            display: block;
+            width: 100px;
+            height: 30px;
+            background-color: green;
+        }
+        </style>
+    </head>
+    <body>
+        <!-- This test passes if it doesn't trigger an ASSERT in debug mode. It makes sure that the timer
+            used to update the marquee will not run before the layout is done. -->
+        
+        <div id="container">
+            <marquee behavior="scroll" direction="left" scrolldelay="1" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="2" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="3" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="4" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="5" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="6" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="7" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+            <marquee behavior="scroll" direction="left" scrolldelay="8" truespeed="true" scrollamount="1"><span class="content"></span></marquee>
+        </div>
+
+        <p>Counter: <span id="counter"></span></p>
+
+        <script>
+            if (window.testRunner) {
+                testRunner.waitUntilDone();
+                testRunner.dumpAsText(false);
+            }
+
+            var times = 0;
+            // Trigger "pending layouts", that might run after the marquee timer is fired.
+            var counter = document.getElementById("counter");
+            var textNode = document.createTextNode();
+            counter.appendChild(textNode);
+            var interval = setInterval(function() {
+                textNode.nodeValue = Math.random();
+                if ((++times) >= 10) {
+                    document.getElementById("container").remove();
+                    textNode.nodeValue = "";
+                    clearInterval(interval);
+                    if (window.testRunner)
+                        testRunner.notifyDone();
+                }
+            }, 10);
+        </script>
+    </body>
+</html>
\ No newline at end of file
index 5a8e313..fccabe5 100644 (file)
@@ -1,3 +1,37 @@
+2013-01-08  Alexandru Chiculita  <achicu@adobe.com>
+
+        Assert in RenderGeometryMap::mapToContainer
+        https://bugs.webkit.org/show_bug.cgi?id=106068
+
+        Reviewed by Simon Fraser.
+
+        The assert was due to a pending layout, so the values used to compute the layer bounding boxes were incorrect.
+        That was because of the Document::setVisualUpdatesAllowed mechanism, which triggers a compositor update
+        and a repaint, but before this patch didn't check whether a layout was pending or not.
+
+        Added a check in Document::setVisualUpdatesAllowed for pending layouts and bailed when such case happened.
+        A layout will come anyway and trigger the correct updates. Couldn't not force an inline layout at that time
+        as this function is sometimes called really soon, when the WebKit parts are not fully created yet and updates were
+        calling back into some client callbacks that were not ready.
+
+        Also added an assert in RenderLayerCompositor::updateCompositingLayers to check for other cases that might
+        try to update the layers with a layout pending. That one led to finding an issue in the RenderMarquee, which
+        was updating on a timer callback. It might happen that a layout is pending while this timer fires and it 
+        tries to update the scroll position of the layers while a layout is still due.
+
+        There was already a protection to bail if a layout is pending in RenderMarquee::timerFired, so I've just broadened the scope
+        to the whole RenderView to catch all the layout requests.
+
+        Tests: compositing/geometry/assert-layout-not-done.html
+               compositing/geometry/assert-marquee-timer.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::setVisualUpdatesAllowed):
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::updateCompositingLayers):
+        * rendering/RenderMarquee.cpp:
+        (WebCore::RenderMarquee::timerFired):
+
 2013-01-08  Justin Novosad  <junov@google.com>
 
         CanvasRenderingContext2D::setFont argument may reference destroyed object
index 19d7dad..6ad06f2 100644 (file)
@@ -1260,6 +1260,14 @@ void Document::setVisualUpdatesAllowed(bool visualUpdatesAllowed)
     if (!visualUpdatesAllowed)
         return;
 
+    FrameView* frameView = view();
+    bool needsLayout = frameView && renderer() && (frameView->layoutPending() || renderer()->needsLayout());
+    if (needsLayout) {
+        // There might be a layout pending, so make sure we don't update the screen with bogus data.
+        // The layout will actually update the compositing layers and repaint if needed.
+        return;
+    }
+
 #if USE(ACCELERATED_COMPOSITING)
     if (view())
         view()->updateCompositingLayersAfterLayout();
index 2a5ae12..1a4a257 100644 (file)
@@ -408,6 +408,8 @@ void RenderLayerCompositor::updateCompositingLayers(CompositingUpdateType update
     if (!m_reevaluateCompositingAfterLayout && !m_compositing)
         return;
 
+    ASSERT(!m_renderView->needsLayout());
+
     AnimationUpdateBlock animationUpdateBlock(m_renderView->frameView()->frame()->animation());
 
     bool checkForHierarchyUpdate = m_reevaluateCompositingAfterLayout;
index d54c909..89b093f 100644 (file)
@@ -50,6 +50,7 @@
 #include "HTMLMarqueeElement.h"
 #include "HTMLNames.h"
 #include "RenderLayer.h"
+#include "RenderView.h"
 
 using namespace std;
 
@@ -254,7 +255,7 @@ void RenderMarquee::updateMarqueeStyle()
 
 void RenderMarquee::timerFired(Timer<RenderMarquee>*)
 {
-    if (m_layer->renderer()->needsLayout())
+    if (m_layer->renderer()->view()->needsLayout())
         return;
     
     if (m_reset) {