[chromium] Fix race bug that clobbers CCLayerImpl updateRect
authorshawnsingh@chromium.org <shawnsingh@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Mar 2012 01:28:12 +0000 (01:28 +0000)
committershawnsingh@chromium.org <shawnsingh@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 24 Mar 2012 01:28:12 +0000 (01:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=82109

Reviewed by Dirk Pranke.

Source/WebCore:

If the main thread commits twice before the impl thread actually
draws, then the updateRect of the first frame gets lost forever,
and not propagated to the damage tracker.

The solution is to accumulate the updateRect. The CCLayerImpl
updateRect is already being correctly cleared at the appropriate
time after drawing.

Unit test added to LayerChromiumTest.cpp.

* platform/graphics/chromium/LayerChromium.cpp:
(WebCore::LayerChromium::pushPropertiesTo):

Source/WebKit/chromium:

* tests/LayerChromiumTest.cpp:

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/chromium/LayerChromium.cpp
Source/WebKit/chromium/ChangeLog
Source/WebKit/chromium/tests/LayerChromiumTest.cpp

index 7dca8c5d5570baf314fceddf804d71a8cea46dfc..4eb2c78de2453f1748eff5e22d0ea4d474c90a07 100644 (file)
@@ -1,3 +1,23 @@
+2012-03-23  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Fix race bug that clobbers CCLayerImpl updateRect
+        https://bugs.webkit.org/show_bug.cgi?id=82109
+
+        Reviewed by Dirk Pranke.
+
+        If the main thread commits twice before the impl thread actually
+        draws, then the updateRect of the first frame gets lost forever,
+        and not propagated to the damage tracker.
+
+        The solution is to accumulate the updateRect. The CCLayerImpl
+        updateRect is already being correctly cleared at the appropriate
+        time after drawing.
+
+        Unit test added to LayerChromiumTest.cpp.
+
+        * platform/graphics/chromium/LayerChromium.cpp:
+        (WebCore::LayerChromium::pushPropertiesTo):
+
 2012-03-23  Rafael Weinstein  <rafaelw@chromium.org>
 
         [MutationObservers] attributeFilter should be case sensitive at all times
index dcfb6e31fb08d6d1d693de4f4103da5670b83bf1..a5b36437af048ca42d28dcc017d7d7cce6c5a3ef 100644 (file)
@@ -480,6 +480,11 @@ void LayerChromium::pushPropertiesTo(CCLayerImpl* layer)
     layer->setSublayerTransform(m_sublayerTransform);
     if (!transformIsAnimating())
         layer->setTransform(m_transform);
+
+    // If the main thread commits multiple times before the impl thread actually draws, then damage tracking
+    // will become incorrect if we simply clobber the updateRect here. The CCLayerImpl's updateRect needs to
+    // accumulate (i.e. union) any update changes that have occurred on the main thread.
+    m_updateRect.uniteIfNonZero(layer->updateRect());
     layer->setUpdateRect(m_updateRect);
 
     layer->setScrollDelta(layer->scrollDelta() - layer->sentScrollDelta());
index 27e9043b8112849a5447835afd6162448b147857..0c8e6ec56958c88c8d9099d1e463cac970a438d2 100644 (file)
@@ -1,3 +1,12 @@
+2012-03-23  Shawn Singh  <shawnsingh@chromium.org>
+
+        [chromium] Fix race bug that clobbers CCLayerImpl updateRect
+        https://bugs.webkit.org/show_bug.cgi?id=82109
+
+        Reviewed by Dirk Pranke.
+
+        * tests/LayerChromiumTest.cpp:
+
 2012-03-23  Daniel Cheng  <dcheng@chromium.org>
 
         [chromium] Support file drag out using DataTransferItemList::add(File)
index d8858cc12361f7c30d9d26f15a75a4695d1f1379..e7de356dd0da38a31b896b2d7a9c48a9b1a36c23 100644 (file)
 
 #include "LayerChromium.h"
 
-#include "cc/CCLayerTreeHost.h"
 #include "CCLayerTreeTestCommon.h"
 #include "FakeCCLayerTreeHostClient.h"
 #include "LayerPainterChromium.h"
 #include "NonCompositedContentHost.h"
 #include "WebCompositor.h"
+#include "cc/CCLayerImpl.h"
 #include "cc/CCLayerTreeHost.h"
+#include "cc/CCSingleThreadProxy.h"
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
@@ -530,6 +531,29 @@ TEST_F(LayerChromiumTest, checkPropertyChangeCausesCorrectBehavior)
     EXPECT_TRUE(testLayer->needsDisplay());
 }
 
+TEST_F(LayerChromiumTest, verifyPushPropertiesAccumulatesUpdateRect)
+{
+    DebugScopedSetImplThread setImplThread;
+
+    RefPtr<LayerChromium> testLayer = LayerChromium::create();
+    OwnPtr<CCLayerImpl> implLayer = CCLayerImpl::create(1);
+
+    testLayer->setNeedsDisplayRect(FloatRect(FloatPoint::zero(), FloatSize(5, 5)));
+    testLayer->pushPropertiesTo(implLayer.get());
+    EXPECT_FLOAT_RECT_EQ(FloatRect(FloatPoint::zero(), FloatSize(5, 5)), implLayer->updateRect());
+
+    // The CCLayerImpl's updateRect should be accumulated here, since we did not do anything to clear it.
+    testLayer->setNeedsDisplayRect(FloatRect(FloatPoint(10, 10), FloatSize(5, 5)));
+    testLayer->pushPropertiesTo(implLayer.get());
+    EXPECT_FLOAT_RECT_EQ(FloatRect(FloatPoint::zero(), FloatSize(15, 15)), implLayer->updateRect());
+
+    // If we do clear the CCLayerImpl side, then the next updateRect should be fresh without accumulation.
+    implLayer->resetAllChangeTrackingForSubtree();
+    testLayer->setNeedsDisplayRect(FloatRect(FloatPoint(10, 10), FloatSize(5, 5)));
+    testLayer->pushPropertiesTo(implLayer.get());
+    EXPECT_FLOAT_RECT_EQ(FloatRect(FloatPoint(10, 10), FloatSize(5, 5)), implLayer->updateRect());
+}
+
 class LayerChromiumWithContentScaling : public LayerChromium {
 public:
     explicit LayerChromiumWithContentScaling()