STR #1306: automated safe widget deletion inside the library

STR #1306

Application:	FLTK Library
Status:	1 - Closed w/Resolution
Priority:	3 - Moderate, e.g. unable to compile the software
Scope:	3 - Applies to all machines and operating systems
Subsystem:	Core Library
Summary:	automated safe widget deletion inside the library
Version:	1.3-current
Created By:	AlbrechtS
Assigned To:	AlbrechtS
Fix Version:	1.3-current (SVN: v7115)
Update Notification:	Receive EMails Don't Receive EMails

Trouble Report Files:

No files

Trouble Report Comments:

#1	AlbrechtS 07:23 Jun 02, 2006

If Fl::check() is called from a callback _after_ one or more call(s) to Fl::delete_widget(), then the scheduled widgets will be deleted immediately. Fl::check() calls Fl::wait(0.0), which in turn calls do_widget_deletion(). IMHO, this is not intended, and this is a bug. Proposal: Fl::check() shouldn't call Fl::wait(0.0), or Fl::wait() should not call do_widget_deletion(), if time_to_wait equals 0 (zero). Albrecht

#4	matt 12:37 Apr 22, 2008

Um yes, but what *was* my solution to this problem at that time?

#5	AlbrechtS 02:52 Apr 23, 2008

Probably STR #1162: http://www.fltk.org/str.php?L1162 svn log -v -r5037 ------------------------------------------------------------------------ r5037 | matt | 2006-04-27 23:40:47 +0200 (Do, 27 Apr 2006) | 1 line Geänderte Pfade: M /branches/branch-1.1/CHANGES M /branches/branch-1.1/FL/Fl.H M /branches/branch-1.1/src/Fl.cxx M /branches/branch-1.1/src/Fl_Menu_Button.cxx M /branches/branch-1.1/src/Fl_Widget.cxx STR #1162: Fl_Menu_Button::popup was trying to access a previously deleted widget (itself). The delayed deleting mechanism in 'Fl::delete_widget' did not work in this case because the main loop is called before the callback returns. The fix implements a type of automatic pointer that will be cleared to NULL should the widget get deleted. This may not be a 'nice' solution, but it does fix the problem reliably. We could actually use this for all widget pointers and remove the delayed delete mechanism alltogether ------------------------------------------------------------------------ Though I suspect that this would not be a good general solution. As we know from some requests, there are applications with several hundred or even thousands of widgets (this is one reason for extending the widget coordinates from short to int). And, honestly said (and no offense intended), I can't see anything useful in the modifications in svn -r 5037 - maybe I'm missing an important point. Everything I can see is that a pointer to a widget would be entered into an array of pointers and removed again in the widget destructor. I can't find any reference to "widget_watch" other than those in Fl.cxx in Fl::watch_widget_pointer(), Fl::release_widget_pointer(), and Fl::clear_widget_pointer(). What am I missing here? P.S.: I'm _really_ interested in _safe_ widget deletion, because my main application heavily uses widget creation and deletion.

#6	AlbrechtS 03:31 Apr 23, 2008

Okay, I think I got it :-) It's the widget pointer that gets reset to NULL when the widget is deleted, and this is used in Fl_Menu_Button.cxx to prevent access to a deleted widget by testing the pointer in (about) line 60: if (mb) mb->redraw(); That _could_ indeed be a way to assure that a widget has not been deleted in a callback, _if_ you intend to access the widget _after_ the callback. Now it becomes a bit off topic, but it is still related. Since FLTK 1.1.5-rc2, there has been a modification to reset the changed() flag of widgets _after_ the callback. This would access deleted widgets, if they are deleted in the callback, and for the same reasons as above, Fl::delete_widget() can't help to solve this. This is implemented in FL/Fl_Widget.H (yes, inline in the header file): void do_callback() with different arguments. The solution could be something like: void do_callback(Fl_Widget* o,void* arg=0) { Fl_Widget *wp = this; Fl::watch_widget_pointer(wp); callback_(o,arg); if (wp && callback_ != default_callback) clear_changed(); Fl::release_widget_pointer(wp); } Would this be a way to go? -------------------------- OTOH: I'm not sure that clearing the changed() flag after the callback _is_ the correct/best way to implement the widget-has-changed-question. I would prefer to do it so that the callback has to clear the flag, if it is interested in tracking the changed() state of a widget. This would also allow the user to let the changed() flag as it is over more than one callback, and checking it later, when appropriate. Suggestion: change void clear_changed(); to int clear_changed(); and return the previous changed() value. Sorry for the long post and the deviation from the original topic, but maybe this could be used to solve a problem that I'm fighting with for a very long time now.

#7	matt 03:06 Apr 24, 2008

I remember that Bill was very concerned about accessing widgets after calling the callback and I think he was right. He put a great effort in the original code to avoid doing just that. Changing the behavior of "changed()" is logical, but would likely confuse users to insanity. Maybe the smart widget pointer as implemented solves the entire problem for free. What do you think? Should we put that in wherever appropriate?

#8	AlbrechtS 16:39 Apr 24, 2008

Yes, IMHO Bill was more than right. I know for sure that my main FLTK application started to crash sporadically after the widget-changed modification between 1.1.5-rc1 and 1.1.5-rc2 on Jul 27, 2004, see STR #475: http://www.fltk.org/str.php?L475 and svn log -r 3713, svn diff -c 3713 FL/Fl_Widget.H: =================================================================== --- FL/Fl_Widget.H (revision 3712) +++ FL/Fl_Widget.H (revision 3713) @@ -1,5 +1,5 @@ // -// "$Id: Fl_Widget.H,v 1.6.2.4.2.23 2004/04/11 04:38:54 easysw Exp $" +// "$Id: Fl_Widget.H,v 1.6.2.4.2.24 2004/07/27 16:02:18 easysw Exp $" // // Widget header file for the Fast Light Tool Kit (FLTK). // @@ -185,9 +185,9 @@ int visible_focus() { return flags_ & VISIBLE_FOCUS; } static void default_callback(Fl_Widget*, void*); - void do_callback() {callback_(this,user_data_);} - void do_callback(Fl_Widget* o,void* arg=0) {callback_(o,arg);} - void do_callback(Fl_Widget* o,long arg) {callback_(o,(void*)arg);} + void do_callback() {callback_(this,user_data_); if (callback_ != default_callback) clear_changed();} + void do_callback(Fl_Widget* o,void* arg=0) {callback_(o,arg); if (callback_ != default_callback) clear_changed();} + void do_callback(Fl_Widget* o,long arg) {callback_(o,(void*)arg); if (callback_ != default_callback) clear_changed();} int test_shortcut(); static int test_shortcut(const char*); int contains(const Fl_Widget*) const ; @@ -217,5 +217,5 @@ #endif // -// End of "$Id: Fl_Widget.H,v 1.6.2.4.2.23 2004/04/11 04:38:54 easysw Exp $". +// End of "$Id: Fl_Widget.H,v 1.6.2.4.2.24 2004/07/27 16:02:18 easysw Exp $". // IMHO, there was no need to clear the changed() flag after the callback at all. However, consistently setting the changed() flag was a good thing, although I don't use it myself. Back to the question: My first preference would be to remove the clear_changed() from Fl_Widget.H . AFAIK, it is not even documented clearly that the flag would be cleared _after_ the callback. http://www.fltk.org/doc-1.3/Fl_Widget.html#Fl_Widget.changed : "[...] Most widgets turn this flag off when they do the callback, and when the program sets the stored value." This seems to be an old text, and doesn't clearly say if the flag would be cleared before or after the callback. IMHO, it should be documented that all the FLTK core widgets set the changed flag, when the value is changed, but that the user program is responsible for clearing the changed() flag in the callback or whenever it uses the changed() value. To support this, I proposed the change to clear_changed() above, that it would return the previous changed() flag, thus it would be easy to do in the callback: int changed = clear_changed(); // use changed laster ... or if (clear_changed()) {... widget has been changed ...} If, and only if, this wouldn't be possible for any reason, _then_ I would say that the smart widget pointers should be used like I proposed above. Accessing objects after deletion _must_ be avoided strictly, because it can and will crash a program eventually with a segfault, if the relevant portion of memory has been returned to the system after it has been freed. I demonstrated this in STR #1894 with my crash example test: http://www.fltk.org/str.php?L1894 Using smart pointers is only healing symptoms, but not avoiding the problem at all, just as Fl::delete_widget() did reduce the problem, but could not prevent it completely from happening, because of nested Fl::wait() and Fl::check() calls, and/or fl_message(), fl_ask() et al. BTW: I'm still working on the patch for the problem in STR 1894, and I'm making progress :-)

#9	AlbrechtS 23:41 Apr 24, 2008

I do also think that it is _wrong_ to reset the changed() flag after the callback, because calling the callback doesn't mean that the user (code/program/callback implementation) uses the changed() flag in every code path in the callback. I can imagine situations, where I would want to use an indicator, if the widget value has changed since it has been set by my program, and not since the last callback. The FLTK callback mechanism, as implemented in Fl_Widget.H doesn't know what I intend to do in the callback. Maybe I want to aggregate all changes to more than one widget in a panel, and use the information later, when the okay button would be clicked, or something else. The OP's intent was to detect why a callback was called. He wrote in http://www.fltk.org/newsgroups.php?gfltk.general+v:9777 "If this is supposed to be for every widget it's not working as Fl_Input works the way I expected it: call my callback before clearing the flag so I can know if the callback was called due to a user changing a value or because the focus was lost (of course with a when=FL_WHEN_ENTER_ALWAYS)" Although I think that this doesn't really work anyway, because focus loss can also happen after the widget has been changed, I generally think that a changed() flag should only be reset, when the user code _did_ use it, and calling the callback doesn't guarantee that to be done. From Fl_Input's docs in http://www.fltk.org/doc-1.3/Fl_Input.html#Fl_Input.when you can read: "void Fl_Widget::when(Fl_When) Controls when callbacks are done. The following values are useful, the default value is FL_WHEN_RELEASE: [...] FL_WHEN_RELEASE: The callback will be done when this widget loses the focus, including when the window is unmapped. This is a useful value for text fields in a panel where doing the callback on every change is wasteful. However the callback will also happen if the mouse is moved out of the window, [...]" Note that FL_WHEN_RELEASE is the default, and thus a callback can be called after changing the widget, when minimizing the window or clicking in another window. When coming back to the window, the widget gets the focus again, but the changed() flag would be cleared because the callback cleared it. IMHO, that's a bad side effect. --- _My_ conclusion: Since we are doing API changes now, and since the changed() flag has not been documented in all its aspects anyhow, I think that it would be better, if we change this _now_ and document it with all its consequences, than if we would try to fight the symptoms and use the smart pointers here (although they could be of general use). Should we discuss the changed() flag problem in fltk.general with more users?

#10	AlbrechtS 05:26 Feb 10, 2009

I've started working on the main problem, i.e. that the FLTK core code may access widgets that have been deleted during a callback. The correct solution is to avoid accessing widgets after the callback, if they have been deleted during the callback. My intention is to make the delayed widget deletion mechanism (Fl::delete_widget()) obsolete. The solution is to use the new Fl_Watch class that uses the "smart pointers" (Fl::watch_widget_pointer() and others). The first step was to use this for Fl_Widget::do_callback() [done in svn -r 6651], but more checks with other widgets will have to follow. I'm currently reviewing the code, and I intend to add more callback checks during the next days (or maybe weeks...). To Matthias: I'd like to "take over" this STR, would this be okay for you? ----- Meanwhile I found another idea for a solution that maybe could be applied as well. Matthias wrote in [1]: "This is known bad behavior of the deeply annoying 'delete_widget' function. I suggest that we set a flag before calling any callbacks and clearing it after the callback. This flag would prohibit 'delete_widget' from being called. Are there any better ideas? Can anyone create a patch for this?" To be precise: This flag should prohibit Fl::do_widget_deletion() from doing its work, as long as a callback is active. IMHO this may also be possible, but it would only help _together_with_ Fl::delete_widget()'s delayed widget deletion mechanism. I'll check this ASAP. ----- [1] http://www.fltk.org/newsgroups.php?s9468+gfltk.general+v9468+T0

#11	matt 07:38 Feb 10, 2009

You may certainly take over this STR. I am happy about wrapping the functionality into a class and to get rid of the deferred delete. I would like to suggest a different name for the class though - I am no native speaker, but "watch" to me is such a commonly used word. How about: Fl_Widget_Tracker Fl_Widget_Guard Fl_Widget_Patrol or simply Fl_Smart_Pointer (maybe even making it public?!)

#12	AlbrechtS 10:05 Feb 10, 2009

I took over this STR, raised priority and moved it back to 1.3. As I wrote in fltk.development, I'm open for name suggestions...

#13	AlbrechtS 09:16 Feb 24, 2009

Update: I renamed class Fl_Watch to Fl_Widget_Tracker, as discussed in fltk.development (svn -r 6659)

#14	AlbrechtS 09:44 Feb 20, 2010

Fixed in Subversion repository. I added Fl_Widget_Tracker and checks to handle() methods and other code where callbacks might be called and widgets that have been deleted in the callback could be accessed later (svn -r 7115). I consider this as fixed now, but leave the STR open for a while for possible feedback or problem reports.