Fix crashes with redundant changes or 0 refcount binds. Refs #23, #503

* Two potential crashes here - one from the previous fix to #503, we
  would double-release a refcount on an object that was set for write.
  First we'd decrement the refcount when it was 'unbound', then again
  trying to unbind it for write because the slot wasn't NULL'd. This was
  just plain broken.
* The second more obscure one was when binding for read. If the external
  object had no other refcount than the one in the binding slot, then
  changing its binding would implicitly destroy it. However if the code
  was setting the same object back again (ie. with the pointer they had
  not reference to) then the refcount would drop to 0 then should be
  incremented again to 1 when it's re-bound. However because the count
  bounces off 0, the object is destroyed between being unbound and
  re-bound causing pure virtual calls and other crashes when we try to
  access it.
* The fix is first to check if we're binding something to its own slot
  and skip it. Second we need to keep the new objects ref'd at all times
  during the binding (in case we are e.g. performing an array bind which
  moves the 1-refcount object from slot 2 to slot 3. It would be unbound
  when processing slot 2, hit refcount 0, and then be added again when
  processing slot 3).

renderdoc/driver/d3d11/d3d11_renderstate.h

@@ -387,86 +387,89 @@ struct D3D11RenderState
   void ReleaseRef(ID3D11DeviceChild *p);
 
   template <typename T>
-  void ChangeRefRead(T **stateArray, T *const *newArray, size_t offset, size_t num)
+  void ChangeRefRead(T *&stateItem, T *newItem)
   {
-    for(size_t i = 0; i < num; i++)
-    {
-      T *old = stateArray[offset + i];
-      ReleaseRef(old);
+    // don't do anything for redundant changes. This prevents the object from bouncing off refcount
+    // 0 during the changeover if it's only bound once, has no external refcount.
+    if(stateItem == newItem)
+      return;
 
-      if(newArray[i] == NULL)
-      {
-        stateArray[offset + i] = newArray[i];
-      }
-      else
-      {
-        if(IsBoundForWrite(newArray[i]))
-        {
-          // RDCDEBUG("Resource %d was bound for write, forcing to NULL", offset+i);
-          stateArray[offset + i] = NULL;
-        }
-        else
-        {
-          stateArray[offset + i] = newArray[i];
-        }
-      }
-      TakeRef(stateArray[offset + i]);
+    // release the old item, which may destroy it but we won't use it again as we know is not the
+    // same as the new item.
+    ReleaseRef(stateItem);
+
+    // assign the new item, but don't ref it yet
+    stateItem = newItem;
+
+    // if the item is bound for writing anywhere, we instead bind NULL
+    if(IsBoundForWrite(newItem))
+    {
+      // RDCDEBUG("Resource was bound for write, forcing to NULL");
+      stateItem = NULL;
     }
+
+    // finally we take the ref on the new item
+    TakeRef(stateItem);
   }
 
   template <typename T>
-  void ChangeRefWrite(T **stateArray, T *const *newArray, size_t offset, size_t num)
+  void ChangeRefRead(T **stateArray, T *const *newArray, size_t offset, size_t num)
   {
+    // addref the whole array so none of it can be destroyed during processing
     for(size_t i = 0; i < num; i++)
-    {
-      T *old = stateArray[offset + i];
-      ReleaseRef(old);
+      if(newArray[i])
+        newArray[i]->AddRef();
+
+    for(size_t i = 0; i < num; i++)
+      ChangeRefRead(stateArray[offset + i], newArray[i]);
 
+    // release the ref we added above
+    for(size_t i = 0; i < num; i++)
       if(newArray[i])
-      {
-        UnbindForRead(newArray[i]);
-        // when binding something for write, all other write slots are NULL'd too
-        UnbindForWrite(newArray[i]);
-      }
-      stateArray[offset + i] = newArray[i];
-      TakeRef(stateArray[offset + i]);
-    }
+        newArray[i]->Release();
   }
 
   template <typename T>
-  void ChangeRefRead(T *&stateItem, T *newItem)
+  void ChangeRefWrite(T *&stateItem, T *newItem)
   {
+    // don't do anything for redundant changes. This prevents the object from bouncing off refcount
+    // 0 during the changeover if it's only bound once, has no external refcount.
+    if(stateItem == newItem)
+      return;
+
+    // release the old item, which may destroy it but we won't use it again as we know is not the
+    // same as the new item. We NULL it out so that it doesn't get unbound again below
     ReleaseRef(stateItem);
-    stateItem = newItem;
+    stateItem = NULL;
 
-    if(newItem == NULL)
-    {
-      stateItem = newItem;
-    }
-    else
+    // if we're not binding NULL, then unbind any other conflicting uses
+    if(newItem)
     {
-      if(IsBoundForWrite(newItem))
-      {
-        // RDCDEBUG("Resource was bound for write, forcing to NULL");
-        stateItem = NULL;
-      }
-      else
-      {
-        stateItem = newItem;
-      }
+      UnbindForRead(newItem);
+      // when binding something for write, all other write slots are NULL'd too
+      UnbindForWrite(newItem);
     }
 
-    TakeRef(newItem);
+    // now bind the new item and ref it
+    stateItem = newItem;
+    TakeRef(stateItem);
   }
 
   template <typename T>
-  void ChangeRefWrite(T *&stateItem, T *newItem)
+  void ChangeRefWrite(T **stateArray, T *const *newArray, size_t offset, size_t num)
   {
-    ReleaseRef(stateItem);
-    stateItem = newItem;
-    if(newItem)
-      UnbindForRead(newItem);
-    TakeRef(newItem);
+    // addref the whole array so none of it can be destroyed during processing
+    for(size_t i = 0; i < num; i++)
+      if(newArray[i])
+        newArray[i]->AddRef();
+
+    for(size_t i = 0; i < num; i++)
+      ChangeRefWrite(stateArray[offset + i], newArray[i]);
+
+    // release the ref we added above
+    for(size_t i = 0; i < num; i++)
+      if(newArray[i])
+        newArray[i]->Release();
   }
 
   template <typename T>