Fix threading crash due to conflict in mesh wrapper type

A mesh wrapper might be being accessed for read-only from one thread
while another thread converts the wrapper type to something else.

The proposes solution is to defer assignment of the mesh wrapper
type until the wrapper is fully converted. The good thing about this
approach is that it does not introduce extra synchronization (and,
potentially, evaluation pipeline stalls). The downside is that it
might not work with all possible wrapper types in the future. If a
wrapper type which does not clear data separation is ever added in
the future we will re-consider the threading safety then.

Unfortunately, some changes outside of the mesh wrapper file are
to be made to allow "incremental" construction of the mesh prior
changing its wrapper type.

Unfortunately, there is no simplified file which demonstrates the
issue. It was investigated using Heist production file checked at
the revision 1228: `pro/lib/char/einar/einar.shading.blend`. The
repro case is simple: tab into edit mode, possibly few times.

The gist is that there several surface deform and shrinkwrap
modifiers which uses the same target. While one of them is building
BVH tree (which changes wrapper type) the other one accesses it for
read-only via `BKE_mesh_wrapper_vert_coords_copy_with_mat4()`.

Differential Revision: https://developer.blender.org/D15424

4 files changed, 40 insertions, 19 deletions

diff --git a/source/blender/blenkernel/BKE_mesh.h b/source/blender/blenkernel/BKE_mesh.h
index 731c9872aae..6c61068b1c2 100644
--- a/source/blender/blenkernel/BKE_mesh.h
+++ b/source/blender/blenkernel/BKE_mesh.h
@@ -85,10 +85,18 @@ struct Mesh *BKE_mesh_from_bmesh_for_eval_nomain(struct BMesh *bm,
  * Add original index (#CD_ORIGINDEX) layers if they don't already exist. This is meant to be used
  * when creating an evaluated mesh from an original edit mode mesh, to allow mapping from the
  * evaluated vertices to the originals.
+ *
+ * The mesh is expected to of a `ME_WRAPPER_TYPE_MDATA` wrapper type. This is asserted.
  */
 void BKE_mesh_ensure_default_orig_index_customdata(struct Mesh *mesh);
 
 /**
+ * Same as #BKE_mesh_ensure_default_orig_index_customdata but does not perform any checks: they
+ * must be done by the caller.
+ */
+void BKE_mesh_ensure_default_orig_index_customdata_no_check(struct Mesh *mesh);
+
+/**
  * Find the index of the loop in 'poly' which references vertex,
  * returns -1 if not found
  */
@@ -1002,8 +1010,8 @@ void BKE_mesh_calc_edges(struct Mesh *mesh, bool keep_existing_edges, bool selec
 void BKE_mesh_calc_edges_tessface(struct Mesh *mesh);
 
 /* In DerivedMesh.cc */
-void BKE_mesh_wrapper_deferred_finalize(struct Mesh *me_eval,
-                                        const struct CustomData_MeshMasks *cd_mask_finalize);
+void BKE_mesh_wrapper_deferred_finalize_mdata(struct Mesh *me_eval,
+                                              const struct CustomData_MeshMasks *cd_mask_finalize);
 
 /* **** Depsgraph evaluation **** */
 
diff --git a/source/blender/blenkernel/intern/DerivedMesh.cc b/source/blender/blenkernel/intern/DerivedMesh.cc
index c2ea01bcadf..a29d8726f21 100644
--- a/source/blender/blenkernel/intern/DerivedMesh.cc
+++ b/source/blender/blenkernel/intern/DerivedMesh.cc
@@ -84,6 +84,8 @@ static ThreadRWMutex loops_cache_lock = PTHREAD_RWLOCK_INITIALIZER;
 static void mesh_init_origspace(Mesh *mesh);
 static void editbmesh_calc_modifier_final_normals(Mesh *mesh_final,
                                                   const CustomData_MeshMasks *final_datamask);
+static void editbmesh_calc_modifier_final_normals_or_defer(
+    Mesh *mesh_final, const CustomData_MeshMasks *final_datamask);
 
 /* -------------------------------------------------------------------- */
 
@@ -663,8 +665,8 @@ static void mesh_calc_finalize(const Mesh *mesh_input, Mesh *mesh_eval)
   mesh_eval->edit_mesh = mesh_input->edit_mesh;
 }
 
-void BKE_mesh_wrapper_deferred_finalize(Mesh *me_eval,
-                                        const CustomData_MeshMasks *cd_mask_finalize)
+void BKE_mesh_wrapper_deferred_finalize_mdata(Mesh *me_eval,
+                                              const CustomData_MeshMasks *cd_mask_finalize)
 {
   if (me_eval->runtime.wrapper_type_finalize & (1 << ME_WRAPPER_TYPE_BMESH)) {
     editbmesh_calc_modifier_final_normals(me_eval, cd_mask_finalize);
@@ -1286,12 +1288,6 @@ bool editbmesh_modifier_is_enabled(const Scene *scene,
 static void editbmesh_calc_modifier_final_normals(Mesh *mesh_final,
                                                   const CustomData_MeshMasks *final_datamask)
 {
-  if (mesh_final->runtime.wrapper_type != ME_WRAPPER_TYPE_MDATA) {
-    /* Generated at draw time. */
-    mesh_final->runtime.wrapper_type_finalize = (1 << mesh_final->runtime.wrapper_type);
-    return;
-  }
-
   const bool calc_loop_normals = ((mesh_final->flag & ME_AUTOSMOOTH) != 0 ||
                                   (final_datamask->lmask & CD_MASK_NORMAL) != 0);
 
@@ -1319,6 +1315,18 @@ static void editbmesh_calc_modifier_final_normals(Mesh *mesh_final,
   }
 }
 
+static void editbmesh_calc_modifier_final_normals_or_defer(
+    Mesh *mesh_final, const CustomData_MeshMasks *final_datamask)
+{
+  if (mesh_final->runtime.wrapper_type != ME_WRAPPER_TYPE_MDATA) {
+    /* Generated at draw time. */
+    mesh_final->runtime.wrapper_type_finalize = (1 << mesh_final->runtime.wrapper_type);
+    return;
+  }
+
+  editbmesh_calc_modifier_final_normals(mesh_final, final_datamask);
+}
+
 static void editbmesh_calc_modifiers(struct Depsgraph *depsgraph,
                                      const Scene *scene,
                                      Object *ob,
@@ -1598,9 +1606,9 @@ static void editbmesh_calc_modifiers(struct Depsgraph *depsgraph,
   }
 
   /* Compute normals. */
-  editbmesh_calc_modifier_final_normals(mesh_final, &final_datamask);
+  editbmesh_calc_modifier_final_normals_or_defer(mesh_final, &final_datamask);
   if (mesh_cage && (mesh_cage != mesh_final)) {
-    editbmesh_calc_modifier_final_normals(mesh_cage, &final_datamask);
+    editbmesh_calc_modifier_final_normals_or_defer(mesh_cage, &final_datamask);
   }
 
   /* Return final mesh. */
diff --git a/source/blender/blenkernel/intern/mesh.cc b/source/blender/blenkernel/intern/mesh.cc
index 2a14370bf93..cf05dc0404e 100644
--- a/source/blender/blenkernel/intern/mesh.cc
+++ b/source/blender/blenkernel/intern/mesh.cc
@@ -1190,6 +1190,11 @@ static void ensure_orig_index_layer(CustomData &data, const int size)
 void BKE_mesh_ensure_default_orig_index_customdata(Mesh *mesh)
 {
   BLI_assert(mesh->runtime.wrapper_type == ME_WRAPPER_TYPE_MDATA);
+  BKE_mesh_ensure_default_orig_index_customdata_no_check(mesh);
+}
+
+void BKE_mesh_ensure_default_orig_index_customdata_no_check(Mesh *mesh)
+{
   ensure_orig_index_layer(mesh->vdata, mesh->totvert);
   ensure_orig_index_layer(mesh->edata, mesh->totedge);
   ensure_orig_index_layer(mesh->pdata, mesh->totpoly);
diff --git a/source/blender/blenkernel/intern/mesh_wrapper.cc b/source/blender/blenkernel/intern/mesh_wrapper.cc
index 0362e4866e3..0b61b876abe 100644
--- a/source/blender/blenkernel/intern/mesh_wrapper.cc
+++ b/source/blender/blenkernel/intern/mesh_wrapper.cc
@@ -103,11 +103,7 @@ void BKE_mesh_wrapper_ensure_mdata(Mesh *me)
 
   /* Must isolate multithreaded tasks while holding a mutex lock. */
   blender::threading::isolate_task([&]() {
-    const eMeshWrapperType geom_type_orig = static_cast<eMeshWrapperType>(
-        me->runtime.wrapper_type);
-    me->runtime.wrapper_type = ME_WRAPPER_TYPE_MDATA;
-
-    switch (geom_type_orig) {
+    switch (static_cast<eMeshWrapperType>(me->runtime.wrapper_type)) {
       case ME_WRAPPER_TYPE_MDATA:
       case ME_WRAPPER_TYPE_SUBD: {
         break; /* Quiet warning. */
@@ -132,7 +128,7 @@ void BKE_mesh_wrapper_ensure_mdata(Mesh *me)
          * There is also a performance aspect, where this also assumes that original indices are
          * always needed when converting an edit mesh to a mesh. That might be wrong, but it's not
          * harmful. */
-        BKE_mesh_ensure_default_orig_index_customdata(me);
+        BKE_mesh_ensure_default_orig_index_customdata_no_check(me);
 
         EditMeshData *edit_data = me->runtime.edit_data;
         if (edit_data->vertexCos) {
@@ -144,8 +140,12 @@ void BKE_mesh_wrapper_ensure_mdata(Mesh *me)
     }
 
     if (me->runtime.wrapper_type_finalize) {
-      BKE_mesh_wrapper_deferred_finalize(me, &me->runtime.cd_mask_extra);
+      BKE_mesh_wrapper_deferred_finalize_mdata(me, &me->runtime.cd_mask_extra);
     }
+
+    /* Keep type assignment last, so that read-only access only uses the mdata code paths after all
+     * the underlying data has been initialized. */
+    me->runtime.wrapper_type = ME_WRAPPER_TYPE_MDATA;
   });
 
   BLI_mutex_unlock(mesh_eval_mutex);