Fix manipulator Python API crash w/ undo

Split up manipulator free & unlink, so freeing window data doesn't
run callbacks that might use freed data.

8 files changed, 67 insertions, 28 deletions

diff --git a/source/blender/makesrna/intern/rna_wm_manipulator.c b/source/blender/makesrna/intern/rna_wm_manipulator.c
index fc7dcdbe3cb..28821ecb0bd 100644
--- a/source/blender/makesrna/intern/rna_wm_manipulator.c
+++ b/source/blender/makesrna/intern/rna_wm_manipulator.c
@@ -522,14 +522,14 @@ static wmManipulator *rna_ManipulatorGroup_manipulator_new(
 static void rna_ManipulatorGroup_manipulator_remove(
         wmManipulatorGroup *mgroup, bContext *C, wmManipulator *mpr)
 {
-	WM_manipulator_free(&mgroup->manipulators, mgroup->parent_mmap, mpr, C);
+	WM_manipulator_unlink(&mgroup->manipulators, mgroup->parent_mmap, mpr, C);
 }
 
 static void rna_ManipulatorGroup_manipulator_clear(
         wmManipulatorGroup *mgroup, bContext *C)
 {
 	while (mgroup->manipulators.first) {
-		WM_manipulator_free(&mgroup->manipulators, mgroup->parent_mmap, mgroup->manipulators.first, C);
+		WM_manipulator_unlink(&mgroup->manipulators, mgroup->parent_mmap, mgroup->manipulators.first, C);
 	}
 }
 
diff --git a/source/blender/windowmanager/manipulators/WM_manipulator_api.h b/source/blender/windowmanager/manipulators/WM_manipulator_api.h
index 9211c3365c5..a4b4964384c 100644
--- a/source/blender/windowmanager/manipulators/WM_manipulator_api.h
+++ b/source/blender/windowmanager/manipulators/WM_manipulator_api.h
@@ -63,12 +63,14 @@ struct wmManipulator *WM_manipulator_new_ptr(
 struct wmManipulator *WM_manipulator_new(
         const char *idname, struct wmManipulatorGroup *mgroup,
         struct PointerRNA *properties);
-void WM_manipulator_free(
+void WM_manipulator_free(struct wmManipulator *mpr);
+void WM_manipulator_unlink(
         ListBase *manipulatorlist, struct wmManipulatorMap *mmap, struct wmManipulator *mpr,
         struct bContext *C);
 
 void WM_manipulator_name_set(struct wmManipulatorGroup *mgroup, struct wmManipulator *mpr, const char *name);
 
+bool WM_manipulator_select_unlink(struct wmManipulatorMap *mmap, struct wmManipulator *mpr);
 bool WM_manipulator_select_set(struct wmManipulatorMap *mmap, struct wmManipulator *mpr, bool select);
 
 struct PointerRNA *WM_manipulator_set_operator(
diff --git a/source/blender/windowmanager/manipulators/intern/wm_manipulator.c b/source/blender/windowmanager/manipulators/intern/wm_manipulator.c
index 4936afde0c1..82ce0cf24ba 100644
--- a/source/blender/windowmanager/manipulators/intern/wm_manipulator.c
+++ b/source/blender/windowmanager/manipulators/intern/wm_manipulator.c
@@ -157,10 +157,11 @@ static void wm_manipulator_register(wmManipulatorGroup *mgroup, wmManipulator *m
 }
 
 /**
- * Free \a manipulator and unlink from \a manipulatorlist.
- * \a manipulatorlist is allowed to be NULL.
+ * \warning this doesn't check #wmManipulatorMap (highlight, selection etc).
+ * Typical use is when freeing the windowing data,
+ * where caller can manage clearing selection, highlight... etc.
  */
-void WM_manipulator_free(ListBase *manipulatorlist, wmManipulatorMap *mmap, wmManipulator *mpr, bContext *C)
+void WM_manipulator_free(wmManipulator *mpr)
 {
 #ifdef WITH_PYTHON
 	if (mpr->py_instance) {
@@ -170,16 +171,6 @@ void WM_manipulator_free(ListBase *manipulatorlist, wmManipulatorMap *mmap, wmMa
 	}
 #endif
 
-	if (mpr->state & WM_MANIPULATOR_STATE_HIGHLIGHT) {
-		wm_manipulatormap_highlight_set(mmap, C, NULL, 0);
-	}
-	if (mpr->state & WM_MANIPULATOR_STATE_MODAL) {
-		wm_manipulatormap_modal_set(mmap, C, NULL, NULL);
-	}
-	if (mpr->state & WM_MANIPULATOR_STATE_SELECT) {
-		WM_manipulator_select_set(mmap, mpr, false);
-	}
-
 	if (mpr->op_data.ptr.data) {
 		WM_operator_properties_free(&mpr->op_data.ptr);
 	}
@@ -199,6 +190,26 @@ void WM_manipulator_free(ListBase *manipulatorlist, wmManipulatorMap *mmap, wmMa
 		}
 	}
 
+	MEM_freeN(mpr);
+}
+
+/**
+ * Free \a manipulator and unlink from \a manipulatorlist.
+ * \a manipulatorlist is allowed to be NULL.
+ */
+void WM_manipulator_unlink(ListBase *manipulatorlist, wmManipulatorMap *mmap, wmManipulator *mpr, bContext *C)
+{
+	if (mpr->state & WM_MANIPULATOR_STATE_HIGHLIGHT) {
+		wm_manipulatormap_highlight_set(mmap, C, NULL, 0);
+	}
+	if (mpr->state & WM_MANIPULATOR_STATE_MODAL) {
+		wm_manipulatormap_modal_set(mmap, C, NULL, NULL);
+	}
+	/* Unlink instead of setting so we don't run callbacks. */
+	if (mpr->state & WM_MANIPULATOR_STATE_SELECT) {
+		WM_manipulator_select_unlink(mmap, mpr);
+	}
+
 	if (manipulatorlist) {
 		BLI_remlink(manipulatorlist, mpr);
 	}
@@ -206,7 +217,7 @@ void WM_manipulator_free(ListBase *manipulatorlist, wmManipulatorMap *mmap, wmMa
 	BLI_assert(mmap->mmap_context.highlight != mpr);
 	BLI_assert(mmap->mmap_context.modal != mpr);
 
-	MEM_freeN(mpr);
+	WM_manipulator_free(mpr);
 }
 
 /* -------------------------------------------------------------------- */
@@ -367,7 +378,9 @@ void WM_manipulator_set_fn_custom_modal(struct wmManipulator *mpr, wmManipulator
  *
  * \return if the selection has changed.
  */
-bool wm_manipulator_select_set_ex(wmManipulatorMap *mmap, wmManipulator *mpr, bool select, bool use_array)
+bool wm_manipulator_select_set_ex(
+        wmManipulatorMap *mmap, wmManipulator *mpr, bool select,
+        bool use_array, bool use_callback)
 {
 	bool changed = false;
 
@@ -390,7 +403,9 @@ bool wm_manipulator_select_set_ex(wmManipulatorMap *mmap, wmManipulator *mpr, bo
 		}
 	}
 
-	if (changed) {
+	/* In the case of unlinking we only want to remove from the array
+	 * and not write to the external state */
+	if (use_callback && changed) {
 		if (mpr->type->select_refresh) {
 			mpr->type->select_refresh(mpr);
 		}
@@ -399,9 +414,15 @@ bool wm_manipulator_select_set_ex(wmManipulatorMap *mmap, wmManipulator *mpr, bo
 	return changed;
 }
 
+/* Remove from selection array without running callbacks. */
+bool WM_manipulator_select_unlink(wmManipulatorMap *mmap, wmManipulator *mpr)
+{
+	return wm_manipulator_select_set_ex(mmap, mpr, false, true, false);
+}
+
 bool WM_manipulator_select_set(wmManipulatorMap *mmap, wmManipulator *mpr, bool select)
 {
-	return wm_manipulator_select_set_ex(mmap, mpr, select, true);
+	return wm_manipulator_select_set_ex(mmap, mpr, select, true, true);
 }
 
 bool wm_manipulator_select_and_highlight(bContext *C, wmManipulatorMap *mmap, wmManipulator *mpr)
diff --git a/source/blender/windowmanager/manipulators/intern/wm_manipulator_group.c b/source/blender/windowmanager/manipulators/intern/wm_manipulator_group.c
index 08dc9f2c8f9..f9cce984708 100644
--- a/source/blender/windowmanager/manipulators/intern/wm_manipulator_group.c
+++ b/source/blender/windowmanager/manipulators/intern/wm_manipulator_group.c
@@ -91,11 +91,24 @@ wmManipulatorGroup *wm_manipulatorgroup_new_from_type(
 void wm_manipulatorgroup_free(bContext *C, wmManipulatorGroup *mgroup)
 {
 	wmManipulatorMap *mmap = mgroup->parent_mmap;
+
+	/* Similar to WM_manipulator_unlink, but only to keep mmap state correct,
+	 * we don't want to run callbacks. */
+	if (mmap->mmap_context.highlight && mmap->mmap_context.highlight->parent_mgroup == mgroup) {
+		wm_manipulatormap_highlight_set(mmap, C, NULL, 0);
+	}
+	if (mmap->mmap_context.modal && mmap->mmap_context.modal->parent_mgroup == mgroup) {
+		wm_manipulatormap_modal_set(mmap, C, NULL, NULL);
+	}
+
 	for (wmManipulator *mpr = mgroup->manipulators.first, *mpr_next; mpr; mpr = mpr_next) {
 		mpr_next = mpr->next;
-		WM_manipulator_free(&mgroup->manipulators, mmap, mpr, C);
+		if (mmap->mmap_context.select.len) {
+			WM_manipulator_select_unlink(mmap, mpr);
+		}
+		WM_manipulator_free(mpr);
 	}
-	BLI_assert(BLI_listbase_is_empty(&mgroup->manipulators));
+	BLI_listbase_clear(&mgroup->manipulators);
 
 #ifdef WITH_PYTHON
 	if (mgroup->py_instance) {
diff --git a/source/blender/windowmanager/manipulators/intern/wm_manipulator_intern.h b/source/blender/windowmanager/manipulators/intern/wm_manipulator_intern.h
index f7b82bfc68b..b8be482c3b8 100644
--- a/source/blender/windowmanager/manipulators/intern/wm_manipulator_intern.h
+++ b/source/blender/windowmanager/manipulators/intern/wm_manipulator_intern.h
@@ -39,7 +39,9 @@ struct GHashIterator;
 /* wmManipulator */
 
 
-bool wm_manipulator_select_set_ex(struct wmManipulatorMap *mmap, struct wmManipulator *mpr, bool select, bool use_array);
+bool wm_manipulator_select_set_ex(
+        struct wmManipulatorMap *mmap, struct wmManipulator *mpr, bool select,
+        bool use_array, bool use_callback);
 bool wm_manipulator_select_and_highlight(bContext *C, struct wmManipulatorMap *mmap, struct wmManipulator *mpr);
 
 void wm_manipulator_calculate_scale(struct wmManipulator *mpr, const bContext *C);
diff --git a/source/blender/windowmanager/manipulators/intern/wm_manipulator_map.c b/source/blender/windowmanager/manipulators/intern/wm_manipulator_map.c
index c0da31fdbdd..1a96846e75d 100644
--- a/source/blender/windowmanager/manipulators/intern/wm_manipulator_map.c
+++ b/source/blender/windowmanager/manipulators/intern/wm_manipulator_map.c
@@ -182,6 +182,9 @@ void wm_manipulatormap_remove(wmManipulatorMap *mmap)
 	if (!mmap)
 		return;
 
+	/* Clear first so further calls don't waste time trying to maintain correct array state. */
+	wm_manipulatormap_select_array_clear(mmap);
+
 	for (wmManipulatorGroup *mgroup = mmap->groups.first, *mgroup_next; mgroup; mgroup = mgroup_next) {
 		mgroup_next = mgroup->next;
 		BLI_assert(mgroup->parent_mmap == mmap);
@@ -189,8 +192,6 @@ void wm_manipulatormap_remove(wmManipulatorMap *mmap)
 	}
 	BLI_assert(BLI_listbase_is_empty(&mmap->groups));
 
-	wm_manipulatormap_select_array_clear(mmap);
-
 	MEM_freeN(mmap);
 }
 
@@ -636,7 +637,7 @@ bool wm_manipulatormap_deselect_all(wmManipulatorMap *mmap)
 	}
 
 	for (int i = 0; i < msel->len; i++) {
-		wm_manipulator_select_set_ex(mmap, msel->items[i], false, false);
+		wm_manipulator_select_set_ex(mmap, msel->items[i], false, false, true);
 	}
 
 	wm_manipulatormap_select_array_clear(mmap);
diff --git a/source/blender/windowmanager/manipulators/intern/wm_manipulator_type.c b/source/blender/windowmanager/manipulators/intern/wm_manipulator_type.c
index 976ef1ddab2..18265319024 100644
--- a/source/blender/windowmanager/manipulators/intern/wm_manipulator_type.c
+++ b/source/blender/windowmanager/manipulators/intern/wm_manipulator_type.c
@@ -150,7 +150,7 @@ static void manipulatortype_unlink(
 								mpr_next = mpr->next;
 								BLI_assert(mgroup->parent_mmap == mmap);
 								if (mpr->type == wt) {
-									WM_manipulator_free(&mgroup->manipulators, mgroup->parent_mmap, mpr, C);
+									WM_manipulator_unlink(&mgroup->manipulators, mgroup->parent_mmap, mpr, C);
 									ED_region_tag_redraw(ar);
 								}
 							}
diff --git a/source/blenderplayer/bad_level_call_stubs/stubs.c b/source/blenderplayer/bad_level_call_stubs/stubs.c
index 7016554a1b5..2ead8db0146 100644
--- a/source/blenderplayer/bad_level_call_stubs/stubs.c
+++ b/source/blenderplayer/bad_level_call_stubs/stubs.c
@@ -371,7 +371,7 @@ const struct wmManipulatorType *WM_manipulatortype_find(const char *idname, bool
 struct wmManipulator *WM_manipulator_new_ptr(const struct wmManipulatorType *wt, struct wmManipulatorGroup *mgroup, struct PointerRNA *properties) RET_NULL
 struct wmManipulatorGroupType *WM_manipulatorgrouptype_append_ptr(void (*mnpfunc)(struct wmManipulatorGroupType *, void *), void *userdata) RET_NULL
 struct wmManipulatorGroupType *WM_manipulatorgrouptype_find(const char *idname, bool quiet) RET_NULL
-void WM_manipulator_free(ListBase *manipulatorlist, struct wmManipulatorMap *mmap, struct wmManipulator *mpr, struct bContext *C) RET_NONE
+void WM_manipulator_unlink(ListBase *manipulatorlist, struct wmManipulatorMap *mmap, struct wmManipulator *mpr, struct bContext *C) RET_NONE
 void WM_manipulator_group_add_ptr(struct wmManipulatorGroupType *wgt) RET_NONE
 void WM_manipulator_group_add_ptr_ex(struct wmManipulatorGroupType *wgt, struct wmManipulatorMapType *mmap_type) RET_NONE
 void WM_manipulator_group_remove_ptr(struct Main *bmain, struct wmManipulatorGroupType *wgt) RET_NONE