[BlackBerry] SurfacePool::waitForBuffer() sometimes waits for deleted…

… EGLSyncKHR object

https://bugs.webkit.org/show_bug.cgi?id=94208

Patch by Arvid Nilsson <anilsson@rim.com> on 2012-08-16
Reviewed by Rob Buis.

SurfacePool::notifyBuffersComposited() adds a tile's previous
sync object to the garbage list before replacing it with a new one.
However, it failed to thoroughly clear all tiles that were referencing
the old sync object.

Thus it could happen that if a set of tiles A was composited, then
another set of tiles B was composited, only the intersection of A and B
was cleared of the soon-to-be-deleted sync object, and the subtraction
A - B would reference an invalid sync object in case the BackingStore
decided to render to one of the tiles in A - B before they were
composited again.

Fixed by storing each individual sync object in only one place so we
don't have to rummage through all tiles and remove stale references to
sync objects that are about to be destroyed.

A new reference counted Fence class is added for this purpose, to store
a sync object. Tiles refer to Fence instances instead of holding a sync
object directly. Since Fence is reference counted, several tiles can
refer to the same Fence instance, and clearing its sync object will
instantly remove it from the grasp of all tiles that depend on that
Fence.

Since there's no point in waiting for the same Fence twice, the only
operation provided on Fence is takePlatformSync() which returns the
sync object and clears the Fence of its sync object.

Reviewed internally by Filip Spacek.

PR 193610

* WebKitSupport/BackingStoreTile.cpp:
(BlackBerry::WebKit::TileBuffer::TileBuffer):
* WebKitSupport/BackingStoreTile.h:
(BlackBerry):
(Fence):
(BlackBerry::Fence::create):
(BlackBerry::Fence::takePlatformSync):
(BlackBerry::Fence::Fence):
(BlackBerry::WebKit::TileBuffer::fence):
(BlackBerry::WebKit::TileBuffer::setFence):
(TileBuffer):
* WebKitSupport/SurfacePool.cpp:
(BlackBerry::WebKit::SurfacePool::waitForBuffer):
(BlackBerry::WebKit::SurfacePool::notifyBuffersComposited):
* WebKitSupport/SurfacePool.h:
(SurfacePool):

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

Source/WebKit/blackberry/ChangeLog

@@ -1,3 +1,58 @@
+2012-08-16  Arvid Nilsson  <anilsson@rim.com>
+
+        [BlackBerry] SurfacePool::waitForBuffer() sometimes waits for deleted EGLSyncKHR object
+        https://bugs.webkit.org/show_bug.cgi?id=94208
+
+        Reviewed by Rob Buis.
+
+        SurfacePool::notifyBuffersComposited() adds a tile's previous
+        sync object to the garbage list before replacing it with a new one.
+        However, it failed to thoroughly clear all tiles that were referencing
+        the old sync object.
+
+        Thus it could happen that if a set of tiles A was composited, then
+        another set of tiles B was composited, only the intersection of A and B
+        was cleared of the soon-to-be-deleted sync object, and the subtraction
+        A - B would reference an invalid sync object in case the BackingStore
+        decided to render to one of the tiles in A - B before they were
+        composited again.
+
+        Fixed by storing each individual sync object in only one place so we
+        don't have to rummage through all tiles and remove stale references to
+        sync objects that are about to be destroyed.
+
+        A new reference counted Fence class is added for this purpose, to store
+        a sync object. Tiles refer to Fence instances instead of holding a sync
+        object directly. Since Fence is reference counted, several tiles can
+        refer to the same Fence instance, and clearing its sync object will
+        instantly remove it from the grasp of all tiles that depend on that
+        Fence.
+
+        Since there's no point in waiting for the same Fence twice, the only
+        operation provided on Fence is takePlatformSync() which returns the
+        sync object and clears the Fence of its sync object.
+
+        Reviewed internally by Filip Spacek.
+
+        PR 193610
+
+        * WebKitSupport/BackingStoreTile.cpp:
+        (BlackBerry::WebKit::TileBuffer::TileBuffer):
+        * WebKitSupport/BackingStoreTile.h:
+        (BlackBerry):
+        (Fence):
+        (BlackBerry::Fence::create):
+        (BlackBerry::Fence::takePlatformSync):
+        (BlackBerry::Fence::Fence):
+        (BlackBerry::WebKit::TileBuffer::fence):
+        (BlackBerry::WebKit::TileBuffer::setFence):
+        (TileBuffer):
+        * WebKitSupport/SurfacePool.cpp:
+        (BlackBerry::WebKit::SurfacePool::waitForBuffer):
+        (BlackBerry::WebKit::SurfacePool::notifyBuffersComposited):
+        * WebKitSupport/SurfacePool.h:
+        (SurfacePool):
+
 2012-08-16  Rob Buis  <rbuis@rim.com>
 
         [BlackBerry] Suppress non DRT JS Console output.

Source/WebKit/blackberry/WebKitSupport/BackingStoreTile.cpp

@@ -29,7 +29,7 @@ namespace WebKit {
 
 TileBuffer::TileBuffer(const Platform::IntSize& size)
     : m_size(size)
-    , m_syncObject(0)
+    , m_fence(Fence::create())
     , m_buffer(0)
 {
 }

Source/WebKit/blackberry/WebKitSupport/BackingStoreTile.h

@@ -21,6 +21,9 @@
 
 #include "BlackBerryPlatformIntRectRegion.h"
 #include "BlackBerryPlatformPrimitives.h"
+#include <wtf/PassRefPtr.h>
+#include <wtf/RefCounted.h>
+#include <wtf/RefPtr.h>
 
 namespace BlackBerry {
 namespace Platform {
@@ -29,6 +32,37 @@ struct Buffer;
 }
 }
 
+// Represents a fence that has been inserted into the command stream. You can wait on the corresponding platform sync
+// object to make sure that previous commands have been completed.
+// There is no reason to wait twice on the same platform sync object, so the only mechanism provided to access the sync
+// object will also clear it to prevent anyone from waiting again.
+// Fence is only refcounted on the compositing thread, so RefCounted will suffice, we don't need ThreadSafeRefCounted.
+class Fence : public RefCounted<Fence> {
+public:
+    static PassRefPtr<Fence> create(void* platformSync = 0)
+    {
+        return adoptRef(new Fence(platformSync));
+    }
+
+    // Returns 0 if this fence is stale (someone already waited on it)
+    // Otherwise returns the platform sync object and clears the sync
+    // object so no-one waits again.
+    void* takePlatformSync()
+    {
+        void* tmp = m_platformSync;
+        m_platformSync = 0;
+        return tmp;
+    }
+
+private:
+    Fence(void* platformSync)
+        : m_platformSync(platformSync)
+    {
+    }
+
+    void* m_platformSync;
+};
+
 namespace WebKit {
 class TileBuffer {
     public:
@@ -47,13 +81,13 @@ class TileBuffer {
 
         Platform::Graphics::Buffer* nativeBuffer() const;
 
-        void* syncObject() const { return m_syncObject; }
-        void setSyncObject(void* syncObject) { m_syncObject = syncObject; }
+        Fence* fence() const { return m_fence.get(); }
+        void setFence(PassRefPtr<Fence> fence) { m_fence = fence; }
 
     private:
         Platform::IntSize m_size;
         Platform::IntRectRegion m_renderedRegion;
-        void* m_syncObject;
+        RefPtr<Fence> m_fence;
         mutable Platform::Graphics::Buffer* m_buffer;
 };
 

Source/WebKit/blackberry/WebKitSupport/SurfacePool.cpp

@@ -234,31 +234,24 @@ void SurfacePool::waitForBuffer(TileBuffer* tileBuffer)
         return;
 
 #if BLACKBERRY_PLATFORM_GRAPHICS_EGL
-    EGLSyncKHR syncObject;
+    EGLSyncKHR platformSync;
 
     {
         Platform::MutexLocker locker(&m_mutex);
-        syncObject = tileBuffer->syncObject();
-        if (!syncObject)
-            return;
-
-        // Stale references to this sync object may remain with other tiles, don't wait for it again.
-        for (unsigned int i = 0; i < m_tilePool.size(); ++i) {
-            TileBuffer* tileBuffer = m_tilePool[i]->frontBuffer();
-            if (tileBuffer->syncObject() == syncObject)
-                tileBuffer->setSyncObject(0);
-        }
-        if (backBuffer()->syncObject() == syncObject)
-            backBuffer()->setSyncObject(0);
+        platformSync = tileBuffer->fence()->takePlatformSync();
     }
 
-    eglClientWaitSyncKHR(Platform::Graphics::eglDisplay(), syncObject, 0, 100000000LL);
+    if (!platformSync)
+        return;
+
+    if (!eglClientWaitSyncKHR(Platform::Graphics::eglDisplay(), platformSync, 0, 100000000LL))
+        Platform::logAlways(Platform::LogLevelWarn, "Failed to wait for EGLSyncKHR object!\n");
 
     // Instead of assuming eglDestroySyncKHR is thread safe, we add it to
     // a garbage list for later collection on the thread that created it.
     {
         Platform::MutexLocker locker(&m_mutex);
-        m_syncObjectsToDestroy.insert(syncObject);
+        m_garbage.insert(platformSync);
     }
 #endif
 }
@@ -275,21 +268,23 @@ void SurfacePool::notifyBuffersComposited(const Vector<TileBuffer*>& tileBuffers
 
     // The EGL_KHR_fence_sync spec is nice enough to specify that the sync object
     // is not actually deleted until everyone has stopped using it.
-    for (std::set<void*>::const_iterator it = m_syncObjectsToDestroy.begin(); it != m_syncObjectsToDestroy.end(); ++it)
+    for (std::set<void*>::const_iterator it = m_garbage.begin(); it != m_garbage.end(); ++it)
         eglDestroySyncKHR(display, *it);
-    m_syncObjectsToDestroy.clear();
+    m_garbage.clear();
 
-    // If we didn't blit anything, we don't need to create a new sync object.
+    // If we didn't blit anything, we don't need to create a new fence.
     if (tileBuffers.isEmpty())
         return;
 
-    EGLSyncKHR syncObject = eglCreateSyncKHR(display, EGL_SYNC_FENCE_KHR, 0);
-
+    // Create a new fence and assign to the tiles that were blit. Invalidate any previous
+    // fence that may be active among these tiles and add its sync object to the garbage set
+    // for later destruction to make sure it doesn't leak.
+    RefPtr<Fence> fence = Fence::create(eglCreateSyncKHR(display, EGL_SYNC_FENCE_KHR, 0));
     for (unsigned int i = 0; i < tileBuffers.size(); ++i) {
-        if (EGLSyncKHR previousSyncObject = tileBuffers[i]->syncObject())
-            m_syncObjectsToDestroy.insert(previousSyncObject);
+        if (EGLSyncKHR platformSync = tileBuffers[i]->fence()->takePlatformSync())
+            m_garbage.insert(platformSync);
 
-        tileBuffers[i]->setSyncObject(syncObject);
+        tileBuffers[i]->setFence(fence);
     }
 #endif
 }

Source/WebKit/blackberry/WebKitSupport/SurfacePool.h

@@ -99,7 +99,7 @@ class SurfacePool {
     bool m_initialized; // SurfacePool has been set up, with or without buffers.
     bool m_buffersSuspended; // Buffer objects exist, but pixel memory has been freed.
 
-    std::set<void*> m_syncObjectsToDestroy;
+    std::set<void*> m_garbage;
     bool m_hasFenceExtension;
     mutable pthread_mutex_t m_mutex;
 };