From b488988ab10a5f78d5cefe8346fc2fd2f1232d0f Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:32:27 +0200 Subject: [PATCH 1/9] Fix potential memory leak in Sequencer sound strip creation code. --- source/blender/blenkernel/intern/sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source/blender/blenkernel/intern/sequencer.c b/source/blender/blenkernel/intern/sequencer.c index 757f639775b..dbdffa54d29 100644 --- a/source/blender/blenkernel/intern/sequencer.c +++ b/source/blender/blenkernel/intern/sequencer.c @@ -5183,6 +5183,7 @@ Sequence *BKE_sequencer_add_sound_strip(bContext *C, ListBase *seqbasep, SeqLoad sound = BKE_sound_new_file(bmain, seq_load->path); /* handles relative paths */ if (sound->playback_handle == NULL) { + BKE_libblock_free(bmain, sound); #if 0 if (op) BKE_report(op->reports, RPT_ERROR, "Unsupported audio format"); From 7a80c34f52b69495a47680f45776a163b7e57755 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:34:12 +0200 Subject: [PATCH 2/9] Fix serious bug in 'curve-to-mesh' conversion code. Eeeeeek!^2 Calling unconditionnaly ID freeing `BKE_libblock_free()` on a datablock (ob->data, i.e. Curve) that may be used elsewhere... Veryveryvery bad! --- source/blender/blenkernel/intern/mesh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/blender/blenkernel/intern/mesh.c b/source/blender/blenkernel/intern/mesh.c index 67b1e0b5a7d..89e988782d2 100644 --- a/source/blender/blenkernel/intern/mesh.c +++ b/source/blender/blenkernel/intern/mesh.c @@ -1399,7 +1399,7 @@ void BKE_mesh_from_nurbs_displist(Object *ob, ListBase *dispbase, const bool use cu->totcol = 0; if (ob->data) { - BKE_libblock_free(bmain, ob->data); + BKE_libblock_free_us(bmain, ob->data); } ob->data = me; ob->type = OB_MESH; From 25c0666b901d8a083e79d606b5bd805ee18fee20 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:38:55 +0200 Subject: [PATCH 3/9] Fix potentially dnagerous code in doversionning of brush. Even though in that specific it was probably safe-ish, there is no guarantee at this point Brush we want to remove are not used somewhere, better take the slightly slower, much safer `BKE_libblock_delete()` path here. --- source/blender/blenloader/intern/versioning_defaults.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/blender/blenloader/intern/versioning_defaults.c b/source/blender/blenloader/intern/versioning_defaults.c index e34f12b1cf9..e7e0054e7a1 100644 --- a/source/blender/blenloader/intern/versioning_defaults.c +++ b/source/blender/blenloader/intern/versioning_defaults.c @@ -242,13 +242,13 @@ void BLO_update_defaults_startup_blend(Main *bmain) /* remove polish brush (flatten/contrast does the same) */ br = (Brush *)BKE_libblock_find_name_ex(bmain, ID_BR, "Polish"); if (br) { - BKE_libblock_free(bmain, br); + BKE_libblock_delete(bmain, br); } /* remove brush brush (huh?) from some modes (draw brushes do the same) */ br = (Brush *)BKE_libblock_find_name_ex(bmain, ID_BR, "Brush"); if (br) { - BKE_libblock_free(bmain, br); + BKE_libblock_delete(bmain, br); } /* remove draw brush from texpaint (draw brushes do the same) */ From 7853ebc204238a660b57431cd86ae113ae58c3a0 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:44:15 +0200 Subject: [PATCH 4/9] Fix collada importer doing own handling of usercount/freeing. Better use generic `BKE_libblock_free_us()`. --- source/blender/collada/DocumentImporter.cpp | 8 ++------ source/blender/collada/MeshImporter.cpp | 5 +++-- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/source/blender/collada/DocumentImporter.cpp b/source/blender/collada/DocumentImporter.cpp index 226f319cefd..435eaa0208a 100644 --- a/source/blender/collada/DocumentImporter.cpp +++ b/source/blender/collada/DocumentImporter.cpp @@ -388,9 +388,7 @@ Object *DocumentImporter::create_camera_object(COLLADAFW::InstanceCamera *camera Camera *cam = uid_camera_map[cam_uid]; Camera *old_cam = (Camera *)ob->data; ob->data = cam; - id_us_min(&old_cam->id); - if (old_cam->id.us == 0) - BKE_libblock_free(G.main, old_cam); + BKE_libblock_free_us(G.main, old_cam); return ob; } @@ -406,9 +404,7 @@ Object *DocumentImporter::create_lamp_object(COLLADAFW::InstanceLight *lamp, Sce Lamp *la = uid_lamp_map[lamp_uid]; Lamp *old_lamp = (Lamp *)ob->data; ob->data = la; - id_us_min(&old_lamp->id); - if (old_lamp->id.us == 0) - BKE_libblock_free(G.main, old_lamp); + BKE_libblock_free_us(G.main, old_lamp); return ob; } diff --git a/source/blender/collada/MeshImporter.cpp b/source/blender/collada/MeshImporter.cpp index a1bfce88131..6ca53c64299 100644 --- a/source/blender/collada/MeshImporter.cpp +++ b/source/blender/collada/MeshImporter.cpp @@ -1173,8 +1173,9 @@ Object *MeshImporter::create_mesh_object(COLLADAFW::Node *node, COLLADAFW::Insta BKE_mesh_assign_object(ob, new_mesh); BKE_mesh_calc_normals(new_mesh); - if (old_mesh->id.us == 0) BKE_libblock_free(G.main, old_mesh); - + id_us_plus(&old_mesh->id); /* Because BKE_mesh_assign_object would have already decreased it... */ + BKE_libblock_free_us(G.main, old_mesh); + char layername[100]; layername[0] = '\0'; MTFace *texture_face = NULL; From 2bd51474a440aaa2f3a7a90b2e3da663f2fc1f42 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:49:40 +0200 Subject: [PATCH 5/9] Cleanup: make Group Unlink operator use BKE_libblock_delete(), since that's what it is doing. Previous code (same as what `BKE_libblock_free_us()` is doing when usercount reach 0) was probably OK in that specific case, but still not good idea, and potentially risky. --- source/blender/editors/object/object_group.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/blender/editors/object/object_group.c b/source/blender/editors/object/object_group.c index 0fe43c44d7d..568778c0a86 100644 --- a/source/blender/editors/object/object_group.c +++ b/source/blender/editors/object/object_group.c @@ -528,8 +528,7 @@ static int group_unlink_exec(bContext *C, wmOperator *UNUSED(op)) if (!group) return OPERATOR_CANCELLED; - BKE_libblock_unlink(bmain, group, false, false); - BKE_libblock_free(bmain, group); + BKE_libblock_delete(bmain, group); WM_event_add_notifier(C, NC_OBJECT | ND_DRAW, NULL); From ee5ed2ae26bd86ac843fd9220ae0e5fa721fa33f Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 12:57:08 +0200 Subject: [PATCH 6/9] Fix dangerous code when deleting Scene. That one was probably not an actual issue, except maybe in some corner cases (like deleting a linked scene also used by some other linked scene). Again, better not try to do smart & complex freeing logic outside of BKE_library area, let's keep spaghetti nitghmare in a single place! --- source/blender/editors/screen/screen_edit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/blender/editors/screen/screen_edit.c b/source/blender/editors/screen/screen_edit.c index 7d2b77028fc..acc396e42fd 100644 --- a/source/blender/editors/screen/screen_edit.c +++ b/source/blender/editors/screen/screen_edit.c @@ -1762,7 +1762,7 @@ bool ED_screen_delete_scene(bContext *C, Scene *scene) BKE_libblock_remap(bmain, scene, newscene, ID_REMAP_SKIP_INDIRECT_USAGE | ID_REMAP_SKIP_NEVER_NULL_USAGE); - BKE_libblock_free(bmain, scene); + BKE_libblock_free_us(bmain, scene); return true; } From ddb8330571ac9458ffb50191e8556aad666e40e3 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 13:00:11 +0200 Subject: [PATCH 7/9] Cleanup: remove useless call to `BKE_screen_free()` in screen deletion. `BKE_libblock_free()` will call it itself. --- source/blender/editors/screen/screen_edit.c | 1 - 1 file changed, 1 deletion(-) diff --git a/source/blender/editors/screen/screen_edit.c b/source/blender/editors/screen/screen_edit.c index acc396e42fd..18f02aff482 100644 --- a/source/blender/editors/screen/screen_edit.c +++ b/source/blender/editors/screen/screen_edit.c @@ -1926,7 +1926,6 @@ ScrArea *ED_screen_state_toggle(bContext *C, wmWindow *win, ScrArea *sa, const s ED_screen_set(C, sc); - BKE_screen_free(oldscreen); BKE_libblock_free(CTX_data_main(C), oldscreen); /* After we've restored back to SCREENNORMAL, we have to wait with From 9e0a253ea161634c03325b6fac16e09ba636fe8a Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 13:05:29 +0200 Subject: [PATCH 8/9] Cleanup: make Group Unlink outliner action use `BKE_libblock_delete() too. Same as in rB2bd51474a44... --- source/blender/editors/space_outliner/outliner_tools.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/source/blender/editors/space_outliner/outliner_tools.c b/source/blender/editors/space_outliner/outliner_tools.c index d1392b6c07e..29dcf73109c 100644 --- a/source/blender/editors/space_outliner/outliner_tools.c +++ b/source/blender/editors/space_outliner/outliner_tools.c @@ -235,8 +235,7 @@ static void unlink_group_cb( } else { Main *bmain = CTX_data_main(C); - BKE_libblock_unlink(bmain, group, false, false); - BKE_libblock_free(bmain, group); + BKE_libblock_delete(bmain, group); } } From 880e96dd667aedea17353803bcc5721f3cc34d50 Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 15 Jun 2017 15:40:24 +0200 Subject: [PATCH 9/9] Fix/workaround 'convert object' messing up linked data. 'Convert To...' Object operation has very weird effect of actually working at obdata level, not object level, which means *all* objects (even unselected/hidden/in other scenes/...) using same obdata will be converted to new selected type. IMHO this is very bad behavior, but... not a bug really, so do not change this for now. But at least, do not do that when working on some linked data, else it leaves Blend file in invalid (incoherent) state until next reload. So workaround for now is to enforce the 'Keep Original' option when some linked object/obdata is affected by the operation. Also fixed somewhat broken usercount handling in Curve->Mesh part. --- source/blender/blenkernel/intern/mesh.c | 9 +++++---- source/blender/editors/object/object_add.c | 15 ++++++++++++++- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/source/blender/blenkernel/intern/mesh.c b/source/blender/blenkernel/intern/mesh.c index 89e988782d2..171ae4485a0 100644 --- a/source/blender/blenkernel/intern/mesh.c +++ b/source/blender/blenkernel/intern/mesh.c @@ -1398,9 +1398,7 @@ void BKE_mesh_from_nurbs_displist(Object *ob, ListBase *dispbase, const bool use cu->mat = NULL; cu->totcol = 0; - if (ob->data) { - BKE_libblock_free_us(bmain, ob->data); - } + /* Do not decrement ob->data usercount here, it's done at end of func with BKE_libblock_free_us() call. */ ob->data = me; ob->type = OB_MESH; @@ -1410,11 +1408,14 @@ void BKE_mesh_from_nurbs_displist(Object *ob, ListBase *dispbase, const bool use if (ob1->data == cu) { ob1->type = OB_MESH; + id_us_min((ID *)ob1->data); ob1->data = ob->data; - id_us_plus((ID *)ob->data); + id_us_plus((ID *)ob1->data); } ob1 = ob1->id.next; } + + BKE_libblock_free_us(bmain, cu); } void BKE_mesh_from_nurbs(Object *ob) diff --git a/source/blender/editors/object/object_add.c b/source/blender/editors/object/object_add.c index b278d6e1e87..a901560079a 100644 --- a/source/blender/editors/object/object_add.c +++ b/source/blender/editors/object/object_add.c @@ -1640,7 +1640,7 @@ static int convert_exec(bContext *C, wmOperator *op) MetaBall *mb; Mesh *me; const short target = RNA_enum_get(op->ptr, "target"); - const bool keep_original = RNA_boolean_get(op->ptr, "keep_original"); + bool keep_original = RNA_boolean_get(op->ptr, "keep_original"); int a, mballConverted = 0; /* don't forget multiple users! */ @@ -1678,6 +1678,19 @@ static int convert_exec(bContext *C, wmOperator *op) { for (CollectionPointerLink *link = selected_editable_bases.first; link; link = link->next) { Base *base = link->ptr.data; + ob = base->object; + + /* The way object type conversion works currently (enforcing conversion of *all* objetcs using converted + * obdata, even some un-selected/hidden/inother scene ones, sounds totally bad to me. + * However, changing this is more design than bugfix, not to mention convoluted code below, + * so that will be for later. + * But at the very least, do not do that with linked IDs! */ + if ((ID_IS_LINKED_DATABLOCK(ob) || ID_IS_LINKED_DATABLOCK(ob->data)) && !keep_original) { + keep_original = true; + BKE_reportf(op->reports, RPT_INFO, + "Converting some linked object/object data, enforcing 'Keep Original' option to True"); + } + DAG_id_tag_update(&base->object->id, OB_RECALC_DATA); }