[Bf-blender-cvs] [f6c7da57598] blender-v2.83-release: Fix T84397: Creating and removing many objects very quickly causes a crash

[Sergey Sharybin]

Commit: f6c7da575987a85e25571163a25dde659e1d56e0
Author: Sergey Sharybin
Date:   Tue Jan 12 17:12:27 2021 +0100
Branches: blender-v2.83-release
https://developer.blender.org/rBf6c7da575987a85e25571163a25dde659e1d56e0

Fix T84397: Creating and removing many objects very quickly causes a crash

The root of the issue was caused by the dependency graph using ID pointer
to map evaluated state from old depsgraph to new one upon relations update.
This was failing when IDs were re-allocated rapidly: was possible that
Object ID's evaluated state assigned to Mesh and vice versa.

Now depsgraph uses Session UUID to identify which IDs to restore evaluated
state to. The session UUID is stored in the IDNode, so that id_orig is not
dereferenced on depsgraph update since the ID might be freed.

The root of the issue is identified by Campbell, original patch was done
by Bastien, thanks! Also thanks to Oliver and Ray and everyone else for
testing!

===================================================================

M	source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
M	source/blender/depsgraph/intern/builder/deg_builder_nodes.h
M	source/blender/depsgraph/intern/node/deg_node_id.cc
M	source/blender/depsgraph/intern/node/deg_node_id.h

===================================================================

diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc b/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
index afe9383a5a7..b04c2f4fc25 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
+++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
@@ -83,6 +83,7 @@ extern "C" {
 #include "BKE_key.h"
 #include "BKE_lattice.h"
 #include "BKE_layer.h"
+#include "BKE_lib_id.h"
 #include "BKE_light.h"
 #include "BKE_mask.h"
 #include "BKE_material.h"
@@ -121,20 +122,6 @@ extern "C" {
 
 namespace DEG {
 
-namespace {
-
-void free_copy_on_write_datablock(void *id_info_v)
-{
-  DepsgraphNodeBuilder::IDInfo *id_info = (DepsgraphNodeBuilder::IDInfo *)id_info_v;
-  if (id_info->id_cow != nullptr) {
-    deg_free_copy_on_write_datablock(id_info->id_cow);
-    MEM_freeN(id_info->id_cow);
-  }
-  MEM_freeN(id_info);
-}
-
-} /* namespace */
-
 /* ************ */
 /* Node Builder */
 
@@ -148,26 +135,31 @@ DepsgraphNodeBuilder::DepsgraphNodeBuilder(Main *bmain,
       view_layer_(nullptr),
       view_layer_index_(-1),
       collection_(nullptr),
-      is_parent_collection_visible_(true),
-      id_info_hash_(nullptr)
+      is_parent_collection_visible_(true)
 {
 }
 
 DepsgraphNodeBuilder::~DepsgraphNodeBuilder()
 {
-  if (id_info_hash_ != nullptr) {
-    BLI_ghash_free(id_info_hash_, nullptr, free_copy_on_write_datablock);
+ for (IDInfo *id_info : id_info_hash_.values()) {
+    if (id_info->id_cow != nullptr) {
+      deg_free_copy_on_write_datablock(id_info->id_cow);
+      MEM_freeN(id_info->id_cow);
+    }
+    MEM_freeN(id_info);
   }
 }
 
 IDNode *DepsgraphNodeBuilder::add_id_node(ID *id)
 {
+  BLI_assert(id->session_uuid != MAIN_ID_SESSION_UUID_UNSET);
+
   IDNode *id_node = nullptr;
   ID *id_cow = nullptr;
   IDComponentsMask previously_visible_components_mask = 0;
   uint32_t previous_eval_flags = 0;
   DEGCustomDataMeshMasks previous_customdata_masks;
-  IDInfo *id_info = (IDInfo *)BLI_ghash_lookup(id_info_hash_, id);
+  IDInfo *id_info = id_info_hash_.lookup_default(id->session_uuid, nullptr);
   if (id_info != nullptr) {
     id_cow = id_info->id_cow;
     previously_visible_components_mask = id_info->previously_visible_components_mask;
@@ -321,7 +313,6 @@ void DepsgraphNodeBuilder::begin_build()
 {
   /* Store existing copy-on-write versions of datablock, so we can re-use
    * them for new ID nodes. */
-  id_info_hash_ = BLI_ghash_ptr_new("Depsgraph id hash");
   for (IDNode *id_node : graph_->id_nodes) {
     /* It is possible that the ID does not need to have CoW version in which case id_cow is the
      * same as id_orig. Additionally, such ID might have been removed, which makes the check
@@ -345,7 +336,8 @@ void DepsgraphNodeBuilder::begin_build()
     id_info->previously_visible_components_mask = id_node->visible_components_mask;
     id_info->previous_eval_flags = id_node->eval_flags;
     id_info->previous_customdata_masks = id_node->customdata_masks;
-    BLI_ghash_insert(id_info_hash_, id_node->id_orig, id_info);
+    BLI_assert(!id_info_hash_.contains(id_node->id_orig_session_uuid));
+    id_info_hash_.add_new(id_node->id_orig_session_uuid, id_info);
     id_node->id_cow = nullptr;
   }
 
diff --git a/source/blender/depsgraph/intern/builder/deg_builder_nodes.h b/source/blender/depsgraph/intern/builder/deg_builder_nodes.h
index bac3751cc31..56d97e18106 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder_nodes.h
+++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes.h
@@ -31,6 +31,8 @@
 
 #include "DEG_depsgraph.h"
 
+#include "BLI_map.h"
+
 struct CacheFile;
 struct Camera;
 struct Collection;
@@ -278,8 +280,8 @@ class DepsgraphNodeBuilder : public DepsgraphBuilder {
    * very root is visible (aka not restricted.). */
   bool is_parent_collection_visible_;
 
-  /* Indexed by original ID, values are IDInfo. */
-  GHash *id_info_hash_;
+  /* Indexed by original ID.session_uuid, values are IDInfo. */
+  BLI::Map<uint, IDInfo *> id_info_hash_;
 
   /* Set of IDs which were already build. Makes it easier to keep track of
    * what was already built and what was not. */
diff --git a/source/blender/depsgraph/intern/node/deg_node_id.cc b/source/blender/depsgraph/intern/node/deg_node_id.cc
index 4b6120a6985..fd51ed9fb31 100644
--- a/source/blender/depsgraph/intern/node/deg_node_id.cc
+++ b/source/blender/depsgraph/intern/node/deg_node_id.cc
@@ -104,6 +104,7 @@ void IDNode::init(const ID *id, const char *UNUSED(subdata))
   /* Store ID-pointer. */
   id_type = GS(id->name);
   id_orig = (ID *)id;
+  id_orig_session_uuid = id->session_uuid;
   eval_flags = 0;
   previous_eval_flags = 0;
   customdata_masks = DEGCustomDataMeshMasks();
diff --git a/source/blender/depsgraph/intern/node/deg_node_id.h b/source/blender/depsgraph/intern/node/deg_node_id.h
index 80bb67f182f..787767b556c 100644
--- a/source/blender/depsgraph/intern/node/deg_node_id.h
+++ b/source/blender/depsgraph/intern/node/deg_node_id.h
@@ -73,12 +73,22 @@ struct IDNode : public Node {
 
   IDComponentsMask get_visible_components_mask() const;
 
-  /* ID Block referenced. */
   /* Type of the ID stored separately, so it's possible to perform check whether CoW is needed
    * without de-referencing the id_cow (which is not safe when ID is NOT covered by CoW and has
    * been deleted from the main database.) */
   ID_Type id_type;
+
+  /* ID Block referenced. */
   ID *id_orig;
+
+  /* Session-wide UUID of the id_orig.
+   * Is used on relations update to map evaluated state from old nodes to the new ones, without
+   * relying on pointers (which are not guaranteed to be unique) and without dereferencing id_orig
+   * which could be "stale" pointer. */
+  uint id_orig_session_uuid;
+
+  /* Evaluated datablock.
+   * Will be covered by the copy-on-write system if the ID Type needs it. */
   ID *id_cow;
 
   /* Hash to make it faster to look up components. */