Change Canvas2DLayerBridge to stay alive until last mailbox is returned.

Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp

Index: Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
diff --git a/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp b/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
index 415175a95465266f875e6a43df3cd182bebda1cc..34af5311abd72ba1090012ac5b746c568dc58333 100644
--- a/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
+++ b/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
@@ -59,14 +59,14 @@ static SkSurface* createSurface(GraphicsContext3D* context3D, const IntSize& siz
     return SkSurface::NewRenderTarget(gr, info);
 }
 
-PassOwnPtr<Canvas2DLayerBridge> Canvas2DLayerBridge::create(PassRefPtr<GraphicsContext3D> context, const IntSize& size, OpacityMode opacityMode)
+PassRefPtr<Canvas2DLayerBridge> Canvas2DLayerBridge::create(PassRefPtr<GraphicsContext3D> context, const IntSize& size, OpacityMode opacityMode)
 {
     TRACE_EVENT_INSTANT0("test_gpu", "Canvas2DLayerBridgeCreation");
     SkAutoTUnref<SkSurface> surface(createSurface(context.get(), size));
     if (!surface.get())
-        return PassOwnPtr<Canvas2DLayerBridge>();
+        return PassRefPtr<Canvas2DLayerBridge>();
     SkDeferredCanvas* canvas = SkDeferredCanvas::Create(surface.get());
-    OwnPtr<Canvas2DLayerBridge> layerBridge = adoptPtr(new Canvas2DLayerBridge(context, canvas, opacityMode));
+    RefPtr<Canvas2DLayerBridge> layerBridge = adoptRef(new Canvas2DLayerBridge(context, canvas, opacityMode));
     return layerBridge.release();
 }
 
@@ -77,6 +77,8 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(PassRefPtr<GraphicsContext3D> context,
     , m_didRecordDrawCommand(false)
     , m_surfaceIsValid(true)
     , m_framesPending(0)
+    , m_liveMailboxCount(0)
+    , m_destructionInProgress(false)
     , m_rateLimitingEnabled(false)
     , m_next(0)
     , m_prev(0)
@@ -95,16 +97,39 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(PassRefPtr<GraphicsContext3D> context,
 
 Canvas2DLayerBridge::~Canvas2DLayerBridge()
 {
-    GraphicsLayer::unregisterContentsLayer(m_layer->layer());
-    Canvas2DLayerManager::get().layerToBeDestroyed(this);
-    m_canvas->setNotificationClient(0);
-    m_mailboxes.clear();
-    m_layer->clearTexture();
+    ASSERT(m_destructionInProgress);
     m_layer.clear();
+    Vector<MailboxInfo>::iterator mailboxInfo;
+    for (mailboxInfo = m_mailboxes.begin(); mailboxInfo < m_mailboxes.end(); mailboxInfo++) {
+        ASSERT(mailboxInfo->m_status != MailboxInUse);
+        if (mailboxInfo->m_status == MailboxReleased) {
+            if (mailboxInfo->m_mailbox.syncPoint) {
+                context()->waitSyncPoint(mailboxInfo->m_mailbox.syncPoint);
+                mailboxInfo->m_mailbox.syncPoint = 0;
+            }
+            // Invalidate texture state in case the compositor altered it since the copy-on-write.
+            mailboxInfo->m_image->getTexture()->invalidateCachedState();
+        }
+    }
+    m_mailboxes.clear();
+}
+
+void Canvas2DLayerBridge::deref()
+{
+    if (this->refCount() == m_liveMailboxCount + 1 && !m_destructionInProgress) {

[piman, 2013/08/20 22:09:00]

I don't know if this is kosher in Webkit land to play with the refcount - it
usually makes things very fragile if something else takes a reference. What I
was thinking of was keeping an explicit "destroy" (or whatever) that sets
m_destructionInProgress and removes from the tree, and refcount is just needed
to keep the MailboxInfos alive while we're waiting for them to be returned.

[Stephen White, 2013/08/21 17:13:29]

I agree that this is an unfortunate pattern. Of the two options I think
destroy() is less fragile than messing with the refcount, although still not
ideal (since you have to remember to call it). Another option would be to split
the LayerBridge in two, so that you can distinguish between the refcounts on one
half versus the other.

+        // removing last external reference -> initiate destruction
+        m_destructionInProgress = true;
+        GraphicsLayer::unregisterContentsLayer(m_layer->layer());
+        m_canvas->setNotificationClient(0);
+        m_layer->clearTexture();
+        Canvas2DLayerManager::get().layerToBeDestroyed(this);
+    }
+    RefCounted<Canvas2DLayerBridge>::deref();
 }
 
 void Canvas2DLayerBridge::limitPendingFrames()
 {
+    ASSERT(!m_destructionInProgress);
     if (m_didRecordDrawCommand) {
         m_framesPending++;
         m_didRecordDrawCommand = false;
@@ -121,6 +146,7 @@ void Canvas2DLayerBridge::limitPendingFrames()
 
 void Canvas2DLayerBridge::prepareForDraw()
 {
+    ASSERT(!m_destructionInProgress);
     ASSERT(m_layer);
     if (!isValid()) {
         if (m_canvas) {
@@ -134,6 +160,7 @@ void Canvas2DLayerBridge::prepareForDraw()
 
 void Canvas2DLayerBridge::storageAllocatedForRecordingChanged(size_t bytesAllocated)
 {
+    ASSERT(!m_destructionInProgress);
     intptr_t delta = (intptr_t)bytesAllocated - (intptr_t)m_bytesAllocated;
     m_bytesAllocated = bytesAllocated;
     Canvas2DLayerManager::get().layerAllocatedStorageChanged(this, delta);
@@ -141,17 +168,20 @@ void Canvas2DLayerBridge::storageAllocatedForRecordingChanged(size_t bytesAlloca
 
 size_t Canvas2DLayerBridge::storageAllocatedForRecording()
 {
+    ASSERT(!m_destructionInProgress);
     return m_canvas->storageAllocatedForRecording();
 }
 
 void Canvas2DLayerBridge::flushedDrawCommands()
 {
+    ASSERT(!m_destructionInProgress);
     storageAllocatedForRecordingChanged(storageAllocatedForRecording());
     m_framesPending = 0;
 }
 
 void Canvas2DLayerBridge::skippedPendingDrawCommands()
 {
+    ASSERT(!m_destructionInProgress);
     // Stop triggering the rate limiter if SkDeferredCanvas is detecting
     // and optimizing overdraw.
     setRateLimitingEnabled(false);
@@ -160,6 +190,7 @@ void Canvas2DLayerBridge::skippedPendingDrawCommands()
 
 void Canvas2DLayerBridge::setRateLimitingEnabled(bool enabled)
 {
+    ASSERT(!m_destructionInProgress || !enabled);
     if (m_rateLimitingEnabled != enabled) {
         m_rateLimitingEnabled = enabled;
         m_layer->setRateLimitContext(m_rateLimitingEnabled);
@@ -168,6 +199,7 @@ void Canvas2DLayerBridge::setRateLimitingEnabled(bool enabled)
 
 size_t Canvas2DLayerBridge::freeMemoryIfPossible(size_t bytesToFree)
 {
+    ASSERT(!m_destructionInProgress);
     size_t bytesFreed = m_canvas->freeMemoryIfPossible(bytesToFree);
     if (bytesFreed)
         Canvas2DLayerManager::get().layerAllocatedStorageChanged(this, -((intptr_t)bytesFreed));
@@ -177,6 +209,7 @@ size_t Canvas2DLayerBridge::freeMemoryIfPossible(size_t bytesToFree)
 
 void Canvas2DLayerBridge::flush()
 {
+    ASSERT(!m_destructionInProgress);
     if (m_canvas->hasPendingCommands()) {
         TRACE_EVENT0("cc", "Canvas2DLayerBridge::flush");
         m_canvas->flush();
@@ -196,6 +229,8 @@ WebGraphicsContext3D* Canvas2DLayerBridge::context()
 bool Canvas2DLayerBridge::isValid()
 {
     ASSERT(m_layer);
+    if (m_destructionInProgress)
+        return false;
     if (m_context->webContext()->isContextLost() || !m_surfaceIsValid) {
         // Attempt to recover.
         m_layer->clearTexture();
@@ -221,11 +256,15 @@ bool Canvas2DLayerBridge::isValid()
     if (!m_surfaceIsValid)
         setRateLimitingEnabled(false);
     return m_surfaceIsValid;
-
 }
 
 bool Canvas2DLayerBridge::prepareMailbox(WebKit::WebExternalTextureMailbox* outMailbox, WebKit::WebExternalBitmap* bitmap)
 {
+    if (m_destructionInProgress) {
+        const WebKit::WebExternalTextureMailbox emptyMailbox;
+        *outMailbox = emptyMailbox;
+        return false;

[piman, 2013/08/20 22:09:00]

we called clearTexture() when setting m_destructionInProgress, so returning
false means we're not setting a new texture. returning true would mean "yes we
changed the mailbox, but hey, it's still null"

+    }
     ASSERT(!bitmap);
     if (!isValid())
         return false;
@@ -278,11 +317,18 @@ bool Canvas2DLayerBridge::prepareMailbox(WebKit::WebExternalTextureMailbox* outM
     // we must dirty the context.
     m_context->grContext()->resetContext(kTextureBinding_GrGLBackendState);
 
+    // set m_parentLayerBridge to make sure 'this' stays alive as long as it has
+    // live mailboxes
+    ASSERT(!mailboxInfo->m_parentLayerBridge);
+    this->ref(); // Because we are adopting an already referenced object
+    mailboxInfo->m_parentLayerBridge = adoptRef(this);
+    m_liveMailboxCount++;
     *outMailbox = mailboxInfo->m_mailbox;
     return true;
 }
 
 Canvas2DLayerBridge::MailboxInfo* Canvas2DLayerBridge::createMailboxInfo() {
+    ASSERT(!m_destructionInProgress);
     MailboxInfo* mailboxInfo;
     for (mailboxInfo = m_mailboxes.begin(); mailboxInfo < m_mailboxes.end(); mailboxInfo++) {
         if (mailboxInfo->m_status == MailboxAvailable) {
@@ -313,6 +359,12 @@ void Canvas2DLayerBridge::mailboxReleased(const WebKit::WebExternalTextureMailbo
             mailboxInfo->m_mailbox.syncPoint = mailbox.syncPoint;
             ASSERT(mailboxInfo->m_status == MailboxInUse);
             mailboxInfo->m_status = MailboxReleased;
+            // Trigger Canvas2DLayerBridge self-destruction if this is the
+            // last live mailbox and the layer bridge is not externally
+            // referenced.
+            ASSERT(mailboxInfo->m_parentLayerBridge.get() == this);
+            m_liveMailboxCount--;
+            mailboxInfo->m_parentLayerBridge.clear();
             return;
         }
     }
@@ -326,12 +378,14 @@ WebKit::WebLayer* Canvas2DLayerBridge::layer()
 
 void Canvas2DLayerBridge::contextAcquired()
 {
+    ASSERT(!m_destructionInProgress);
     Canvas2DLayerManager::get().layerDidDraw(this);
     m_didRecordDrawCommand = true;
 }
 
 unsigned Canvas2DLayerBridge::backBufferTexture()
 {
+    ASSERT(!m_destructionInProgress);
     if (!isValid())
         return 0;
     contextAcquired();