bugGNU Octave - Bugs: bug #45366, Two objects not cleaning...

 
 

bug #45366: Two objects not cleaning dynamically allocated widgets leading to crash at exit

Submitter:  Dan Sebald <sebald>
Submitted:  Sun 21 Jun 2015 06:57:15 AM UTC
   
 
Category:  GUI Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 10 Aug 2016 05:03:17 AM UTC, comment #12: 

The patch that was applied was merely to fix seg faults sometimes appearing at exit because some GUI objects were left active and without a parent.

You may be thinking of the patch here:

https://savannah.gnu.org/bugs/?func=detailitem&item_id=44485

the intention of which was to allow the GUI to exit even though the core is active or stuck.  I felt like I came close to solving that one, but at some point it started to feel too convoluted so I cried uncle, I guess.

The strategy I think is the right one.  If the user selects "exit" and the core is busy or stuck the GUI does not get an acknowledgement so sits silent.  So what I had done was start a Qt timer at the time of exit and if after half a second or so with no acknowledgement the timer expires and causes a dialog box to appear asking if exit should be forced.  (We don't want to toss someone's possibly long-running job just because they accidentally select close of the GUI window or type cntrl-Q when trying to close some other application's window.)  But that's not all of it.  Say the dialog box is displayed and THEN the acknowledgement comes.  Well, then we have to force that dialog closed even though the user hasn't selected anything so that the GUI can continue on and exit.  Oh, and the other thing is that one has to create some variable to block MULTIPLE exits with cntrl-Q or whatnot so that the dialog box doesn't appear more than once.

It just felt like it was getting to be too complex such that there might be a better way.  I could get active on that

https://savannah.gnu.org/bugs/?func=detailitem&item_id=44485

patch again if something is needed for the next release.

Dan Sebald <sebald>
Wed 10 Aug 2016 03:15:23 AM UTC, comment #11: 

I tried the test case in comment #2 from Philip, but Octave just continues to calculate forever on eigs.  I can't verify whether the patch has fixed anything or not.

Rik <rik5>
Group administrator
Mon 25 Jul 2016 09:47:01 PM UTC, comment #10: 

I updated and pushed this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/6e9f5408c0db

John W. Eaton <jwe>
Group administrator
Fri 01 Jul 2016 06:47:56 PM UTC, comment #9: 

[OT]
@Lachlan:
I usually have the following patches in my (64-bit) Windows builds that work fine AFAICS the last months. Many of them are from you:
- bug_36372_ranks_v2.cset
- bug_46236_workspace_colheaders_v2.cset (I just uploaded an updated patch)
- bug_46632_cursor_and_breakpoints_v3.cset
- bug_47149_line_x_y_transpose.cset (you wrote you polish it a bit more. For me it works fine)
- bug_47414_folded_goto.cset
- bug_48292_unhide_markers.cset (recent addition)
- patch_8956_F3-find_next-prev_shortcuts.diff (by Torsten)

I think all of these can be applied after final core dev review.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 29 Jun 2016 12:32:06 AM UTC, comment #8: 

Good suggestion, Philip.

Once I've finished a pass of triage, I'll check and polish this and all other supplied patches to the best of my ability, and mark that on the Wiki.

I'll also select some of my patches that aren't in one of Rik's selected categories but which I think should go into 4.2, and see if someone is willing to review and commit them :)

Lachlan Andrew <lachlan>
Tue 28 Jun 2016 04:26:06 PM UTC, comment #7: 

Rik cc'd

Philip Nienhuis <philipnienhuis>
Group Member
Tue 28 Jun 2016 04:25:34 PM UTC, comment #6: 

Adding Rik to cc list.

Lachlan why don't you polish the patch up to standards, and I'll test, and then we bug the core devs again? 
I normally don't push csets containing C/C++ code.

BTW, noting I tested this over a year ago.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 16 Jan 2016 07:41:06 AM UTC, comment #5: 

Can this patch be applied and the bug closed?

I've looked at the patch, and there are just a few things needed to make it comply with the coding standards:

In resource-manager.cc, resource_manager::instance_ok, the indentation of "instance = new resource_manager ();" should be reduced.  The same for shortcut-manager.cc, shortcut_manager::instance_ok.

In settings-dialog.cc, settings_dialog::show_tab, lines


ui->tabWidget->setCurrentIndex (settings->value ("settings/last_tab",
                                        0).toInt ());


the second line should align with the bracket after "settings->value", not the one before it.

Lachlan Andrew <lachlan>
Sun 21 Jun 2015 07:08:36 PM UTC, comment #4: 

Good.  Thanks for testing.

Dan Sebald <sebald>
Sun 21 Jun 2015 06:49:44 PM UTC, comment #3: 

OK works OK on Windows too, similar to Linux.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 21 Jun 2015 12:59:17 PM UTC, comment #2: 

Works fine on Linux (Mageia-4 64bit). Exits gracefully:

When doing:
[V, D] = eigs (rand (10000));
and then
File | Exit
after ~ 3 seconds I see "exit" flashing by in the terminal and Octave closes.

Cross-building for Windows now in background (while I help my daughter with her homework) - maybe tonight I can report on the Windows side.


Philip Nienhuis <philipnienhuis>
Group Member
Sun 21 Jun 2015 08:10:23 AM UTC, comment #1: 

Attached is a patch to fix the bugs--denoted by (bug fix) in the comment section.  There is also extensive rearranging of the cleanup code.  Basically, all the saving of settings that was done in destructors was moved to new member slot functions.  These new slots were then connected as follows:


      // Order is important.  Deleting QSettings must be last.
      connect (qApp, SIGNAL (aboutToQuit ()),
               command_window, SLOT (save_settings ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               history_window, SLOT (save_settings ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               file_browser_window, SLOT (save_settings ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               doc_browser_window, SLOT (save_settings ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               workspace_window, SLOT (save_settings ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               this, SLOT (prepare_to_exit ()));
      connect (qApp, SIGNAL (aboutToQuit ()),
               shortcut_manager::instance, SLOT (cleanup_instance ()));
      // QSettings are saved upon deletion (i.e., cleanup_instance)
      connect (qApp, SIGNAL (aboutToQuit ()),
               resource_manager::instance, SLOT (cleanup_instance ()));


One can see by the sequence above that all necessary settings saves are done (one via prepare_to_exit()), then the shortcut_manager and finally resource_manager are destroyed.

With these changes there is no longer a need for this route of cleanup:


-      if (instance)
-        singleton_cleanup_list::add (cleanup_instance);


which has been removed.

(file #34281)

Dan Sebald <sebald>
Sun 21 Jun 2015 06:57:15 AM UTC, original submission:  

In attempting to use the qApp aboutToQuit signal to save object settings I continually experienced a SEGV (11) segment fault at exit.  (This fact is probably led to the approach whereby the worker thread singleton-cleanup mechanism is used to clean up some GUI object.  That holds off deletion until the very last step.)  I eventually tracked down some bugs that are the source of segfault:

1) terminal_dock_widget::~terminal_dock_widget destructor is not deleting the dynamically-created QTerminal pointed to via 'terminal'.

2) QUnixTerminalImpl::~QUnixTerminalImpl destructor is not deleting the dynamically created TerminalModel (m_terminal), KPty (m_kpty), and TerminalView (m_terminalView).

The reason this causes a segmentation fault is that some of the objects left undeleted are widgets that remain active in the event processing of the GUI thread.  But at deletion during exit, their parents are gone and they may continue to access things such as resource_manager and shortcut_manager.  Objects have to delete the dynamic memory/objects they utilize, or hand those objects over to a container class such as the layout classes.

I will submit a patch shortly that includes the bug fixes in a larger reworking of the exit and cleanup code.

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 #34281:  octave-destructor_bugs-djs2015jun20.patch added by sebald (21KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by lachlan (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-10 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2016-07-25 jwe StatusPatch Submitted Ready For Test
    2016-07-01 rik5 Severity3 - Normal 4 - Important
    2016-06-28 philipnienhuis Carbon-Copy- Added rik
    2016-06-28 lachlan StatusNone Patch Submitted
    2015-06-21 sebald Attached File- Added octave-destructor_bugs-djs2015jun20.patch, #34281

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code