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..c6643848f4cad3ee4f5c8be1dc314dacd92ada87 100644
--- a/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
+++ b/Source/core/platform/graphics/chromium/Canvas2DLayerBridge.cpp
@@ -59,15 +59,17 @@ 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)
+Canvas2DLayerBridgePtr 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>();
+    if (!surface.get()) {
+        Canvas2DLayerBridgePtr ptr;
+        return ptr;
+    }
     SkDeferredCanvas* canvas = SkDeferredCanvas::Create(surface.get());
-    OwnPtr<Canvas2DLayerBridge> layerBridge = adoptPtr(new Canvas2DLayerBridge(context, canvas, opacityMode));
-    return layerBridge.release();
+    Canvas2DLayerBridgePtr layerBridge(new Canvas2DLayerBridge(context, canvas, opacityMode));
+    return layerBridge;
 }
 
 Canvas2DLayerBridge::Canvas2DLayerBridge(PassRefPtr<GraphicsContext3D> context, SkDeferredCanvas* canvas, OpacityMode opacityMode)
@@ -78,6 +80,7 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(PassRefPtr<GraphicsContext3D> context,
     , m_surfaceIsValid(true)
     , m_framesPending(0)
     , m_rateLimitingEnabled(false)
+    , m_destructionInProgress(false)
     , m_next(0)
     , m_prev(0)
     , m_lastImageId(0)
@@ -93,18 +96,54 @@ Canvas2DLayerBridge::Canvas2DLayerBridge(PassRefPtr<GraphicsContext3D> context,
     m_canvas->setNotificationClient(this);
 }
 
-Canvas2DLayerBridge::~Canvas2DLayerBridge()
+void Canvas2DLayerBridge::destroy()
 {
+    ASSERT(!m_destructionInProgress);
     GraphicsLayer::unregisterContentsLayer(m_layer->layer());
-    Canvas2DLayerManager::get().layerToBeDestroyed(this);
     m_canvas->setNotificationClient(0);
-    m_mailboxes.clear();
     m_layer->clearTexture();
+    // Orphan the layer so the compositor will stop requesting new texture mailboxes.
+    m_layer->layer()->removeFromParent();

[Justin Novosad, 2013/08/20 15:51:10]

This line appears to be necessary if we want the mailboxes to be released
without having to produce new ones. Open issue: are texture layers always leaf
nodes in the compositor tree? If not, perhaps we need to worry about re
attaching the children of this layer to the parent layer?

[danakj, 2013/08/20 16:57:08]

You could also set the texture mailbox on the layer to be empty, then that would
be passed to the compositor thread and the old one would be released.

[piman, 2013/08/20 17:03:07]

I'll let the blink experts confirm, but the fact that you
GraphicsLayer::unregisterContentsLayer it either way means you're safe, I think.

+    Canvas2DLayerManager::get().layerToBeDestroyed(this);
+    m_destructionInProgress = true;
+    deleteIfPossible();
+}
+
+void Canvas2DLayerBridge::deleteIfPossible()
+{
+    // If we can't delete now, destruction is deferred until mailboxReleased
+    // is called on all remaining active mailboxes.
+    ASSERT(m_destructionInProgress);
+    bool deleteIsPossible = true;
+    Vector<MailboxInfo>::iterator mailboxInfo;
+    for (mailboxInfo = m_mailboxes.begin(); mailboxInfo < m_mailboxes.end(); mailboxInfo++) {
+        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();
+            mailboxInfo->m_image.reset(0);
+            mailboxInfo->m_status = MailboxAvailable;
+        } else if (mailboxInfo->m_status == MailboxInUse) {
+            deleteIsPossible = false;
+        }
+    }
+    if (deleteIsPossible)
+        delete this;

[piman, 2013/08/20 17:03:07]

Could it make sense to make Canvas2DLayerBridge refcounted, and have each of the
MailboxInfo keep a ref while it's in the MailboxInUse state?
You could move the waitSyncPoint / m_image reset logic to the destructor and
that way you don't need to add deleteIfPossible logic in the various places.

[Stephen White, 2013/08/20 17:32:18]

+1000 to this. This poor-man's refcounting here looks confusing and difficult to
maintain.

+}
+
+Canvas2DLayerBridge::~Canvas2DLayerBridge()
+{
+    ASSERT(m_destructionInProgress);
+    m_mailboxes.clear();
     m_layer.clear();
 }
 
 void Canvas2DLayerBridge::limitPendingFrames()
 {
+    ASSERT(!m_destructionInProgress);
     if (m_didRecordDrawCommand) {
         m_framesPending++;
         m_didRecordDrawCommand = false;
@@ -121,6 +160,7 @@ void Canvas2DLayerBridge::limitPendingFrames()
 
 void Canvas2DLayerBridge::prepareForDraw()
 {
+    ASSERT(!m_destructionInProgress);
     ASSERT(m_layer);
     if (!isValid()) {
         if (m_canvas) {
@@ -134,6 +174,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 +182,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 +204,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 +213,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 +223,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();
@@ -187,7 +234,7 @@ WebGraphicsContext3D* Canvas2DLayerBridge::context()
 {
     // Check on m_layer is necessary because context() may be called during
     // the destruction of m_layer
-    if (m_layer) {
+    if (m_layer && !m_destructionInProgress) {
         isValid(); // To ensure rate limiter is disabled if context is lost.
     }
     return m_context->webContext();
@@ -195,6 +242,7 @@ WebGraphicsContext3D* Canvas2DLayerBridge::context()
 
 bool Canvas2DLayerBridge::isValid()
 {
+    ASSERT(!m_destructionInProgress);
     ASSERT(m_layer);
     if (m_context->webContext()->isContextLost() || !m_surfaceIsValid) {
         // Attempt to recover.
@@ -221,11 +269,11 @@ bool Canvas2DLayerBridge::isValid()
     if (!m_surfaceIsValid)
         setRateLimitingEnabled(false);
     return m_surfaceIsValid;
-
 }
 
 bool Canvas2DLayerBridge::prepareMailbox(WebKit::WebExternalTextureMailbox* outMailbox, WebKit::WebExternalBitmap* bitmap)
 {
+    ASSERT(!m_destructionInProgress)
     ASSERT(!bitmap);
     if (!isValid())
         return false;
@@ -283,6 +331,7 @@ bool Canvas2DLayerBridge::prepareMailbox(WebKit::WebExternalTextureMailbox* outM
 }
 
 Canvas2DLayerBridge::MailboxInfo* Canvas2DLayerBridge::createMailboxInfo() {
+    ASSERT(!m_destructionInProgress);
     MailboxInfo* mailboxInfo;
     for (mailboxInfo = m_mailboxes.begin(); mailboxInfo < m_mailboxes.end(); mailboxInfo++) {
         if (mailboxInfo->m_status == MailboxAvailable) {
@@ -316,22 +365,27 @@ void Canvas2DLayerBridge::mailboxReleased(const WebKit::WebExternalTextureMailbo
             return;
         }
     }
+    if (m_destructionInProgress)
+        deleteIfPossible();
 }
 
 WebKit::WebLayer* Canvas2DLayerBridge::layer()
 {
+    ASSERT(!m_destructionInProgress);
     ASSERT(m_layer);
     return m_layer->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();