search

mity / mctrl

C library providing set of additional user interface controls for Windows, intended to be complementary to standard Win32API controls from USER32.DLL and COMCTL32.DLL.
http://mctrl.org
235 stars 52 forks source link

Treelist crash still occurs during delete #8

Closed ArmstrongJ closed 11 years ago

ArmstrongJ commented 11 years ago

After a pull, update, and rebuild I'm still seeing crashes similar to what was reported in Issue #7 . It appears that tl->scrolled_item can still retain a pointer after a delete. Adding a

if(tl->scrolled_item == item)
    tl->scrolled_item = NULL;

in src/treelist.c in the function treelist_delete_item() seems to fix the problem again, but I'm not sure that's the fix you're looking for.

The error is a bit difficult for me to pinpoint namely because I am calling an mCtrl.dll built using GCC from an executable compiled via OpenWatcom. Their debugger formats don't seem to be cooperating (even though they're both set to DWARF), making finding the exact crash difficult. However, the call sequence leading to the crash is:

treelist_delete_item treelist_do_select treelist_invalidate_item treelist_get_item_y item_next_displayed item_next_displayed_ex

mity commented 11 years ago

By chance, aren't you trying to send the delete message from other thread then where the window lives (its win. proc is running?)

Can you provide a code reproducing it?

If not, can you provide some info about how the tree topology looks, which of the items is scrolled (i.e. 1st visible) and which one you are deleting?

Having debug log would be helpful too. Rebuildi with make CPPFLAGS="-DTREELIST_DEBUG=1 -DDEBUG=1" and reproduce while catching the debug traces (OutputDebugString()). For example debugview.exe from SysInternals can do that (http://technet.microsoft.com/en-us/sysinternals/bb896647), and then post the log here.

mity commented 11 years ago

I (locally) changed the examples/ex_treelist.c to delete right-clicked items (added some new notifications into the control, so at least it brought something good) and tried to reproduce with no luck.

But it helped me to realize how much broken the treelist_delete_item() was anyway fro many points of view so some fixes followed. There were also fixes of ->scrolled_item usage in other places. I cannot verify if it also helps in your case. So please test and let me know.

ArmstrongJ commented 11 years ago

I added a simple delete loop to examples/ex_treelist.c (without updating, just to see if I could replicate). The code was:

static void DeleteItems()
{
MC_HTREELISTITEM item;
int i;

    i = 0;

    item = (MC_HTREELISTITEM)SendMessage(hwndTreeList, MC_TLM_GETNEXTITEM, (WPARAM) MC_TLGN_ROOT , (LPARAM)NULL);
    while(item != NULL) {
        i++; 

        SendMessage(hwndTreeList, MC_TLM_DELETEITEM, (WPARAM)0, (LPARAM)item);

        item = (MC_HTREELISTITEM)SendMessage(hwndTreeList, MC_TLM_GETNEXTITEM, (WPARAM) MC_TLGN_ROOT , (LPARAM)NULL);
    }

}

I triggered the method on right-click, and I'm seeing a crash on deleting the first item (i == 1). The item isn't NULL or anything.

I'll try a pull with your changes and try again.

ArmstrongJ commented 11 years ago

After pulling the code, I'm still seeing the crash. The only other change I made to the example in addition to my last comment is that I flattened the list so that the Sun was not every planet's parent. Log is:

00000024    62.97957993 [253648] ****************************************************************************** 
00000025    62.98078156 [253648] DllMain(DLL_PROCESS_ATTACH): MCTRL.DLL version 0.9.3-dbg(1) (32 bit)   
00000026    62.98142242 [253648] setup_win_version: Detected Windows 7 (2.6.1, service pack 1.0, build 7601)    
00000027    62.98431778 [253648] setup_comctl32_version: Detected COMCTL32.DLL 6.16 (build 7601)    
00000028    63.01206589 [253648] theme_init_module: IsThemeActive() -> yes  
00000029    63.01226807 [253648] theme_init_module: IsAppThemed() -> yes    
00000030    63.01239014 [253648] theme_init_module: GetThemeAppProperties() -> 0x3 (nonclient, controls)    
00000031    63.01394653 [253648] theme_init_module: IsCompositionActive() -> no 
00000032    63.01802063 [253648] treelist_notify_format: Will use ANSI notifications.   
00000033    63.06684113 [253648] treelist_insert_column(006629C8, 0, 0028F834, 0)   
00000034    63.07402802 [253648] treelist_insert_column(006629C8, 1, 0028F834, 0)   
00000035    63.07475281 [253648] treelist_insert_column(006629C8, 2, 0028F834, 0)   
00000036    63.07492447 [253648] treelist_insert_column(006629C8, 3, 0028F834, 0)   
00000037    63.07541656 [253648] treelist_insert_item(006629C8, 0028F7FC, 0)    
00000038    63.07561493 [253648] treelist_set_subitem(006629C8, 00662A20, 0028F77C, 0)  
00000039    63.07569504 [253648] treelist_set_subitem(006629C8, 00662A20, 0028F77C, 0)  
00000040    63.07576752 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000041    63.07589722 [253648] treelist_set_subitem(006629C8, 00662AD0, 0028F77C, 0)  
00000042    63.07610703 [253648] treelist_set_subitem(006629C8, 00662AD0, 0028F77C, 0)  
00000043    63.07617188 [253648] treelist_set_subitem(006629C8, 00662AD0, 0028F77C, 0)  
00000044    63.07626343 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000045    63.07648468 [253648] treelist_set_subitem(006629C8, 00662BA0, 0028F77C, 0)  
00000046    63.07656479 [253648] treelist_set_subitem(006629C8, 00662BA0, 0028F77C, 0)  
00000047    63.07663345 [253648] treelist_set_subitem(006629C8, 00662BA0, 0028F77C, 0)  
00000048    63.07670212 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000049    63.07682037 [253648] treelist_set_subitem(006629C8, 00662C78, 0028F77C, 0)  
00000050    63.07701492 [253648] treelist_set_subitem(006629C8, 00662C78, 0028F77C, 0)  
00000051    63.07711792 [253648] treelist_set_subitem(006629C8, 00662C78, 0028F77C, 0)  
00000052    63.07718277 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000053    63.07725525 [253648] treelist_set_subitem(006629C8, 00662D50, 0028F77C, 0)  
00000054    63.07731628 [253648] treelist_set_subitem(006629C8, 00662D50, 0028F77C, 0)  
00000055    63.07738495 [253648] treelist_set_subitem(006629C8, 00662D50, 0028F77C, 0)  
00000056    63.07744598 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000057    63.07760620 [253648] treelist_set_subitem(006629C8, 00662E20, 0028F77C, 0)  
00000058    63.07767868 [253648] treelist_set_subitem(006629C8, 00662E20, 0028F77C, 0)  
00000059    63.07774734 [253648] treelist_set_subitem(006629C8, 00662E20, 0028F77C, 0)  
00000060    63.07780838 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000061    63.07788849 [253648] treelist_set_subitem(006629C8, 00662EF8, 0028F77C, 0)  
00000062    63.07795715 [253648] treelist_set_subitem(006629C8, 00662EF8, 0028F77C, 0)  
00000063    63.07802200 [253648] treelist_set_subitem(006629C8, 00662EF8, 0028F77C, 0)  
00000064    63.07808685 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000065    63.07816696 [253648] treelist_set_subitem(006629C8, 00662FB8, 0028F77C, 0)  
00000066    63.07840729 [253648] treelist_set_subitem(006629C8, 00662FB8, 0028F77C, 0)  
00000067    63.07847595 [253648] treelist_set_subitem(006629C8, 00662FB8, 0028F77C, 0)  
00000068    63.07854080 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000069    63.07873535 [253648] treelist_set_subitem(006629C8, 00666DA0, 0028F77C, 0)  
00000070    63.07880402 [253648] treelist_set_subitem(006629C8, 00666DA0, 0028F77C, 0)  
00000071    63.07889557 [253648] treelist_set_subitem(006629C8, 00666DA0, 0028F77C, 0)  
00000072    63.07895279 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000073    63.07902145 [253648] treelist_set_subitem(006629C8, 00667648, 0028F77C, 0)  
00000074    63.07908249 [253648] treelist_set_subitem(006629C8, 00667648, 0028F77C, 0)  
00000075    63.07915115 [253648] treelist_set_subitem(006629C8, 00667648, 0028F77C, 0)  
00000076    63.07921219 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000077    63.07928085 [253648] treelist_set_subitem(006629C8, 006676F8, 0028F77C, 0)  
00000078    63.07934189 [253648] treelist_set_subitem(006629C8, 006676F8, 0028F77C, 0)  
00000079    63.07939911 [253648] treelist_set_subitem(006629C8, 006676F8, 0028F77C, 0)  
00000080    63.07946396 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000081    63.07952499 [253648] treelist_set_subitem(006629C8, 00667798, 0028F77C, 0)  
00000082    63.07958984 [253648] treelist_set_subitem(006629C8, 00667798, 0028F77C, 0)  
00000083    63.07965088 [253648] treelist_set_subitem(006629C8, 00667798, 0028F77C, 0)  
00000084    63.07971191 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000085    63.07978439 [253648] treelist_set_subitem(006629C8, 00667858, 0028F77C, 0)  
00000086    63.08000946 [253648] treelist_set_subitem(006629C8, 00667858, 0028F77C, 0)  
00000087    63.08008575 [253648] treelist_set_subitem(006629C8, 00667858, 0028F77C, 0)  
00000088    63.08013916 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000089    63.08038330 [253648] treelist_set_subitem(006629C8, 00667890, 0028F77C, 0)  
00000090    63.08045959 [253648] treelist_set_subitem(006629C8, 00667890, 0028F77C, 0)  
00000091    63.08052826 [253648] treelist_set_subitem(006629C8, 00667890, 0028F77C, 0)  
00000092    63.08059311 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000093    63.08066177 [253648] treelist_set_subitem(006629C8, 006678C8, 0028F77C, 0)  
00000094    63.08072662 [253648] treelist_set_subitem(006629C8, 006678C8, 0028F77C, 0)  
00000095    63.08079910 [253648] treelist_set_subitem(006629C8, 006678C8, 0028F77C, 0)  
00000096    63.08086014 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000097    63.08092499 [253648] treelist_set_subitem(006629C8, 00667900, 0028F77C, 0)  
00000098    63.08098602 [253648] treelist_set_subitem(006629C8, 00667900, 0028F77C, 0)  
00000099    63.08105087 [253648] treelist_set_subitem(006629C8, 00667900, 0028F77C, 0)  
00000100    63.08111191 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000101    63.08117294 [253648] treelist_set_subitem(006629C8, 00667938, 0028F77C, 0)  
00000102    63.08124161 [253648] treelist_set_subitem(006629C8, 00667938, 0028F77C, 0)  
00000103    63.08130264 [253648] treelist_set_subitem(006629C8, 00667938, 0028F77C, 0)  
00000104    63.08136749 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000105    63.08142853 [253648] treelist_set_subitem(006629C8, 021517F0, 0028F77C, 0)  
00000106    63.08149719 [253648] treelist_set_subitem(006629C8, 021517F0, 0028F77C, 0)  
00000107    63.08155823 [253648] treelist_set_subitem(006629C8, 021517F0, 0028F77C, 0)  
00000108    63.08162308 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000109    63.08168030 [253648] treelist_set_subitem(006629C8, 02151828, 0028F77C, 0)  
00000110    63.08174515 [253648] treelist_set_subitem(006629C8, 02151828, 0028F77C, 0)  
00000111    63.08202744 [253648] treelist_set_subitem(006629C8, 02151828, 0028F77C, 0)  
00000112    63.08213043 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000113    63.08219910 [253648] treelist_set_subitem(006629C8, 02151860, 0028F77C, 0)  
00000114    63.08226013 [253648] treelist_set_subitem(006629C8, 02151860, 0028F77C, 0)  
00000115    63.08232498 [253648] treelist_set_subitem(006629C8, 02151860, 0028F77C, 0)  
00000116    63.08238602 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000117    63.08244324 [253648] treelist_set_subitem(006629C8, 02151898, 0028F77C, 0)  
00000118    63.08250809 [253648] treelist_set_subitem(006629C8, 02151898, 0028F77C, 0)  
00000119    63.08256531 [253648] treelist_set_subitem(006629C8, 02151898, 0028F77C, 0)  
00000120    63.08262634 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000121    63.08282471 [253648] treelist_set_subitem(006629C8, 021518D0, 0028F77C, 0)  
00000122    63.08289719 [253648] treelist_set_subitem(006629C8, 021518D0, 0028F77C, 0)  
00000123    63.08296204 [253648] treelist_set_subitem(006629C8, 021518D0, 0028F77C, 0)  
00000124    63.08303070 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000125    63.08309174 [253648] treelist_set_subitem(006629C8, 02151908, 0028F77C, 0)  
00000126    63.08316040 [253648] treelist_set_subitem(006629C8, 02151908, 0028F77C, 0)  
00000127    63.08321762 [253648] treelist_set_subitem(006629C8, 02151908, 0028F77C, 0)  
00000128    63.08327866 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000129    63.08334351 [253648] treelist_set_subitem(006629C8, 02151940, 0028F77C, 0)  
00000130    63.08340454 [253648] treelist_set_subitem(006629C8, 02151940, 0028F77C, 0)  
00000131    63.08346939 [253648] treelist_set_subitem(006629C8, 02151940, 0028F77C, 0)  
00000132    63.08352661 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000133    63.08358765 [253648] treelist_set_subitem(006629C8, 02151978, 0028F77C, 0)  
00000134    63.08365250 [253648] treelist_set_subitem(006629C8, 02151978, 0028F77C, 0)  
00000135    63.08375549 [253648] treelist_set_subitem(006629C8, 02151978, 0028F77C, 0)  
00000136    63.08383179 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000137    63.08389282 [253648] treelist_set_subitem(006629C8, 021519B0, 0028F77C, 0)  
00000138    63.08395004 [253648] treelist_set_subitem(006629C8, 021519B0, 0028F77C, 0)  
00000139    63.08401489 [253648] treelist_set_subitem(006629C8, 021519B0, 0028F77C, 0)  
00000140    63.08407593 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000141    63.08413696 [253648] treelist_set_subitem(006629C8, 021519E8, 0028F77C, 0)  
00000142    63.08420181 [253648] treelist_set_subitem(006629C8, 021519E8, 0028F77C, 0)  
00000143    63.08426285 [253648] treelist_set_subitem(006629C8, 021519E8, 0028F77C, 0)  
00000144    63.08432007 [253648] treelist_insert_item(006629C8, 0028F7C4, 0)    
00000145    63.08446503 [253648] treelist_set_subitem(006629C8, 02151A20, 0028F77C, 0)  
00000146    63.08453751 [253648] treelist_set_subitem(006629C8, 02151A20, 0028F77C, 0)  
00000147    63.08459854 [253648] treelist_set_subitem(006629C8, 02151A20, 0028F77C, 0)  
00000148    63.08465958 [253648] treelist_insert_item(006629C8, 0028F78C, 0)    
00000149    63.08474731 [253648] treelist_set_subitem(006629C8, 02151A58, 0028F77C, 0)  
00000150    63.08480835 [253648] treelist_set_subitem(006629C8, 02151A58, 0028F77C, 0)  
00000151    63.08487320 [253648] treelist_set_subitem(006629C8, 02151A58, 0028F77C, 0)  
00000152    63.16356277 [253648] treelist_do_select([NULL] -> Sun)  
00000153    67.71814728 [253648] treelist_delete_item(006629C8, 00662A20)   
00000154    67.71989441 [253648] treelist_do_select(Sun -> Mercury)
ArmstrongJ commented 11 years ago

Based on the log above, it looks like a delete occurs, but the selection is updated afterwards. Checking the code, the deleted item is freed, but the selection is then updated after freeing the memory (src/treelist.c:2522), which seems like it is too late.

EDIT: Looking at the code, it appears the selected item should be reset (src/treelist.c:2414), so I'm not sure what's happening.

ArmstrongJ commented 11 years ago

Little more information: after treelist_delete_item_helper is called, tl->scrolled_item still refers to the now-deleted-and-freed item. Setting it to NULL after the call to treelist_delete_item_helper again fixes the issue.

mity commented 11 years ago

Please reread this comment

According to the line numbers you refer you haven't pull the latest fixes.

mity commented 11 years ago

Especially commits 9eeb222511eacc9f065706bbbc609486105d5319 and 9a5173a7b2baa81b2b363e4593436abb6c5a862f are important.

mity commented 11 years ago

Opps, sorry copied wrong SHAs: 9a5173a7b2baa81b2b363e4593436abb6c5a862f and 9a5173a7b2baa81b2b363e4593436abb6c5a862f are the right ones.

ArmstrongJ commented 11 years ago

With the pull, I'm still getting the crash with my example, but the issue (with some additional trace statements) seems to be that parent can be NULL by the time we reach src/treelist.c:2578. If you just drop a check in for parent != NULL, the last problem should be fixed.

ArmstrongJ commented 11 years ago

I also wanted to point out that the redraws seem to be working much better with your changes after deleting items as opposed to when I arbitrarily set tl->scrolled_item to NULL. Very nice!

mity commented 11 years ago

Yep, overlooked this one. Should be fixed right now in af121db38870be4df0a8044ec181363f9ad62ca9. Thanks for all the time you spent on this.

If you feel satisfied now (or after some testing on your side), feel free to close.

ArmstrongJ commented 11 years ago

I should be able to check this evening. Thanks for all your effort too!

ArmstrongJ commented 11 years ago

With that change, the example now works fine for me, and the resultant DLL is working great with my main project. Nicely done, and thanks for your patience! Cheers!