[CoordGraphics] CoordinatedGraphicsLayer::updateContentBuffers() should always assume...
authorzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Nov 2017 13:04:45 +0000 (13:04 +0000)
committerzandobersek@gmail.com <zandobersek@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Nov 2017 13:04:45 +0000 (13:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=179977

Reviewed by Carlos Garcia Campos.

Source/WebCore:

CoordinatedGraphicsLayer::updateContentBuffers() retrieves a RefPtr<CoordinatedBuffer>
from the CompositingCoordinator. This pointer should never be null since if no
existing UpdateAtlas can provide the necessary memory area, a fresh UpdateAtlas is
created and its buffer returned. This can't fail in theory since the tiles that are
being updated are smaller than the UpdateAtlas area.

The CoordinatedGraphicsLayerClient::getCoordinatedBuffer() method is updated to
always return a Ref<CoordinatedBuffer> value. Code in updateContentBuffers() is
updated to remove a null-check on what is now the Ref<CoordinatedBuffer> value.

No new tests -- no change in functionality.

* platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:
(WebCore::CoordinatedGraphicsLayer::updateContentBuffers):
* platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:

Source/WebKit:

Make CompositingCoordinator::getCoordinatedBuffer() return a Ref<CoordinatedBuffer>
value. In case an UpdateAtlas with enough free area is found, its CoordinatedBuffer
is dereferenced into the return value. In case a new UpdateAtlas is created, the
returned CoordinatedBuffer pointer is asserted to be non-null and dereferenced.

The retrieved CoordinatedBuffer pointer on a newly-created UpdateAtlas should never
be null since the tiles are smaller in size than the UpdateAtlas area. The assert
is done in release configurations as well since the code in CoordinatedGraphicsLayer
assumes the returned pointer will be non-null, so it's just a matter of where to
crash first in case somehow a null value is returned.

* WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:
(WebKit::CompositingCoordinator::getCoordinatedBuffer):
* WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h:

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp
Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h

index e48fe1f..1feeeca 100644 (file)
@@ -1,3 +1,26 @@
+2017-11-24  Zan Dobersek  <zdobersek@igalia.com>
+
+        [CoordGraphics] CoordinatedGraphicsLayer::updateContentBuffers() should always assume a non-null CoordinatedBuffer
+        https://bugs.webkit.org/show_bug.cgi?id=179977
+
+        Reviewed by Carlos Garcia Campos.
+
+        CoordinatedGraphicsLayer::updateContentBuffers() retrieves a RefPtr<CoordinatedBuffer>
+        from the CompositingCoordinator. This pointer should never be null since if no
+        existing UpdateAtlas can provide the necessary memory area, a fresh UpdateAtlas is
+        created and its buffer returned. This can't fail in theory since the tiles that are
+        being updated are smaller than the UpdateAtlas area.
+
+        The CoordinatedGraphicsLayerClient::getCoordinatedBuffer() method is updated to
+        always return a Ref<CoordinatedBuffer> value. Code in updateContentBuffers() is
+        updated to remove a null-check on what is now the Ref<CoordinatedBuffer> value.
+
+        No new tests -- no change in functionality.
+
+        * platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:
+        (WebCore::CoordinatedGraphicsLayer::updateContentBuffers):
+        * platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:
+
 2017-11-23  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Perform accelerated animations when possible
index ad25074..5afdfa2 100644 (file)
@@ -972,9 +972,6 @@ void CoordinatedGraphicsLayer::updateContentBuffers()
             auto coordinatedBuffer = m_coordinator->getCoordinatedBuffer(dirtyRect.size(),
                 contentsOpaque() ? CoordinatedBuffer::NoFlags : CoordinatedBuffer::SupportsAlpha,
                 updateInfo.atlasID, targetRect);
-            if (!coordinatedBuffer)
-                continue;
-
             {
                 GraphicsContext& context = coordinatedBuffer->context();
                 context.save();
index 224f051..23d5e32 100644 (file)
@@ -48,7 +48,7 @@ public:
     virtual FloatRect visibleContentsRect() const = 0;
     virtual Ref<CoordinatedImageBacking> createImageBackingIfNeeded(Image&) = 0;
     virtual void detachLayer(CoordinatedGraphicsLayer*) = 0;
-    virtual RefPtr<CoordinatedBuffer> getCoordinatedBuffer(const IntSize&, CoordinatedBuffer::Flags, uint32_t&, IntRect&) = 0;
+    virtual Ref<CoordinatedBuffer> getCoordinatedBuffer(const IntSize&, CoordinatedBuffer::Flags, uint32_t&, IntRect&) = 0;
 
     virtual void syncLayerState(CoordinatedLayerID, CoordinatedGraphicsLayerState&) = 0;
 };
index 92a9e18..581ea8b 100644 (file)
@@ -1,5 +1,27 @@
 2017-11-24  Zan Dobersek  <zdobersek@igalia.com>
 
+        [CoordGraphics] CoordinatedGraphicsLayer::updateContentBuffers() should always assume a non-null CoordinatedBuffer
+        https://bugs.webkit.org/show_bug.cgi?id=179977
+
+        Reviewed by Carlos Garcia Campos.
+
+        Make CompositingCoordinator::getCoordinatedBuffer() return a Ref<CoordinatedBuffer>
+        value. In case an UpdateAtlas with enough free area is found, its CoordinatedBuffer
+        is dereferenced into the return value. In case a new UpdateAtlas is created, the
+        returned CoordinatedBuffer pointer is asserted to be non-null and dereferenced.
+
+        The retrieved CoordinatedBuffer pointer on a newly-created UpdateAtlas should never
+        be null since the tiles are smaller in size than the UpdateAtlas area. The assert
+        is done in release configurations as well since the code in CoordinatedGraphicsLayer
+        assumes the returned pointer will be non-null, so it's just a matter of where to
+        crash first in case somehow a null value is returned.
+
+        * WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.cpp:
+        (WebKit::CompositingCoordinator::getCoordinatedBuffer):
+        * WebProcess/WebPage/CoordinatedGraphics/CompositingCoordinator.h:
+
+2017-11-24  Zan Dobersek  <zdobersek@igalia.com>
+
         [CoordGraphics] UpdateAtlas constructor should receive an IntSize, not a dimension value
         https://bugs.webkit.org/show_bug.cgi?id=179976
 
index 9ef8582..c0d385a 100644 (file)
@@ -385,19 +385,25 @@ void CompositingCoordinator::purgeBackingStores()
     m_updateAtlases.clear();
 }
 
-RefPtr<CoordinatedBuffer> CompositingCoordinator::getCoordinatedBuffer(const IntSize& size, CoordinatedBuffer::Flags flags, uint32_t& atlasID, IntRect& allocatedRect)
+Ref<CoordinatedBuffer> CompositingCoordinator::getCoordinatedBuffer(const IntSize& size, CoordinatedBuffer::Flags flags, uint32_t& atlasID, IntRect& allocatedRect)
 {
     for (auto& atlas : m_updateAtlases) {
         if (atlas->supportsAlpha() == (flags & CoordinatedBuffer::SupportsAlpha)) {
             if (auto buffer = atlas->getCoordinatedBuffer(size, atlasID, allocatedRect))
-                return buffer;
+                return *buffer;
         }
     }
 
     static const IntSize s_atlasSize { 1024, 1024 }; // This should be a square.
     m_updateAtlases.append(std::make_unique<UpdateAtlas>(*this, s_atlasSize, flags));
     scheduleReleaseInactiveAtlases();
-    return m_updateAtlases.last()->getCoordinatedBuffer(size, atlasID, allocatedRect);
+
+    // Specified size should always fit into a newly-created UpdateAtlas and a non-null
+    // CoordinatedBuffer value should be returned from UpdateAtlas::getCoordinatedBuffer().
+    // We use a RELEASE_ASSERT() to stop any malfunctioning at the earliest point.
+    auto buffer = m_updateAtlases.last()->getCoordinatedBuffer(size, atlasID, allocatedRect);
+    RELEASE_ASSERT(buffer);
+    return *buffer;
 }
 
 const Seconds releaseInactiveAtlasesTimerInterval { 500_ms };
index f0ec26c..b28bc3a 100644 (file)
@@ -117,7 +117,7 @@ private:
     WebCore::FloatRect visibleContentsRect() const override;
     Ref<WebCore::CoordinatedImageBacking> createImageBackingIfNeeded(WebCore::Image&) override;
     void detachLayer(WebCore::CoordinatedGraphicsLayer*) override;
-    RefPtr<WebCore::CoordinatedBuffer> getCoordinatedBuffer(const WebCore::IntSize&, WebCore::CoordinatedBuffer::Flags, uint32_t&, WebCore::IntRect&) override;
+    Ref<WebCore::CoordinatedBuffer> getCoordinatedBuffer(const WebCore::IntSize&, WebCore::CoordinatedBuffer::Flags, uint32_t&, WebCore::IntRect&) override;
     void syncLayerState(WebCore::CoordinatedLayerID, WebCore::CoordinatedGraphicsLayerState&) override;
 
     // UpdateAtlas::Client