padgettr / rfm

Simple file manager for dwm
Other
11 stars 2 forks source link

segfault in my code #5

Closed guyuming76 closed 1 year ago

guyuming76 commented 1 year ago

I make some enhancement to the program recently:

https://github.com/guyuming76/rfm/tree/Segfault

Starting from reading stdin via pipeline for file name lists output by find or locate.

And then add TreeView widget(actually just used as flat list view) showing content like "ls -l"

both IconView and TreeView use the same 'store' variable as underlying data source.

Now, i am trying to switch between the pipe line stdin datasource and the current working directory datasource. It works to some extent. But i got segfault if i switch the datasource(by clicking the (CurrentPath/StdIn toolbar button for the Switch_CurPath_StdIn method) AFTER switched the List/Icon view(the Switch_View method in code).

Can it be some objects that i should free when switching view or datasource?

padgettr commented 1 year ago

Hi guyuming,

I think the issue is that in switch_view() you use gtk_container_remove(), then later use add_view(). add_view() creates all of the widgets again (including sw), but these already exist. A simple solution would be to destroy everything before calling add_view() by using gtk_widget_destroy(sw) - this will destroy all widgets under sw, and add_view() will recreate them again once called. Probably not the most efficient method, you may want to consider how you reuse the widgets in future, but this worked without segfault for me (Note changes required to both functions):

static void switch_view(GtkToolItem *item, RFM_ctx *rfmCtx) {
  GList *  selectionList=get_view_selection_list(icon_or_tree_view,treeview,&treemodel);
  gtk_widget_hide(rfm_main_box);

gtk_widget_destroy(sw);
  treeview=!treeview;
  icon_or_tree_view=add_view(rfmCtx);
  gtk_widget_show_all(window);
  set_view_selection_list(icon_or_tree_view, treeview, selectionList);
}
static void Switch_CurPath_StdIn(GtkToolItem *item, RFM_ctx *rfmCtx)
{
  if (PictureFullNamesFromStdin != NULL && rfm_curPath!= NULL) {
    inotify_rm_watch(rfm_inotify_fd, rfm_curPath_wd);
    gtk_widget_hide(rfm_main_box);

    rfm_stop_all(rfmCtx);
    clear_store(); 

gtk_widget_destroy(sw);
    gtk_window_remove_accel_group(GTK_WINDOW(window), agMain);
    gtk_container_remove(GTK_CONTAINER(rfm_main_box), tool_bar);

    readFromPipe=!readFromPipe;

    add_toolbar(rfm_main_box, defaultPixbufs, rfmCtx);
    icon_or_tree_view=add_view(rfmCtx); 
    set_rfm_curPath(rfm_curPath); // this is to update rfm_new_wd
    fill_store(rfmCtx);
    gtk_widget_show_all(window);
  }
}
guyuming76 commented 1 year ago

thanks for replying!

I tried your suggestion, and the segfault disappears.

The screenshot shows that the segfault was thrown inside the first clear_store call in Switch_CurPath_Stdin, before any widget-remove calling in Switch_CurPath_Stdin, but after previous widgets-remove call from previous Switch_View call.

https://gitee.com/guyuming76/rfm/commit/d6dcf3128e30fc87a65045ddfb1139c24262851f

https://gitee.com/guyuming76/rfm/commit/7edb293b2fee02b072e06fd872eb9d91a18c621e

Does it means that when do clear on store that "bounds" to a view which was removed without destroying and recreated, when the delete row operation from clear_store tries to use its reference to the removed view?

And destroying a view will remove the store's reference to the view?

You can see that in my commented code in switch_view, i tried destroy before, but unfortunately on the view first instead of on sw first. And after reading https://docs.gtk.org/gtk3/method.Widget.destroy.html and see the talking about "inert" state, i give up destroy.

The difference may be store can keep reference to a view because they are "bound", but store won't keep reference on sw.

Anyway, this incident give me the chance to learn more about gtk. For rfm, i don't think the idea of Switch_curPath_StdIn so valuable now Click your icon on the toolbar to open a new instance of rfm from current directory is more "suckless"

guyuming76 commented 1 year ago

I just find that with Switch_View function, which only recreate the view without clear_store, if i manually click the Refresh button after Switch_View, i get this same segfault sometimes.

The important thing is SOMETIMEs, i means, sometimes it works fine.

If it keeps giving out segfault, i just add valgrind before rfm , most probably, it will work fine again, only that the performance is slowed down. I cannot explain this if my speculation in last comment was correct.

locate 202304|grep png|valgrind --tool=memcheck rfm -p

padgettr commented 1 year ago

Yes I think clear_store is not the root cause of the problem, it is most likely that old references are not being freed (I mean reference count to zero) before recreating objects. In the original code the view was never destroyed until quit. For example, when sw was destroyed references count for all child objects would be decreased. As we don't take any additional references, this would go to zero and all child objects are released.

I think the current problem might be store references pointing to old rows - each time the store changes any old references must be cleared and re established. Have you considered getting rid of icon view and just using treeview? In another project I wrote a similar file manager but for dicom (medical imaging) objects, and I found tree view to be much more flexible than icon view, and it renders much quicker too. I had considered swapping iconview for treeview in rfm, but don't get the time at the moment!

I'm away at the moment and can't really look at the code, but have a good look at any store references and make sure you unref or destroy before recreating, I'm pretty sure it will be related to that.

Sent from Outlook for Androidhttps://aka.ms/AAb9ysg


From: guyuming @.> Sent: Thursday, 13 April 2023, 06:52 To: padgettr/rfm @.> Cc: Dr Rodney Padgett @.>; Comment @.> Subject: Re: [padgettr/rfm] segfault in my code (Issue #5)

I just find that with Switch_View function, which only recreate the view without clear_store, if i manually click the Refresh button after Switch_View, i get this same segfault sometimes.

The important thing is SOMETIMEs, i means, sometimes it works fine.

If it keeps giving out segfault, i just add valgrind before rfm , most probably, it will work fine again, only that the performance is slowed down. I cannot explain this if my speculation in last comment was correct.

locate 202304|grep png|valgrind --tool=memcheck rfm -p

— Reply to this email directly, view it on GitHubhttps://github.com/padgettr/rfm/issues/5#issuecomment-1506383947, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB25FQ5FP4K5CF3GLRKV3O3XA6IALANCNFSM6AAAAAAW3LXN6I. You are receiving this because you commented.Message ID: @.***>

guyuming76 commented 1 year ago

@padgettr . I have not thought about getting ride of icon view with treeview.

I think the icon view is great! I only add switching to list in case i need other file attributes to help me make multiple file selection. I can switch between icon view and list view without losing selection.

i have not tried rfm with medical imaging system yet, although i had 3D Slicer installed maybe two years ago. But i seldom opens 3D slicer.

I begin to make enhancement to rfm to read from locate command output via pipeline, so that i can view my collection of plant pictures and mineral collection pictures more conveniently.

And i am thinking of having school kids to manage pictures such as screenshots of homework with rfm and using git to save homework.

The further enhancement i am planning in short term is adding some Git related information in listview, such as whether the files is tracked in git and whether they have been modified, so that i can multi-select files to do "git stage". I only plan to do "git stage" currently from rfm, won't need to run git commit or git log from rfm yet.

padgettr commented 1 year ago

Sounds like a good idea, hope it goes OK!

I was thinking about the segfaults, does it only happen when thumbnails are shown? Assuming everything is being freed correctly, I'm thinking the segfault might be due to the thumbnail list getting out of sync with the display when the view is switched. There is a list which keeps track of the store model path so that once the thumbnail is created, inotify callback can then check which thumbnail it got and update the appropriate icon in the view. But if those paths change between switching view, it may cause the segfault as the paths only update on a dir read. Best way to check would be to disable thumbnailing, or edit inotify callback to simply re-read the dir instead of just updating icons. Hope that makes sense, cheers, Rod.

Sent from Outlook for Androidhttps://aka.ms/AAb9ysg


From: guyuming @.> Sent: Saturday, April 15, 2023 5:23:38 AM To: padgettr/rfm @.> Cc: Dr Rodney Padgett @.>; Mention @.> Subject: Re: [padgettr/rfm] segfault in my code (Issue #5)

@padgettrhttps://github.com/padgettr . I have not thought about getting ride of icon view with treeview.

I think the icon view is great! I only add switching to list in case i need other file attributes to help me make multiple file selection. I can switch between icon view and list view without losing selection.

i have not tried rfm with medical imaging system yet, although i had 3D Slicer installed maybe two years ago. But i seldom opens 3D slicer.

I begin to make enhancement to rfm to read from locate command output via pipeline, so that i can view my collection of plant pictures and mineral collection pictures more conveniently.

And i am thinking of having school kids to manage pictures such as screenshots of homework with rfm and using git to save homework.

The further enhancement i am planning in short term is adding some Git related information in listview, such as whether the files is tracked in git and whether they have been modified, so that i can multi-select files to do "git stage". I only plan to do "git stage" currently from rfm, won't need to run git commit or git log from rfm yet.

— Reply to this email directly, view it on GitHubhttps://github.com/padgettr/rfm/issues/5#issuecomment-1509496676, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB25FQ5V4HAG6W3JRGRFU3TXBIPEVANCNFSM6AAAAAAW3LXN6I. You are receiving this because you were mentioned.Message ID: @.***>

guyuming76 commented 1 year ago

@padgettr , "a list which keeps track of the store model path", do you mean the thumb_hash ghashtable? i see all its content cleared before the store is cleared and refilled.

And my switch_view implementation does not change the lifecycle of store and thumb_hash. it only add some columns into the store and change the view. When switching view, store is not cleared and refilled.

padgettr commented 1 year ago

Yes, the hash table. But as long as you don't change the store, the model references should be valid and the problem must be elsewhere. I wonder if changing view could cause the model to change, e.g. if sort order changes - I don't know, will have a think about it!

Sent from Outlook for Androidhttps://aka.ms/AAb9ysg


From: guyuming @.> Sent: Wednesday, May 3, 2023 5:49:35 AM To: padgettr/rfm @.> Cc: Dr Rodney Padgett @.>; Mention @.> Subject: Re: [padgettr/rfm] segfault in my code (Issue #5)

@padgettrhttps://github.com/padgettr , "a list which keeps track of the store model path", do you mean the thumb_hash ghashtable? i see all its content cleared before the store is cleared and refilled.

And my switch_view implementation does not change the lifecycle of store and thumb_hash. it only add some columns into the store and change the view. When switching view, store is not cleared and refilled.

— Reply to this email directly, view it on GitHubhttps://github.com/padgettr/rfm/issues/5#issuecomment-1532450235, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AB25FQ6K74SW42LMTZE2VADXEHPV7ANCNFSM6AAAAAAW3LXN6I. You are receiving this because you were mentioned.Message ID: @.***>