bugGNU Octave - Bugs: bug #47699, FLTK can segfault after using...

 
 

bug #47699: FLTK can segfault after using legend

Submitter:  Rik <rik5>
Submitted:  Thu 14 Apr 2016 08:10:54 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
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

Fri 29 Apr 2016 04:16:48 PM UTC, comment #25: 

Finally, we have a solution.  I checked in a modified version of your cset here http://hg.savannah.gnu.org/hgweb/octave/rev/c32590cd0080.  Specifically, I changed the flag variable from an int to a bool, since this construct was widely used elsewhere in the code.  And I changed the name from pause_redraw to delete_executing to match other examples in the code like drawnow_executing.  It should happen very rarely, but I still thought it good practice to preface the warning with the function that generated it.  If someone does encounter this problem later it will be much more informative then just "internal error" which could occur anywhere.

Rik <rik5>
Group administrator
Fri 29 Apr 2016 08:50:41 AM UTC, comment #24: 

You raised a good point about debugging the callback.  That makes me lean towards the second cset, which had a global flag.

This attached changeset inhibits drawnow during delete regardless of where it is called from.  That solves the "dbnext" problem, but not the "dbquit" problem.  If we dbquit then the deletion is not completed, but the unwind_protect resets the global flag and so the next redraw will cause problems.

I toyed with a patch that blocked "dbquit" in that context, but it still crashes if you "quit" while in debug mode, so I gave up.

However, the only reason it seems to crash is an assert that seems to be called onlly while closing.  The attached changeset replaces that by a warning so that other clean up (like saving the history) can continue despite there being some undeleted graphics objects.

(file #37018)

Lachlan Andrew <lachlan>
Fri 29 Apr 2016 12:58:35 AM UTC, comment #23: 

@Lachlan: I've tried all three csets.  I guess I like the last as the most minimal.  However, there is still a corner case problem.  If I set a breakpoint in the callback deletelegend2 and then step through the program I get same error about invalid graphics object that was originally reported.  I assume that this is because returning to the debug prompt is like returning to the Octave prompt and an implicit call to update events and redraw plots is made each time.

Steps to Reproduce:


plot (1:10)
h = legend ("abc");
dbstop legend>deletelegend2
delete (h)
...
dbnext
...



Maybe this is too much of a corner case to be concerned with, or should we isolate the offending code with a try/catch block?  I tried that with the attached diff.  It works, but produces an error message which isn't great.


(file #37015)

Rik <rik5>
Group administrator
Thu 28 Apr 2016 11:26:11 PM UTC, comment #22: 

I checked in a partial fix in legend.m (http://hg.savannah.gnu.org/hgweb/octave/rev/2e07dcdc6139).  This at least allows the figure to be closed after an error has been generated.

Rik <rik5>
Group administrator
Thu 28 Apr 2016 06:44:31 AM UTC, comment #21: 

No, the guilt is still mine :(

It turns out that the error is thrown exactly in the second chunk of text Rik identified:


graphics_object go = gh_manager::get_object (h);
figure::properties& fprops
   = dynamic_cast <figure::properties&> (go.get_properties ());


The problem is that  h  has been partly deleted and so we can't get its properties, but it is still in hlist, and still passes the test h.ok ().

The only mystery is why this didn't initially show up with qt.  Juan Pablo's example gives me the error with qt.

We can't refrain from calling callbacks during deletion, since the whole purpose of the "deletefcn" callback is deletion.  In this case, it is calling @deletelegend2.

It seems that a lot of care has gone into the sequencing of deletion events around the callback -- such as the following code:
+verbatim
  // NOTE: free the handle before removing it from its parent's
  //       children, such that the object's state is correct when the
  //       deletefcn callback is executed
    gh_manager::free (h);

  // A callback function might have already deleted the parent
    if (parent_go.valid_object ())
      parent_go.remove_child (h);
-verbatim-

I think the best thing to do is simply not to redraw during deletion.  The attached patch is possibly a  cleaner approach.  Instead of storing a global flag, it simply refrains from redrawing if the object of the current callback is being deleted.

(file #37009)

Lachlan Andrew <lachlan>
Thu 28 Apr 2016 04:22:49 AM UTC, comment #20: 

Great.  If this is seen as my cset just triggering an existing bug I'll fell less guilty :)

I'll keep looking into it.


Lachlan Andrew <lachlan>
Thu 28 Apr 2016 03:21:13 AM UTC, comment #19: 

I applied Lachlan's patch, on default, and it does fix the problem.  I'm reluctant to push it, however, because I don't understand the root cause.  Shouldn't objects which are being deleted refrain from executing callbacks?  Octave seems to have that already in delete_object in graphics.cc.


      // Don't do recursive deleting, due to callbacks
      if (! go.get_properties ().is_beingdeleted ())
        {
          graphics_handle parent_h = go.get_parent ();

          graphics_object parent_go = gh_manager::get_object (parent_h);

          // NOTE: free the handle before removing it from its parent's
          //       children, such that the object's state is correct when the
          //       deletefcn callback is executed

          gh_manager::free (h);

          // A callback function might have already deleted the parent
          if (parent_go.valid_object ())
            parent_go.remove_child (h);

          Vdrawnow_requested = true;
        }


Alternatively, even if Fdrawnow is called, shouldn't it detect that a graphics object is invalid before re-drawing it?  We seem to have that as well.


          Matrix hlist = gh_manager::figure_handle_list (true);

          for (int i = 0; i < hlist.numel (); i++)
            {
              graphics_handle h = gh_manager::lookup (hlist(i));

              if (h.ok () && h != 0)
                {
                  graphics_object go = gh_manager::get_object (h);
                  figure::properties& fprops
                    = dynamic_cast <figure::properties&> (go.get_properties ());

                  if (fprops.is_modified ())
                    {
                      if (fprops.is_visible ())
                        {
                          gh_manager::unlock ();

                          fprops.get_toolkit ().redraw_figure (go);

                          gh_manager::lock ();
                        }

                      fprops.set_modified (false);
                    }
                }
            }


Maybe besides checking the modified status, it should check whether it is being deleted before calling redraw_figure?

Rik <rik5>
Group administrator
Thu 28 Apr 2016 12:15:36 AM UTC, comment #18: 

I didn't observe the crash in stable -- which isn't surprising since the changeset that Rik identified doesn't appear in stable.

Juan Pablo, are you sure that it is "stable" rather than "default" that is giving you the problem?

Lachlan Andrew <lachlan>
Wed 27 Apr 2016 08:44:08 AM UTC, comment #17: 

Juan Pablo, have you tried the patch, file #36987?

I just tried your example on my patched default branch and it didn't crash.  I'm just compiling stable now.

Lachlan Andrew <lachlan>
Wed 27 Apr 2016 08:23:11 AM UTC, comment #16: 

Oh, it is worth mentioning that if the script is changed to

y   = randn(100,5);
h   = plot (y);
txt = {'1','2','3','4','5'};
h   = legend (h, txt);

pause (1)

close all


the bug is not triggered.
So my guess is that this is a multi-thread issue, where the graphics object is not done yet but the "close all" command is called nevertheless.

Juan Pablo Carbajal <juanpi>
Group Member
Wed 27 Apr 2016 08:18:49 AM UTC, comment #15: 

I think the following is related to this bug.


octave --no-gui --no-history --silent --eval "close_test"
error: base_graphics_object::get_properties: invalid graphics object
error: unhandled execution exception -- eval failed
octave-gui: ../default/libinterp/corefcn/graphics.cc:2725: void gh_manager::do_close_all_figures(): Assertion `hlist.numel () == 0' failed.
panic: Aborted -- stopping myself...
Aborted (core dumped)


or

octave-gui:1> close_test
error: base_graphics_object::get_properties: invalid graphics object
error: called from
    delete at line 60 column 5
    closereq at line 42 column 5
    close at line 96 column 5
    test at line 5 column 1
octave-gui:1> exit
octave-gui: ../default/libinterp/corefcn/graphics.cc:2725: void gh_manager::do_close_all_figures(): Assertion `hlist.numel () == 0' failed.
panic: Aborted -- stopping myself...
Aborted (core dumped)


The close_test.m script is

y   = randn(100,5);
h   = plot (y);
txt = {'1','2','3','4','5'};
h   = legend (h, txt);
close all


This bug makes all the scripts I am currently using with octave stable crash, so for me this is quite a severe bug. Let me know if I can be of assistance.

Juan Pablo Carbajal <juanpi>
Group Member
Mon 25 Apr 2016 03:00:28 AM UTC, comment #14: 

Thanks for confirming that, Philip.

On Linux, I don't get that warning from 'demo ("copyobj", 1)', so I can't help track it down.

Lachlan Andrew <lachlan>
Sun 24 Apr 2016 08:40:23 PM UTC, comment #13: 

@Panxto:
Thanks for that explanation, I believe it must be something like that. If I only could find the offending command in .octave-hist...


Anyway, the crash doesn't occur anymore on Windows with Lachlan's patch from comment #11.

I still get the

copyobj example 1: failed
varargin(1): out of bound 0


warning/error (what is it? no clue) and in the copied figure the little parabola isn't drawn.
But on Windows copyobj always FAILs when running _run_test_suite_.m the last months (similarly hgsave) so there is probably a relation.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Apr 2016 02:06:00 PM UTC, comment #12: 

@Phillip: about comment #8, I don't think this is related, bare patch objects don't trigger listener callbacks that are affected by cset 21543. As it says, this (too) invasive warning appears when *data properties of a patch have inconsistent shapes. You can reproduce it easily e.g:


hpa = patch ();
xdata = [0; 2; 2];
ydata = [0; 0; 1];

set (hpa, "xdata", xdata, "ydata", ydata')
## then the warning is triggered after each and every redrawing
set (hpa, "xdata", xdata, "ydata", ydata)
## fix the shape inconsistency and the warning disappears


I think the warning could be made to appear only once but this is another eventual bug report.

Pantxo Diribarne <pantxo>
Group Member
Sun 24 Apr 2016 01:00:39 PM UTC, comment #11: 

This version is slightly more robust; it uses unwind_protect to keep the lock in sync even if an error occurs in the deletions.

(file #36987)

Lachlan Andrew <lachlan>
Sun 24 Apr 2016 11:27:07 AM UTC, comment #10: 

The problem seems to be that FLTK deletion triggers a callback while not all of the graphics objects are valid.  That only causes a problem if callbacks trigger redraws.

One approach is simply to back out changeset 21543:4f0f9f029d12.

Another is to place a lock around the deletion (attached patch).

I don't like fixing all bugs by adding extra code.  Would it be better to try to modify the FLTK code to ensure callbacks only happen when all objects are valid?  That would make the code "delicate", and leaves the possibility of similar bugs from other pieces of code

(file #36986)

Lachlan Andrew <lachlan>
Sun 24 Apr 2016 11:03:59 AM UTC, comment #9: 

(got distracted, sorry)
Here's opol.mat

(file #36985)

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Apr 2016 11:02:35 AM UTC, comment #8: 

Rik, you suspected the legend. Running the example w/o the legend line makes no difference here.


Then, maybe a red herring, but while experimenting with some graphics things (polygon clipping) I hit a similar issue with the qt tookit after I tried plotting a patch (really nothing more fancy than that) as follows:

figure
patch (opol{1}(:, 1), opol{1}(:, 2), 'c')
warning: opengl_renderer: x/y/zdata must have the same dimensions. Not rendering.
## ...Indeed, no drawing

warning: opengl_renderer: x/y/zdata must have the same dimensions. Not rendering.
## ^^^ that message ^^^ repeated over and over asynchronously
##     in between other commands in the terminal
:
:
>> clear all; close all
warning: opengl_renderer: x/y/zdata must have the same dimensions. Not rendering.
warning: called from
    delete at line 58 column 3
    closereq at line 42 column 5
    close at line 96 column 5
>>


(opol.mat attached)

However, I couldn't reproduce it later on.

Apparently the graphics system is a bit unstable ATM.

Original bug reproducible on Windows as well => OS->Any

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Apr 2016 07:24:30 AM UTC, comment #7: 

Thanks, Rik.  I have no idea off the top of my head, but will look into it.

Lachlan Andrew <lachlan>
Sat 23 Apr 2016 04:29:36 PM UTC, comment #6: 

This is weird.  Using bisection, the cset that introduced this segfault was


changeset:   21543:4f0f9f029d12
user:        Lachlan Andrew <lachlanbis@gmail.com>
date:        Mon Feb 22 13:18:02 2016 +1100
summary:     Draw updates to GUI at end of GUI callback function (bug #47216)


I've added Lachlan to the CC list for this bug.  Maybe he has an idea where this is coming from.

Rik <rik5>
Group administrator
Sat 23 Apr 2016 02:44:44 PM UTC, comment #5: 

The '0' is probably reasonable as the root figure is the parent of just about everything.

I'm running a bisection now to try and locate which changeset caused this.  So far I've determined that it occurred somewhere in the last two months.

Rik <rik5>
Group administrator
Sat 23 Apr 2016 11:04:39 AM UTC, comment #4: 

@comment #2:
I wasn't too clear about that for... syntax question, I'm afraid :-)
What I see in copyobj.m is this:

<... debugging ...>
52:     for hp = hparent(:)'
debug> hparent
hparent = 0
debug>


so the for loop header is "for hp = 0".
Trying out I see such loops run once; but it's obscure syntax.
(In this case no harm done, it's just input checking)

Philip Nienhuis <philipnienhuis>
Group Member
Fri 22 Apr 2016 11:08:44 PM UTC, comment #3: 

I've found an easier example.


run-octave  -f --no-gui
>> graphics_toolkit fltk
>> plot (1:10)
>> legend ('abc')
>> close all
error: base_graphics_object::set: invalid graphics object
error: called from
    delete at line 60 column 5
    closereq at line 42 column 5
    close at line 96 column 5


My hunch is that the legend function, which is very complex, has screwed things up.

Rik <rik5>
Group administrator
Fri 22 Apr 2016 04:52:32 PM UTC, comment #2: 

Answering the question in the second part of comment #1, yes, it is valid syntax.  The suffix (:)' universally transforms whatever array precedes it into a row vector.  This is required because the Octave/Matlab for loop operates over columns supplying one column per iteration of the loop.

Here's an example


magic(3)(:)'
ans =

   8   3   4   1   5   9   6   7   2

for x = magic(3)(:)'
  disp (x);
endfor
 8
 3
 4
 1
 5
 9
 6
 7
 2



Rik <rik5>
Group administrator
Fri 22 Apr 2016 09:14:26 AM UTC, comment #1: 

Instead of title-bar clicking the segfault can be triggered by "close all" as well.

Running it on Windows (both using the GUI and the BASH shell, mxe-octave configured with "--enable-devel-tools"), I get this:

octave:1> graphics_toolkit fltk
octave:2> demo ("copyobj", 1)
copyobj example 1:
:
< demo commands >
:

copyobj example 1: failed
varargin(1): out of bound 0
{END}


I could track it a little bit by manually executing the demo commands (finally invoking debug_on_error(1)), to a call to line.m
(copyobj.m L.87 -> struct2hdl.m L.144 -> struct2hdl.m.createline L.290 -> line.m L.69)
where I think that just above that, in line.m L.56 the call to _plt_get_axis_arg_ has reset varargin to empty. Why that happened is beyond me; previous calls to line.m along this sequence in this demo all worked well.

BTW in copyobj.m I see this syntax on L.52:

 for hp = hparent(:)'

Is that valid syntax?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 14 Apr 2016 08:10:54 PM UTC, original submission:  

This is a regression, and doesn't affect the qt toolkit.

Steps to reproduce


./run-octave -f --no-gui
graphics_toolkit fltk
demo ("copyobj", 1)
## Click on the title bar of the window labeled "Original" to activate it.
## Click on the title bar button to close the window completely.
>> error: base_graphics_object::set: invalid graphics object
error: parse error
error: called from
    delete at line 60 column 5
    closereq at line 42 column 5
    close at line 96 column 5
## Click on the title bar button to close the "Copyobj" window

## Octave segfaults
error: base_graphics_object::set: invalid graphics object
terminate called after throwing an instance of 'octave_execution_exception'
panic: Aborted -- stopping myself...



Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #37015:  47699_trycatch.cset added by rik5 (2KiB - application/octet-stream)
file #36986:  bug_47699_fltk_close added by lachlan (2KiB - application/octet-stream)
file #36985:  opol.mat added by philipnienhuis (719B - 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 juanpi (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (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 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-04-29 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2016-04-29 lachlan Attached File- Added bug_47699_fltk_close_v4.cset, #37018
    2016-04-29 rik5 Attached File- Added 47699_trycatch.cset, #37015
    2016-04-28 lachlan Attached File- Added bug_47699_fltk_close_v3.cset, #37009
    2016-04-25 lachlan StatusConfirmed Patch Submitted
    2016-04-24 lachlan Attached File- Added bug_47699_fltk_close_v2.cset, #36987
    2016-04-24 lachlan Attached File- Added bug_47699_fltk_close, #36986
        Operating SystemAny GNU/Linux
    2016-04-24 philipnienhuis Attached File- Added opol.mat, #36985
    2016-04-24 philipnienhuis Operating SystemGNU/Linux Any
    2016-04-23 rik5 Carbon-Copy- Added lachlan
    2016-04-22 rik5 SummaryFLTK can segfault after using copyobj FLTK can segfault after using legend
    2016-04-22 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code