From bfeb285dcf6c5a30b0c9b27956d1a7b8cc31f7af Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Tue, 17 Apr 2018 16:37:07 -0400
Subject: [PATCH] Fix BindGroupLayout leak in PipelineLayout default.

PipelineLayout was re-entering in NXT to create the default empty BGLs,
but forgot to remove the initial external refcount for them.

Fixing this showed an issue where it was impossible to externally
reference a RefCounted that had only internal references, fix this as
well and add a test.
---
 src/backend/PipelineLayout.cpp          |  2 ++
 src/backend/RefCounted.cpp              | 14 +++++++++++++-
 src/tests/unittests/RefCountedTests.cpp | 18 ++++++++++++++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/src/backend/PipelineLayout.cpp b/src/backend/PipelineLayout.cpp
index 6adca64f29..1b0e528b8a 100644
--- a/src/backend/PipelineLayout.cpp
+++ b/src/backend/PipelineLayout.cpp
@@ -60,6 +60,8 @@ namespace backend {
         for (size_t group = 0; group < kMaxBindGroups; ++group) {
             if (!mBindGroupLayouts[group]) {
                 mBindGroupLayouts[group] = mDevice->CreateBindGroupLayoutBuilder()->GetResult();
+                // Remove the external ref objects are created with
+                mBindGroupLayouts[group]->Release();
             }
         }
 
diff --git a/src/backend/RefCounted.cpp b/src/backend/RefCounted.cpp
index c5a56c1f6c..133dd5c354 100644
--- a/src/backend/RefCounted.cpp
+++ b/src/backend/RefCounted.cpp
@@ -26,13 +26,16 @@ namespace backend {
 
     void RefCounted::ReferenceInternal() {
         ASSERT(mInternalRefs != 0);
+
         // TODO(cwallez@chromium.org): what to do on overflow?
         mInternalRefs++;
     }
 
     void RefCounted::ReleaseInternal() {
         ASSERT(mInternalRefs != 0);
+
         mInternalRefs--;
+
         if (mInternalRefs == 0) {
             ASSERT(mExternalRefs == 0);
             // TODO(cwallez@chromium.org): would this work with custom allocators?
@@ -49,14 +52,23 @@ namespace backend {
     }
 
     void RefCounted::Reference() {
-        ASSERT(mExternalRefs != 0);
+        ASSERT(mInternalRefs != 0);
+
+        // mExternalRefs != 0 counts as one internal ref.
+        if (mExternalRefs == 0) {
+            ReferenceInternal();
+        }
+
         // TODO(cwallez@chromium.org): what to do on overflow?
         mExternalRefs++;
     }
 
     void RefCounted::Release() {
+        ASSERT(mInternalRefs != 0);
         ASSERT(mExternalRefs != 0);
+
         mExternalRefs--;
+        // mExternalRefs != 0 counts as one internal ref.
         if (mExternalRefs == 0) {
             ReleaseInternal();
         }
diff --git a/src/tests/unittests/RefCountedTests.cpp b/src/tests/unittests/RefCountedTests.cpp
index 1ba3a97628..f5b0007f22 100644
--- a/src/tests/unittests/RefCountedTests.cpp
+++ b/src/tests/unittests/RefCountedTests.cpp
@@ -60,6 +60,24 @@ TEST(RefCounted, InternalRefKeepsAlive) {
     ASSERT_TRUE(deleted);
 }
 
+// Test that when adding an external ref from 0, an internal ref is added
+TEST(RefCounted, AddExternalRefFromZero) {
+    bool deleted = false;
+    auto test = new RCTest(&deleted);
+
+    test->ReferenceInternal();
+    test->Release();
+    ASSERT_FALSE(deleted);
+
+    // Reference adds an internal ref and release removes one
+    test->Reference();
+    test->Release();
+    ASSERT_FALSE(deleted);
+
+    test->ReleaseInternal();
+    ASSERT_TRUE(deleted);
+}
+
 // Test Ref remove internal reference when going out of scope
 TEST(Ref, EndOfScopeRemovesInternalRef) {
     bool deleted = false;