Fix race condition when keeping D3D12 buffers alive during capture

* There was a gap between stopping active capturing (meaning newly created
  buffers are given an extra refcount) and releasing the extra refcount. Any
  newly created buffers in that gap would still be released but that would mean
  one of the application's ref's is removed, possibly destroying a buffer by
  mistake.
* Instead we keep a specific list of buffers that we're extending so that we
  know that the buffers we release are exactly those we gave extra refs to.

renderdoc/driver/d3d12/d3d12_device.cpp

@@ -1829,8 +1829,8 @@ void WrappedID3D12Device::StartFrameCapture(void *dev, void *wnd)
                                                         eFrameRef_Read);
     }
 
-    // also keep buffers alive
-    WrappedID3D12Resource1::AddRefBuffersBeforeCapture(GetResourceManager());
+    m_RefQueues = m_Queues;
+    m_RefBuffers = WrappedID3D12Resource1::AddRefBuffersBeforeCapture(GetResourceManager());
   }
 
   GetResourceManager()->MarkResourceFrameReferenced(m_ResourceID, eFrameRef_Read);
@@ -2136,14 +2136,14 @@ bool WrappedID3D12Device::EndFrameCapture(void *dev, void *wnd)
   SAFE_DELETE(m_HeaderChunk);
 
   for(auto it = queues.begin(); it != queues.end(); ++it)
-  {
     (*it)->ClearAfterCapture();
 
-    // remove the reference held during capture, potentially releasing the queue.
-    (*it)->Release();
-  }
+  // remove the references held during capture, potentially releasing the queue/buffer.
+  for(WrappedID3D12CommandQueue *q : m_RefQueues)
+    q->Release();
 
-  WrappedID3D12Resource1::ReleaseBuffersAfterCapture(GetResourceManager());
+  for(ID3D12Resource *r : m_RefBuffers)
+    r->Release();
 
   GetResourceManager()->MarkUnwrittenResources();
 
@@ -2189,6 +2189,13 @@ bool WrappedID3D12Device::DiscardFrameCapture(void *dev, void *wnd)
   for(auto it = queues.begin(); it != queues.end(); ++it)
     (*it)->ClearAfterCapture();
 
+  // remove the reference held during capture, potentially releasing the queue.
+  for(WrappedID3D12CommandQueue *q : m_RefQueues)
+    q->Release();
+
+  for(ID3D12Resource *r : m_RefBuffers)
+    r->Release();
+
   GetResourceManager()->MarkUnwrittenResources();
 
   GetResourceManager()->ClearReferencedResources();

renderdoc/driver/d3d12/d3d12_device.h

@@ -361,6 +361,13 @@ private:
   rdcarray<WrappedID3D12CommandQueue *> m_Queues;
   rdcarray<ID3D12Fence *> m_QueueFences;
 
+  // list of queues and buffers kept alive during capture artificially even if the user destroys
+  // them, so we can use them in the capture. Storing this separately prevents races where a
+  // queue/buffer is added between us transitioning away from active capturing (so we don't addref
+  // it) and us releasing our reference on them.
+  rdcarray<WrappedID3D12CommandQueue *> m_RefQueues;
+  rdcarray<ID3D12Resource *> m_RefBuffers;
+
   // the queue we use for all internal work, the first DIRECT queue
   WrappedID3D12CommandQueue *m_Queue;
 

renderdoc/driver/d3d12/d3d12_device_wrap.cpp

@@ -136,15 +136,20 @@ HRESULT WrappedID3D12Device::CreateCommandQueue(const D3D12_COMMAND_QUEUE_DESC *
     {
       SCOPED_READLOCK(m_CapTransitionLock);
       capframe = IsActiveCapturing(m_State);
+
+      // while capturing don't allow any queues to be freed, by adding another refcount, since we
+      // gather any commands submitted to them at the end of the capture.
+      if(capframe)
+      {
+        wrapped->AddRef();
+        m_RefQueues.push_back(wrapped);
+      }
     }
 
-    // while capturing don't allow any queues to be freed, by adding another refcount, since we
-    // gather any commands submitted to them at the end of the capture.
     if(capframe)
     {
       GetResourceManager()->MarkResourceFrameReferenced(
           wrapped->GetCreationRecord()->GetResourceID(), eFrameRef_Read);
-      wrapped->AddRef();
     }
 
     *ppCommandQueue = (ID3D12CommandQueue *)wrapped;
@@ -1475,6 +1480,18 @@ HRESULT WrappedID3D12Device::CreateCommittedResource(const D3D12_HEAP_PROPERTIES
     }
 
     *ppvResource = (ID3D12Resource *)wrapped;
+
+    // while actively capturing we keep all buffers around to prevent the address lookup from
+    // losing addresses we might need (or the manageable but annoying problem of an address being
+    // re-used)
+    {
+      SCOPED_READLOCK(m_CapTransitionLock);
+      if(IsActiveCapturing(m_State))
+      {
+        wrapped->AddRef();
+        m_RefBuffers.push_back(wrapped);
+      }
+    }
   }
 
   return ret;
@@ -1750,6 +1767,18 @@ HRESULT WrappedID3D12Device::CreatePlacedResource(ID3D12Heap *pHeap, UINT64 Heap
     }
 
     *ppvResource = (ID3D12Resource *)wrapped;
+
+    // while actively capturing we keep all buffers around to prevent the address lookup from
+    // losing addresses we might need (or the manageable but annoying problem of an address being
+    // re-used)
+    {
+      SCOPED_READLOCK(m_CapTransitionLock);
+      if(IsActiveCapturing(m_State))
+      {
+        wrapped->AddRef();
+        m_RefBuffers.push_back(wrapped);
+      }
+    }
   }
 
   return ret;
@@ -2574,6 +2603,18 @@ HRESULT WrappedID3D12Device::OpenSharedHandleInternal(D3D12Chunk chunkType, REFI
 
         states.fill(GetNumSubresources(m_pDevice, &desc), InitialResourceState);
       }
+
+      // while actively capturing we keep all buffers around to prevent the address lookup from
+      // losing addresses we might need (or the manageable but annoying problem of an address being
+      // re-used)
+      {
+        SCOPED_READLOCK(m_CapTransitionLock);
+        if(IsActiveCapturing(m_State))
+        {
+          wrapped->AddRef();
+          m_RefBuffers.push_back(wrapped);
+        }
+      }
     }
 
     CACHE_THREAD_SERIALISER();

renderdoc/driver/d3d12/d3d12_device_wrap4.cpp

@@ -379,6 +379,18 @@ HRESULT WrappedID3D12Device::CreateCommittedResource1(
     }
 
     *ppvResource = (ID3D12Resource *)wrapped;
+
+    // while actively capturing we keep all buffers around to prevent the address lookup from
+    // losing addresses we might need (or the manageable but annoying problem of an address being
+    // re-used)
+    {
+      SCOPED_READLOCK(m_CapTransitionLock);
+      if(IsActiveCapturing(m_State))
+      {
+        wrapped->AddRef();
+        m_RefBuffers.push_back(wrapped);
+      }
+    }
   }
 
   return ret;

renderdoc/driver/d3d12/d3d12_resources.cpp

@@ -463,26 +463,24 @@ void WrappedID3D12Resource1::RefBuffers(D3D12ResourceManager *rm)
     rm->MarkResourceFrameReferenced(m_Addresses.addresses[i].id, eFrameRef_Read);
 }
 
-void WrappedID3D12Resource1::AddRefBuffersBeforeCapture(D3D12ResourceManager *rm)
+rdcarray<ID3D12Resource *> WrappedID3D12Resource1::AddRefBuffersBeforeCapture(D3D12ResourceManager *rm)
 {
-  SCOPED_READLOCK(m_Addresses.addressLock);
-  for(size_t i = 0; i < m_Addresses.addresses.size(); i++)
-    rm->GetCurrentResource(m_Addresses.addresses[i].id)->AddRef();
-}
+  rdcarray<ID3D12Resource *> ret;
 
-void WrappedID3D12Resource1::ReleaseBuffersAfterCapture(D3D12ResourceManager *rm)
-{
-  // make a copy because we might release the last reference on a buffer which will need to modify
-  // the actual m_Addresses
   rdcarray<GPUAddressRange> addresses;
-
   {
     SCOPED_READLOCK(m_Addresses.addressLock);
     addresses = m_Addresses.addresses;
   }
 
   for(size_t i = 0; i < addresses.size(); i++)
-    rm->GetCurrentResource(addresses[i].id)->Release();
+  {
+    ID3D12Resource *resource = (ID3D12Resource *)rm->GetCurrentResource(m_Addresses.addresses[i].id);
+    resource->AddRef();
+    ret.push_back(resource);
+  }
+
+  return ret;
 }
 
 WrappedID3D12DescriptorHeap::WrappedID3D12DescriptorHeap(ID3D12DescriptorHeap *real,

renderdoc/driver/d3d12/d3d12_resources.h

@@ -879,8 +879,7 @@ public:
 
   static void RefBuffers(D3D12ResourceManager *rm);
 
-  static void AddRefBuffersBeforeCapture(D3D12ResourceManager *rm);
-  static void ReleaseBuffersAfterCapture(D3D12ResourceManager *rm);
+  static rdcarray<ID3D12Resource *> AddRefBuffersBeforeCapture(D3D12ResourceManager *rm);
 
   static void GetResIDFromAddr(D3D12_GPU_VIRTUAL_ADDRESS addr, ResourceId &id, UINT64 &offs)
   {
@@ -924,12 +923,6 @@ public:
       range.id = GetResourceID();
 
       m_Addresses.AddTo(range);
-
-      // while actively capturing we keep all buffers around to prevent the address lookup from
-      // losing addresses we might need (or the manageable but annoying problem of an address being
-      // re-used)
-      if(IsActiveCapturing(device->GetState()))
-        AddRef();
     }
   }
   virtual ~WrappedID3D12Resource1();