bugGNU Octave - Bugs: bug #37234, GUI fails to properly call all...

 
 

bug #37234: GUI fails to properly call all destructors and free all memory

Submitter:  Dan Sebald <sebald>
Submitted:  Fri 31 Aug 2012 08:11:29 AM UTC
   
 
Category:  GUI Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 11 Nov 2012 02:52:51 PM UTC, comment #12: 

Thanks for the detailed explanation. Imported and pushed the patch:
http://hg.savannah.gnu.org/hgweb/octave/rev/c9c79d4a0a00

We can remove the tests for 0 later if desired.

Torsten Lilge <ttl>
Group Member
Sun 11 Nov 2012 08:40:07 AM UTC, comment #11: 

The first delete should not cause a problem.  This probably requires some digging to find out exactly how memory is deleted, more than I care to know about Qt at the moment.  But one of two things could happen:

1) Deleting _current_directory_combo_box destroys the object then there is an error when the container class attempts to delete the object it had taken ownership of, but we can't see the error for some reason.

2) Deleting _current_directory_combo_box destroys the object and a base class of the widget notifies its parent that it is being destroyed and the container class knows to not attempt a delete.

> Is the test for 0 before deleting necessary?


As for testing whether a pointer is zero before testing, my first exposure to C++ was when it just hit the scene with Straustrup's book.  I can't recall that rule.  Maybe it's always been the case, maybe it was added after the initial definition.  I will investigate.

Generally speaking, it might be good to test if there is more to the conditional than just deleting the pointer.  For example:

  if (lexer)
    {
      delete lexer;
      _edit_area->setLexer(0);
    }

also does something if the lexer was valid and is no longer valid.  But I'll look this over.

> Should we set the pointers to 0 afterwards in order to be sure a second delete would be harmless?


Generally speaking, that is good practice but the cases I've added are in the destructor.  The destructor being the last thing called, those member variables are on the verge of going out of scope so won't be useful any more.  (An optimizing compiler might know enough to remove extraneous assignments like that.)  I've removed such a line (but haven't made the patch yet).

> This would allow to delete _current_directory_combo_box (just to be sure)


I'd say 'no'.  I.e., don't delete something that we know will be deleted elsewhere.  If there is some statistical uncertainty about things, then maybe.  But better to understand how things work and not have extraneous code.  What you are suggesting doesn't work, I believe.  The reason is that once the object is handed off to the container class, it has no knowledge of _current_directory_combo_box; that container isn't going to modify our member variable copy of it (but maybe its own record).  We keep _current_directory_combo_box around so that we can access the object but it is technically no longer ours, as I understand it.  (And maybe the better way to do this is to set up a signal/slot connection before handing the object off...because I do know that if objects on either end of a signal/slot are deleted, the signal connection is removed.

I was going to update the patch with the scheme we discussed on the list, but I've been thinking a couple things: 1) lack of easy way to get file information, 2) buffered vs. unbuffered file access, and maybe we need to be careful on what we choose because often it will be the case that an editor file is saved and then immediately run in the command line thread.  We might want to be more cognizant of what it is we're doing exactly.  I'm worried that saving a file using buffered i/o might not be complete and readable by the octave command line thread.

Dan Sebald <sebald>
Sat 10 Nov 2012 04:22:24 PM UTC, comment #10: 

I agree, ownership should be taken via addItem that in turn is called by addWidget. Strangely enough, _current_directory_combo_box can be deleted without error (only a second delete leads to a segmentation fault).

Two other things:
Is the test for 0 before deleting necessary? The c++ standard says that nothing happens if delete gets a null pointer.
Should we set the pointers to 0 afterwards in order to be sure a second delete would be harmless? This would allow to delete _current_directory_combo_box (just to be sure)

Torsten Lilge <ttl>
Group Member
Sat 10 Nov 2012 09:54:14 AM UTC, comment #9: 

I'm not sure, but I don't think _current_directory_combo_box needs to be deleted at in the destructor.  (Could try, probably would get a system error, but again, this would be hard to debug...which reminds me, I had in mind to rewrite the unix terminal so that right before exit it switches STDERR back to the console.)  The reason, I believe, is noted in the patch file,

+  // addWidget takes ownership of the objects so there is no
+  // need to delete these upon destroying this main_window.

Check the Qt documentation for addWidget and see if you agree with me on that.

The HDF5 error?  That's new to me.  A separate bug report for that seems in order.

Dan Sebald <sebald>
Sat 10 Nov 2012 09:35:39 AM UTC, comment #8: 

What about _current_directory_combo_box? Shouldn't this be deleted, too?

On my system, I am am seeing the following error message on exit (with or without the patch)

HDF5: infinite loop closing library
D,T,F,FD,P,FD,P,FD,P,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E,E

Also a hint for some resource leaks, I guess.

Torsten Lilge <ttl>
Group Member
Mon 15 Oct 2012 01:34:51 AM UTC, comment #7: 

Re-titled bug report and changed the status to "Patch Submitted"

Rik <rik5>
Group administrator
Sun 14 Oct 2012 04:40:46 AM UTC, comment #6: 

Comment #3 was a poor assumption on my part.  Getting rid of several bugs didn't make this shutdown bug go away.

This bug isn't really related to stderr being un-redirected.  It's just that when stderr is un-redirected, it is possible to see what the errors are.  If the stderr is redirected to the GUI command line window there is no way for one to see those error messages because the GUI is no longer visible.  (That is why I say the GUI shouldn't be redirecting STDERR, but that causes other problems with the pager.)

Anyway, I've attached a patch where I've simply gone through and cleaned up the main_window destructor properly.  All these objects created dynamically via "new" must be deleted.  Otherwise, especially in the case of "this" being an argument of the constructor, problems can arise.  There could be orphaned children still looking for "this" if main_window is cleaned up by the system before the children are.  Basically, it is imperative in C++ OOP that dynamic objects be cleaned up properly no matter the case.  Destructors often have very important code.

I've tested out this simple cleanup and I no longer see any errors at shutdown.  In fact, what happens is a slight delay, probably due to the system and Qt properly destroying things.  I didn't extensively test this or look elsewhere.  At this stage of my knowledge and what seems like several issues I'm only going after wide swaths.

(file #26760)

Dan Sebald <sebald>
Sat 13 Oct 2012 02:50:50 PM UTC, comment #5: 

I closed it based on your comment #3, "Let's close this bug report because there are broader bugs in the GUI right now (e.g., command window doesn't update properly) and there are all kinds of shadow variable warnings associated with GUI compilation."

I re-opened it, but re-titled it to your new observations in comment #4.

Rik <rik5>
Group administrator
Sat 13 Oct 2012 03:40:59 AM UTC, comment #4: 

I suggest leaving this bug open, or perhaps create a new one that isn't specific to "when in debug mode".  It's actually more general than that.  I'm pretty sure there is a bad memory/pointer bug in there somewhere.  I've left the stderr un-redirected and when I close the application get a very long string of errors and violations that ends as:

7feedd524000-7feedd7bf000 r-xp 00000000 fd:00 74423                      /usr/lib64/libqscintilla2.so.8.0.2
7feedd7bf000-7feedd9bf000 ---p 0029b000 fd:00 74423                      /usr/lib64/libqscintilla2.so.8.0.2
7feedd9bf000-7feedd9cd000 rw-p 0029b000 fd:00 74423                      /usr/lib64/libqscintilla2.so.8.0.2
7feedd9cd000-7feedd9cf000 rw-p 00000000 00:00 0
7feedd9cf000-7feedd9d0000 r--s 00000000 fd:00 57108                      /var/cache/fontconfig/3c3fb04d32a5211b073874b125d29701-le64.cache-3
7feedd9d0000-7feedd9d1000 r--s 00000000 fd:00 57107                      /var/cache/fontconfig/e61abf8156cc476151baa07d67337cae-le64.cache-3
7feedd9d1000-7feedd9d5000 r--s 00000000 fd:00 57106                      /var/cache/fontconfig/b67b32625a2bb51b023d3814a918f351-le64.cache-3
7feedd9d5000-7feedd9d7000 r--s 00000000 fd:00 57105                      /var/cache/fontconfig/d3379abda271c4acd2ad0c01f565d0b0-le64.cache-3
7feedd9d7000-7feedd9d8000 r--s 00000000 fd:00 57104                      /var/cache/fontconfig/b4d0b56f766d89640448751fcd18ec1e-le64.cache-3
7feedd9d8000-7feedd9e1000 r--s 00000000 fd:00 57103                      /var/cache/fontconfig/12b26b760a24f8b4feb03ad48a333a72-le64.cache-3
7feedd9e1000-7feedd9e8000 r--s 00000000 fd:00 72179                      /usr/lib64/gconv/gconv-modules.cache
7feedd9e8000-7feede9a0000 r-xp 00000000 fd:00 13369622                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libinterp/.libs/liboctinterp.so.1.0.1
7feede9a0000-7feedeba0000 ---p 00fb8000 fd:00 13369622                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libinterp/.libs/liboctinterp.so.1.0.1
7feedeba0000-7feedebfb000 rw-p 00fb8000 fd:00 13369622                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libinterp/.libs/liboctinterp.so.1.0.1
7feedebfb000-7feedec04000 rw-p 00000000 00:00 0
7feedec04000-7feeded03000 r-xp 00000000 fd:00 13369789                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libgui/.libs/liboctgui.so.0.0.0
7feeded03000-7feedef03000 ---p 000ff000 fd:00 13369789                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libgui/.libs/liboctgui.so.0.0.0
7feedef03000-7feedef0c000 rw-p 000ff000 fd:00 13369789                   /usr/local/src/octave/octave-gui_no_casts2/build-gui-2/libgui/.libs/liboctgui.so.0.0.0
7feedef0c000-7feedef12000 rw-p 00000000 00:00 0
7fffc0f76000-7fffc0f99000 rw-p 00000000 00:00 0                          [stack]
7fffc0fff000-7fffc1000000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
panic: Aborted -- stopping myself...
attempting to save variables to 'octave-workspace'...
panic: attempted clean up apparently failed -- aborting...

There is a patch at

http://savannah.gnu.org/patch/?7857

that cleans up a large amount of code, properly deleting objects so that memory isn't leaked, fixing a number of bugs.  It only addresses startup and editor behavior.  Addressing these other bugs should follow that.

My guess about this particular bug is that an object isn't being deleted properly.  (Whenever there is a "new" used, the first thing the programmer should do is find the proper spot for its associated "delete".)  Cleanup is often very important in object oriented programming.

The shadow variables I've found to be innocuous, but they should be cleaned up.  Typically it is where "parent" is used as the input to a constructor, but the variable "parent" is already in the class.  Just replace constructor argument "parent" with "p", etc.

Dan Sebald <sebald>
Sat 15 Sep 2012 05:41:16 PM UTC, comment #3: 

Someone just reported having the GUI not start generally:

https://savannah.gnu.org/bugs/index.php?37359

so this may not be related to the debug problems.  Let's close this bug report because there are broader bugs in the GUI right now (e.g., command window doesn't update properly) and there are all kinds of shadow variable warnings associated with GUI compilation.

Dan Sebald <sebald>
Sat 15 Sep 2012 05:35:27 PM UTC, comment #2: 

I tried again--setting a couple breakpoints, stepping through--the unix prompt returns.  However, I relaunch the GUI and the process ends without the appearance of the GUI.  My default-qt-settings doesn't seem to have changed...oh wait, the file now seems to be

~/.config/octave/qt-settings

and the time/date of that file suggests it was modified when I recently tested this.  The file is small, so I'm going to print this out:


[General]
connectOnStartup=true
showMessageOfTheDay=true
showTopic=true
autoIdentification=false
nickServPassword=
useCustomFileEditor=false
customFileEditor=emacs
showFilenames=true
showFileSize=false
showFileType=false
showLastModified=false
showHiddenFiles=false
useAlternatingRowColors=true
useProxyServer=false
proxyType=
proxyHostName=none
proxyPort=8080
proxyUserName=
proxyPassword=

[editor]
showLineNumbers=true
highlightCurrentLine=true
codeCompletion=true
fontName=DejaVu Sans Mono
fontSize=12
shortWindowTitle=true
longWindowTitle=true
savedSessionTabs=/home/sebald/octave/example.m

[terminal]
fontSize=10
fontName=DejaVu Sans Mono
cursorBlinking=false
cursorType=ibeam

[MainWindow]
geometry=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\0\x19\0\0\x3\xd1\0\0\x3\x12\0\0\0\x4\0\0\0\x31\0\0\x3\xcd\0\0\x3\xe\0\0\0\0\0\0)
windowState=@ByteArray(\0\0\0\xff\0\0\0\0\xfd\0\0\0\x1\0\0\0\x1\0\0\x3\xca\0\0\x2\x87\xfc\x2\0\0\0\x2\xfc\0\0\0\x41\0\0\x1\xc7\0\0\x1\xc7\0\xff\xff\xff\xfc\x1\0\0\0\x3\xfc\0\0\0\0\0\0\0\xb3\0\0\0X\0\xff\xff\xff\xfc\x2\0\0\0\x2\xfb\0\0\0\x1a\0W\0o\0r\0k\0s\0p\0\x61\0\x63\0\x65\0V\0i\0\x65\0w\x1\0\0\0\x41\0\0\0\xf1\0\0\0q\0\xff\xff\xff\xfb\0\0\0\"\0H\0i\0s\0t\0o\0r\0y\0\x44\0o\0\x63\0k\0W\0i\0\x64\0g\0\x65\0t\x1\0\0\x1\x38\0\0\0\xd0\0\0\0\x92\0\xff\xff\xff\xfc\0\0\0\xb9\0\0\x2Z\0\0\x2X\0\xff\xff\xff\xfa\0\0\0\0\x2\0\0\0\x2\xfb\0\0\0$\0T\0\x65\0r\0m\0i\0n\0\x61\0l\0\x44\0o\0\x63\0k\0W\0i\0\x64\0g\0\x65\0t\x1\0\0\0\0\xff\xff\xff\xff\0\0\x1\xa9\0\xff\xff\xff\xfb\0\0\0\x14\0\x46\0i\0l\0\x65\0\x45\0\x64\0i\0t\0o\0r\x1\0\0\0\x41\0\0\x1\xc7\0\0\0\xdf\0\xff\xff\xff\xfb\0\0\0\x1e\0\x46\0i\0l\0\x65\0s\0\x44\0o\0\x63\0k\0W\0i\0\x64\0g\0\x65\0t\x1\0\0\x3\x19\0\0\0\xb1\0\0\0V\0\xff\xff\xff\xfb\0\0\0.\0\x44\0o\0\x63\0u\0m\0\x65\0n\0t\0\x61\0t\0i\0o\0n\0\x44\0o\0\x63\0k\0W\0i\0\x64\0g\0\x65\0t\x1\0\0\x2\xe\0\0\0\xba\0\0\0\xba\0\xff\xff\xff\0\0\0\0\0\0\x2\x87\0\0\0\x4\0\0\0\x4\0\0\0\b\0\0\0\b\xfc\0\0\0\x1\0\0\0\x2\0\0\0\x1\xff\xff\xff\xff\x1\0\0\0\0\xff\xff\xff\xff\0\0\0\0\0\0\0\0)

[DockWidgets]
qt_rubberband=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\0\0\xff\xff\xff\xff\xff\xff\xff\xff\0\0\0\0\0\0\0\0\xff\xff\xff\xff\xff\xff\xff\xff\0\0\0\0\0\0)
WorkspaceView=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\0\x41\0\0\0\xb2\0\0\x1\x31\0\0\0\0\0\0\0\x41\0\0\0\xb2\0\0\x1\x31\0\0\0\0\0\0)
HistoryDockWidget=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\x1\x38\0\0\0\xb2\0\0\x2\a\0\0\0\0\0\0\x1\x38\0\0\0\xb2\0\0\x2\a\0\0\0\0\0\0)
FilesDockWidget=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\x3\x19\0\0\0\x41\0\0\x3\xc9\0\0\x2\a\0\0\x3\x19\0\0\0\x41\0\0\x3\xc9\0\0\x2\a\0\0\0\0\0\0)
DocumentationDockWidget=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\x2\xe\0\0\x3\xc9\0\0\x2\xc7\0\0\0\0\0\0\x2\xe\0\0\x3\xc9\0\0\x2\xc7\0\0\0\0\0\0)
CentralDummyWidget=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\0\0\0\0\x19\xff\xff\xff\xff\0\0\0\x18\0\0\0\0\0\0\0\0\xff\xff\xff\xff\xff\xff\xff\xff\0\0\0\0\0\0)
FileEditor=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\xff\xff\xfb\xb2\xff\xff\xfc\x63\xff\xff\xfe\v\xff\xff\xfe\v\xff\xff\xfb\xb2\xff\xff\xfc\x63\xff\xff\xfe\v\xff\xff\xfe\v\0\0\0\0\0\0)
TerminalDockWidget=@ByteArray(\x1\xd9\xd0\xcb\0\x1\0\0\0\0\0\xb9\0\0\0\x41\0\0\x3\x12\0\0\x1\xe9\0\0\0\xb9\0\0\0\x41\0\0\x3\x12\0\0\x1\xe9\0\0\0\0\0\0)

[workspaceview]
local_collapsed=false
global_collapsed=false
persistent_collapsed=false

Dan Sebald <sebald>
Sat 15 Sep 2012 01:49:35 PM UTC, comment #1: 

Do you still encounter the problem with a recent build?
I am not able to reproduce it, the IDE closes and the unix prompt returns.

Torsten Lilge <ttl>
Group Member
Fri 31 Aug 2012 08:11:29 AM UTC, original submission:  

I launced Octave-IDE from a unix command line as a foreground process.  I created a simple program in the editor of the IDE and set some break points.  I ran the program and the command line showed that Octave was in debug mode.  I inadvertently typed "exit" at the command line.  The IDE closed by the command line did not return.

I then ran octave-cli and set a break point manually.  I processed the script, at which point Octave displayed the debug prompt.  Typing "exit" brought back the unix command line.

Dan Sebald <sebald>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #26760:  octave-gui_main_window_cleanup-2012oct13.patch added by sebald (5KiB - text/x-patch - Small patch that deletes objects in ~main_window() for proper shutdown cleanup.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by ttl (Posted a comment)
  • -email is unavailable- added by sebald (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-11-11 ttl StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2012-10-15 rik5 StatusNone Patch Submitted
        SummaryGUI crashes when stderr un-redirected GUI fails to properly call all destructors and free all memory
    2012-10-14 sebald Attached File- Added octave-gui_main_window_cleanup-2012oct13.patch, #26760
    2012-10-13 rik5 CategoryNone GUI
        Open/ClosedClosed Open
        SummaryIDE hangs upon &quot;exit&quot; when in debug mode. GUI crashes when stderr un-redirected
    2012-10-12 rik5 Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code