WebGL: Don't destroy mailbox textures in the destructor until they're released.

Source/platform/graphics/gpu/DrawingBuffer.cpp

Index: Source/platform/graphics/gpu/DrawingBuffer.cpp
diff --git a/Source/platform/graphics/gpu/DrawingBuffer.cpp b/Source/platform/graphics/gpu/DrawingBuffer.cpp
index 607a68216facf6a5eb479a95ecc0656f587caf7c..b1528e89266ac121a9bb37cf9b899d97c6374a34 100644
--- a/Source/platform/graphics/gpu/DrawingBuffer.cpp
+++ b/Source/platform/graphics/gpu/DrawingBuffer.cpp
@@ -42,20 +42,26 @@
 #include "public/platform/WebExternalTextureLayer.h"
 #include "public/platform/WebGraphicsContext3D.h"
 #include "public/platform/WebGraphicsContext3DProvider.h"
+#ifndef NDEBUG
+#include "wtf/RefCountedLeakCounter.h"
+#endif
 
 using namespace std;
 
 namespace WebCore {
 
+namespace {
 // Global resource ceiling (expressed in terms of pixels) for DrawingBuffer creation and resize.
 // When this limit is set, DrawingBuffer::create() and DrawingBuffer::reset() calls that would
 // exceed the global cap will instead clear the buffer.
-static const int s_maximumResourceUsePixels = 16 * 1024 * 1024;
-static int s_currentResourceUsePixels = 0;
-static const float s_resourceAdjustedRatio = 0.5;
+const int s_maximumResourceUsePixels = 16 * 1024 * 1024;
+int s_currentResourceUsePixels = 0;
+const float s_resourceAdjustedRatio = 0.5;
+
+const bool s_allowContextEvictionOnCreate = true;
+const int s_maxScaleAttempts = 3;
 
-static const bool s_allowContextEvictionOnCreate = true;
-static const int s_maxScaleAttempts = 3;
+DEFINE_DEBUG_ONLY_GLOBAL(WTF::RefCountedLeakCounter, drawingBufferCounter, ("DrawingBuffer"));
 
 class ScopedTextureUnit0BindingRestorer {
 public:
@@ -77,6 +83,7 @@ private:
     GLenum m_oldActiveTextureUnit;
     Platform3DObject m_oldTextureUnitZeroId;
 };
+} // namespace
 
 PassRefPtr<DrawingBuffer> DrawingBuffer::create(PassOwnPtr<blink::WebGraphicsContext3D> context, const IntSize& size, PreserveDrawingBuffer preserve, PassRefPtr<ContextEvictionManager> contextEvictionManager)
 {
@@ -93,8 +100,10 @@ PassRefPtr<DrawingBuffer> DrawingBuffer::create(PassOwnPtr<blink::WebGraphicsCon
         extensionsUtil.ensureExtensionEnabled("GL_OES_packed_depth_stencil");
 
     RefPtr<DrawingBuffer> drawingBuffer = adoptRef(new DrawingBuffer(context, multisampleSupported, packedDepthStencilSupported, preserve, contextEvictionManager));
-    if (!drawingBuffer->initialize(size))
+    if (!drawingBuffer->initialize(size)) {
+        drawingBuffer->beginDestruction();
         return PassRefPtr<DrawingBuffer>();
+    }
     return drawingBuffer.release();
 }
 
@@ -130,15 +139,25 @@ DrawingBuffer::DrawingBuffer(PassOwnPtr<blink::WebGraphicsContext3D> context,
     , m_maxTextureSize(0)
     , m_sampleCount(0)
     , m_packAlignment(4)
+    , m_destructionInProgress(false)
     , m_contextEvictionManager(contextEvictionManager)
 {
     // Used by browser tests to detect the use of a DrawingBuffer.
     TRACE_EVENT_INSTANT0("test_gpu", "DrawingBufferCreation");
+#ifndef NDEBUG
+    drawingBufferCounter.increment();
+#endif
 }
 
 DrawingBuffer::~DrawingBuffer()
 {
-    releaseResources();
+    ASSERT(m_destructionInProgress);
+    ASSERT(m_textureMailboxes.isEmpty());
+    m_context.clear();
+    m_layer.clear();
+#ifndef NDEBUG
+    drawingBufferCounter.decrement();
+#endif
 }
 
 void DrawingBuffer::markContentsChanged()
@@ -165,6 +184,9 @@ blink::WebGraphicsContext3D* DrawingBuffer::context()
 
 bool DrawingBuffer::prepareMailbox(blink::WebExternalTextureMailbox* outMailbox, blink::WebExternalBitmap* bitmap)
 {
+    // prepareMailbox() is always called after layout.
+    ASSERT(!m_destructionInProgress);
+
     if (!m_contentsChanged)
         return false;
 
@@ -229,6 +251,9 @@ bool DrawingBuffer::prepareMailbox(blink::WebExternalTextureMailbox* outMailbox,
     frontColorBufferMailbox->mailbox.syncPoint = m_context->insertSyncPoint();
     markLayerComposited();
 
+    // set m_parentDrawingBuffer to make sure 'this' stays alive as long as it has live mailboxes
+    ASSERT(!frontColorBufferMailbox->m_parentDrawingBuffer);
+    frontColorBufferMailbox->m_parentDrawingBuffer = this;
     *outMailbox = frontColorBufferMailbox->mailbox;
     m_frontColorBuffer = frontColorBufferMailbox->textureId;
     return true;
@@ -236,10 +261,17 @@ bool DrawingBuffer::prepareMailbox(blink::WebExternalTextureMailbox* outMailbox,
 
 void DrawingBuffer::mailboxReleased(const blink::WebExternalTextureMailbox& mailbox)
 {
+    if (m_destructionInProgress) {
+        mailboxReleasedInDestructionInProgress(mailbox);
+        return;
+    }
+
     for (size_t i = 0; i < m_textureMailboxes.size(); i++) {
         RefPtr<MailboxInfo> mailboxInfo = m_textureMailboxes[i];
         if (!memcmp(mailboxInfo->mailbox.name, mailbox.name, sizeof(mailbox.name))) {
             mailboxInfo->mailbox.syncPoint = mailbox.syncPoint;
+            ASSERT(mailboxInfo->m_parentDrawingBuffer.get() == this);
+            mailboxInfo->m_parentDrawingBuffer.clear();
             m_recycledMailboxes.prepend(mailboxInfo.release());
             return;
         }
@@ -247,6 +279,23 @@ void DrawingBuffer::mailboxReleased(const blink::WebExternalTextureMailbox& mail
     ASSERT_NOT_REACHED();
 }
 
+void DrawingBuffer::mailboxReleasedInDestructionInProgress(const blink::WebExternalTextureMailbox& mailbox)
+{
+    ASSERT(!m_textureMailboxes.isEmpty());
+    for (size_t i = 0; i < m_textureMailboxes.size(); i++) {
+        RefPtr<MailboxInfo> mailboxInfo = m_textureMailboxes[i];
+        if (!memcmp(mailboxInfo->mailbox.name, mailbox.name, sizeof(mailboxInfo->mailbox.name))) {
+            m_context->makeContextCurrent();
+            m_context->deleteTexture(mailboxInfo->textureId);
+            m_textureMailboxes.remove(i);
+            ASSERT(mailboxInfo->m_parentDrawingBuffer.get() == this);
+            mailboxInfo->m_parentDrawingBuffer.clear();
+            return;
+        }
+    }
+    ASSERT_NOT_REACHED();
+}
+
 PassRefPtr<DrawingBuffer::MailboxInfo> DrawingBuffer::recycledMailbox()
 {
     if (m_recycledMailboxes.isEmpty())
@@ -473,14 +522,27 @@ void DrawingBuffer::clearPlatformLayer()
     m_context->flush();
 }
 
-void DrawingBuffer::releaseResources()
+void DrawingBuffer::beginDestruction()
 {
+    ASSERT(!m_destructionInProgress);
+    m_destructionInProgress = true;
+
     m_context->makeContextCurrent();
 
     clearPlatformLayer();
 
-    for (size_t i = 0; i < m_textureMailboxes.size(); i++)
-        m_context->deleteTexture(m_textureMailboxes[i]->textureId);
+    for (size_t i = 0; i < m_recycledMailboxes.size(); i++) {
+        m_context->deleteTexture(m_recycledMailboxes[i]->textureId);
+
+        for (size_t j = 0; j < m_textureMailboxes.size(); j++) {
+            RefPtr<MailboxInfo> mailboxInfo = m_textureMailboxes[j];
+            if (mailboxInfo->textureId == m_recycledMailboxes[i]->textureId) {
+                ASSERT(!memcmp(mailboxInfo->mailbox.name, m_recycledMailboxes[i]->mailbox.name, sizeof(mailboxInfo->mailbox.name)));
+                m_textureMailboxes.remove(j);

[Justin Novosad, 2014/04/09 15:34:35]

I think this is unnecessary complexity. Wouldn't it be sufficient to just have
m_textureMailboxes.clear() in the destructor?  Deleting just the textures as
they are released is good enough IMHO. You could remove the entire inner for
loop here, and avoid O(N^2)

[dshwang, 2014/04/09 18:05:50]

Thank you for review.

As you mentioned, "nested for" is not actually needed. I did it to clarify code
flow.
For example,
1. if m_textureMailboxes.isEmpty(), this method deletes m_context and m_layer
although the destructor can delete them.
2. the destructor ensures ASSERT(m_textureMailboxes.isEmpty());

m_textureMailboxes.size() is usually 3 and m_recycledMailboxes.size() is 1 or 2.
So I think 6 loop is not very heavy.

[Ken Russell (switch to Gerrit), 2014/04/15 02:24:02]

I think keeping this code is a good idea only to enable the assertion in the
destructor. Otherwise the state management of the m_parentDrawingBuffer pointer
in the m_recycledMailboxes and m_textureMailboxes vectors, especially while
destruction is in progress, is really confusing.

+                break;
+            }
+        }
+    }
 
     if (m_multisampleFBO)
         m_context->deleteFramebuffer(m_multisampleFBO);
@@ -503,8 +565,6 @@ void DrawingBuffer::releaseResources()
     if (m_colorBuffer)
         m_context->deleteTexture(m_colorBuffer);
 
-    m_context.clear();
-
     setSize(IntSize());
 
     m_colorBuffer = 0;
@@ -518,10 +578,12 @@ void DrawingBuffer::releaseResources()
     m_contextEvictionManager.clear();
 
     m_recycledMailboxes.clear();
-    m_textureMailboxes.clear();
 
-    if (m_layer) {
+    if (m_layer)
         GraphicsLayer::unregisterContentsLayer(m_layer->layer());
+
+    if (m_textureMailboxes.isEmpty()) {
+        m_context.clear();
         m_layer.clear();

[Ken Russell (switch to Gerrit), 2014/04/15 02:24:02]

This eager clearing of m_context and m_layer adds complexity. It isn't done in
releaseMailbox, above. Please remove this block and let the destructor take care
of clearing out the context and layer.

     }
 }