Closed wilhelmy closed 1 year ago
I didn't catch the stacktrace but I confirm it crashed :(
Btw: the menuitem is called Untile
This is super reproducible and I've been looking into it for a few days (I'm pretty lazy). The "bug" is in ops.c
FOR_ALL_MANAGED_BY_TILING(reg, tiling, tmp)
That for loop gets corrupted by this line:
reg2=group_do_attach(grp, ¶m, &data);
Which frees the tiling->managed_list object from underneath it and thus corrupts the pointers.
Here's a really reproducible version.
Somehow the reparenting has to be done without the rug pull here. It has been there since at least 2014. I didn't follow the full path down though. I could bisect it if we really care.
ops.c may not be the fix point, it's just the first common parent between the defect and crash.
Here's some more details:
(gdb) b ops.c:135
(gdb) c
(gdb) watch tiling->managed_list
(gdb) c
Notice how you'll get something like this:
#0 0x000055cd3c8e4e0a in free_node (ptrlist=0x55cd3dfdb2d0, node=0x55cd3e055210) at ptrlist.c:19
#1 0x000055cd3c8e53a0 in ptrlist_remove (ptrlist=0x55cd3dfdb2d0, ptr=0x55cd3df85180) at ptrlist.c:121
#2 0x00007f2656a257e1 in tiling_do_managed_remove (ws=0x55cd3dfdb210, reg=0x55cd3df85180) at tiling.c:673
#3 0x00007f2656a258f8 in tiling_managed_remove (ws=0x55cd3dfdb210, reg=0x55cd3df85180) at tiling.c:697
#4 0x000055cd3c8b33fe in region_managed_remove (mgr=0x55cd3dfdb210, reg=0x55cd3df85180) at region.c:234
#5 0x000055cd3c8b3b39 in region_detach_manager (reg=0x55cd3df85180) at region.c:576
#6 0x000055cd3c8b5641 in doit_reparent (mgr=0x55cd3e056ba0, par=0x55cd3df455a0, fp=0x7ffe1f188240, cont=0x55cd3c8ce580 <group_do_attach_final>, cont_param=0x7ffe1f1882b0, reg=0x55cd3df85180) at attach.c:86
#7 0x000055cd3c8b58f2 in region_attach_helper (mgr=0x55cd3e056ba0, par=0x55cd3df455a0, fp=0x7ffe1f188240, fn=0x55cd3c8ce580 <group_do_attach_final>, fn_param=0x7ffe1f1882b0, data=0x7ffe1f188290) at attach.c:171
#8 0x000055cd3c8ce96f in group_do_attach (ws=0x55cd3e056ba0, param=0x7ffe1f1882b0, data=0x7ffe1f188290) at group.c:729
#9 0x00007f2656a31cfb in mod_tiling_untile (tiling=0x55cd3dfdb210) at ops.c:148
That's where your memory corruption occurs. I don't know enough about the mechanics here. Maybe a simple copy of the ptr list and then a freeing at the end of the loop would do it?
back. did the git bisect. defect came in at 8d3f26280c0e3778e48ec33409c884d3f44c99d8, it's the rug pull as theorized. It's in tiling_managed_remove
right at splittree_remove
, author is @raboof .
I'll look into it more.
Ouch, sorry about that. Before https://github.com/raboof/notion/commit/8d3f26280c0e3778e48ec33409c884d3f44c99d8 that splittree_remove was already there, but perhaps triggered under different/fewer conditions. That if(!reused)
that got removed looks potentially relevant I guess...
I'll look into it more
Thanks!
it's calling ptrlist_remove(&(ws->managed_list), reg);
in tiling_do_managed_remove
which we identified from above as the issue here.
Here's a really cheap solution
a/mod_tiling/ops.c b/mod_tiling/ops.c
index 0bd21912..bc5059d9 100644
--- a/mod_tiling/ops.c
+++ b/mod_tiling/ops.c
@@ -147,13 +147,20 @@ bool mod_tiling_untile(WTiling *tiling)
reg2=group_do_attach(grp, ¶m, &data);
+ // See #334: tiling->unsplit from the context menu crashes notion
+ if(tiling->managed_list == NULL) {
+ break;
+ }
+
if(reg2==NULL)
warn(TR("Unable to move a region from tiling to group."));
}
tiling->batchop=FALSE;
- region_dispose((WRegion*)tiling);
+ if(tiling->managed_list != NULL) {
+ region_dispose((WRegion*)tiling);
+ }
return TRUE;
}
we can probably do better than that
I'm also open to just using this and then opening up a more minor memory leak bug which I'm sure is sitting around somewhere in this hack and then just move on.
I merged #357 - do you want to keep this issue open to keep looking for a clearer solution or rather close it?
no ... let me handle that
apparently only you have the privilege of closing. Feel free to do that now that we have documented and opened up the more major and now less visible issue.
Edit: can anyone reproduce?