[Bf-blender-cvs] [e9220d5cd01] master: Depsgraph: Fix crash deleting Viewer image from Outliner

[Sergey Sharybin]

Commit: e9220d5cd013340bde288c85b5125028c1400499
Author: Sergey Sharybin
Date:   Wed Mar 11 17:25:17 2020 +0100
Branches: master
https://developer.blender.org/rBe9220d5cd013340bde288c85b5125028c1400499

Depsgraph: Fix crash deleting Viewer image from Outliner

Was happening when having compositor open with Viewer node attached
directly to Render Layers output.

There were two things involved here:

1. The code which was storing CoW-ed versions of IDs was checking all
   IDs for whether they are expanded or not. This was causing access
   of freed memory for deleted IDs which do not need CoW (such as IM).

   Simple fix: store ID type as a scalar and use early check before
   doing more elaborate check based on accessing fields of id_cow.

2. The code which was ensuring view layer pointer is doing CoW for
   scene. This isn't an issue on its own, but scene might have an
   embedded ID such as compositor which was actually traversed by the
   ID remap routines. This was causing remapping procedure to go into
   non-updated copy of compositor, accessing freed Viewer image ID.

   Solved by not recursing into embedded IDs for datablocks as those
   are supposed to have own copy-on-write operations which takes care
   of re-mapping.

Reported my Bastien, and also pair-coded with him.

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

M	source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
M	source/blender/depsgraph/intern/eval/deg_eval.cc
M	source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
M	source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.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 01d84f6b5d8..c512300200e 100644
--- a/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
+++ b/source/blender/depsgraph/intern/builder/deg_builder_nodes.cc
@@ -319,6 +319,18 @@ void DepsgraphNodeBuilder::begin_build()
    * 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
+     * for whether id_cow is expanded to access freed memory. In orderr to deal with this we
+     * check whether CoW is needed based on a scalar value which does not lead to access of
+     * possibly deleted memory.
+     * Additionally, this saves some space in the map by skipping mapping for datablocks which
+     * do not need CoW, */
+    if (!deg_copy_on_write_is_needed(id_node->id_type)) {
+      id_node->id_cow = nullptr;
+      continue;
+    }
+
     IDInfo *id_info = (IDInfo *)MEM_mallocN(sizeof(IDInfo), "depsgraph id info");
     if (deg_copy_on_write_is_expanded(id_node->id_cow) && id_node->id_orig != id_node->id_cow) {
       id_info->id_cow = id_node->id_cow;
diff --git a/source/blender/depsgraph/intern/eval/deg_eval.cc b/source/blender/depsgraph/intern/eval/deg_eval.cc
index df61a1416bd..1296c87bbb0 100644
--- a/source/blender/depsgraph/intern/eval/deg_eval.cc
+++ b/source/blender/depsgraph/intern/eval/deg_eval.cc
@@ -37,6 +37,7 @@
 
 #include "DNA_object_types.h"
 #include "DNA_scene_types.h"
+#include "DNA_node_types.h"
 
 #include "DEG_depsgraph.h"
 #include "DEG_depsgraph_query.h"
@@ -343,11 +344,13 @@ void depsgraph_ensure_view_layer(Depsgraph *graph)
    * - It was tagged for update of CoW component.
    * This allows us to have proper view layer pointer. */
   Scene *scene_cow = graph->scene_cow;
-  if (!deg_copy_on_write_is_expanded(&scene_cow->id) ||
-      scene_cow->id.recalc & ID_RECALC_COPY_ON_WRITE) {
-    const IDNode *id_node = graph->find_id_node(&graph->scene->id);
-    deg_update_copy_on_write_datablock(graph, id_node);
+  if (deg_copy_on_write_is_expanded(&scene_cow->id) &&
+      (scene_cow->id.recalc & ID_RECALC_COPY_ON_WRITE) == 0) {
+    return;
   }
+
+  const IDNode *scene_id_node = graph->find_id_node(&graph->scene->id);
+  deg_update_copy_on_write_datablock(graph, scene_id_node);
 }
 
 }  // namespace
diff --git a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
index 50870d1e4aa..d8ce590c611 100644
--- a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
+++ b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
@@ -914,8 +914,11 @@ ID *deg_expand_copy_on_write_datablock(const Depsgraph *depsgraph,
   user_data.depsgraph = depsgraph;
   user_data.node_builder = node_builder;
   user_data.create_placeholders = create_placeholders;
-  BKE_library_foreach_ID_link(
-      nullptr, id_cow, foreach_libblock_remap_callback, (void *)&user_data, IDWALK_NOP);
+  BKE_library_foreach_ID_link(nullptr,
+                              id_cow,
+                              foreach_libblock_remap_callback,
+                              (void *)&user_data,
+                              IDWALK_IGNORE_EMBEDDED_ID);
   /* Correct or tweak some pointers which are not taken care by foreach
    * from above. */
   update_id_after_copy(depsgraph, id_node, id_orig, id_cow);
@@ -1113,6 +1116,11 @@ bool deg_copy_on_write_is_expanded(const ID *id_cow)
 bool deg_copy_on_write_is_needed(const ID *id_orig)
 {
   const ID_Type id_type = GS(id_orig->name);
+  return deg_copy_on_write_is_needed(id_type);
+}
+
+bool deg_copy_on_write_is_needed(const ID_Type id_type)
+{
   return ID_TYPE_IS_COW(id_type);
 }
 
diff --git a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h
index 1992c80e036..05464d11f13 100644
--- a/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h
+++ b/source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.h
@@ -25,6 +25,8 @@
 
 #include <stddef.h>
 
+#include "DNA_ID.h"
+
 struct ID;
 
 /* Uncomment this to have verbose log about original and CoW pointers
@@ -94,5 +96,6 @@ bool deg_copy_on_write_is_expanded(const struct ID *id_cow);
  * This includes images.
  */
 bool deg_copy_on_write_is_needed(const ID *id_orig);
+bool deg_copy_on_write_is_needed(const ID_Type id_type);
 
 }  // namespace DEG
diff --git a/source/blender/depsgraph/intern/node/deg_node_id.cc b/source/blender/depsgraph/intern/node/deg_node_id.cc
index 0fbf658ceb3..cd25cc14069 100644
--- a/source/blender/depsgraph/intern/node/deg_node_id.cc
+++ b/source/blender/depsgraph/intern/node/deg_node_id.cc
@@ -103,6 +103,7 @@ void IDNode::init(const ID *id, const char *UNUSED(subdata))
 {
   BLI_assert(id != nullptr);
   /* Store ID-pointer. */
+  id_type = GS(id->name);
   id_orig = (ID *)id;
   eval_flags = 0;
   previous_eval_flags = 0;
diff --git a/source/blender/depsgraph/intern/node/deg_node_id.h b/source/blender/depsgraph/intern/node/deg_node_id.h
index 886c25b5a4e..6eea31ebff9 100644
--- a/source/blender/depsgraph/intern/node/deg_node_id.h
+++ b/source/blender/depsgraph/intern/node/deg_node_id.h
@@ -25,6 +25,7 @@
 
 #include "intern/node/deg_node.h"
 #include "BLI_sys_types.h"
+#include "DNA_ID.h"
 
 struct GHash;
 
@@ -73,6 +74,10 @@ 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 *id_orig;
   ID *id_cow;