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

 
 

bug #47699: FLTK can segfault after using legend

Submitted by:  Rik <rik5>
Submitted on:  Thu 14 Apr 2016 08:10:54 PM UTC  
 
Category: Plotting with OpenGLSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Regression
Status: FixedAssigned to: None
Originator Name: Open/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project Member
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:

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>
Project 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>
Project 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:

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>
Project Member
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>
Project Member
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.

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.

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

Rik <rik5>
Project 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>
Project Member
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>
Project Member
Wed 27 Apr 2016 08:23:11 AM UTC, comment #16:

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

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>
Project Member
Wed 27 Apr 2016 08:18:49 AM UTC, comment #15:

I think the following is related to this bug.

or

The close_test.m script is

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>
Project 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>
Project Member
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

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>
Project 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:

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

Pantxo Diribarne <pantxo>
Project 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>
Project Member
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>
Project Member
Sun 24 Apr 2016 11:03:59 AM UTC, comment #9:

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

(file #36985)

Philip Nienhuis <philipnienhuis>
Project 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:

(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>
Project 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>
Project Member
Sat 23 Apr 2016 04:29:36 PM UTC, comment #6:

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

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

Rik <rik5>
Project 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>
Project 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:

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>
Project Member
Fri 22 Apr 2016 11:08:44 PM UTC, comment #3:

I've found an easier example.

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

Rik <rik5>
Project 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

Rik <rik5>
Project 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:

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>
Project 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

Rik <rik5>
Project Administrator

 

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

Attach File(s):
   
   
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
  • -unavailable- added by juanpi (Posted a comment)
  • -unavailable- added by pantxo (Posted a comment)
  • -unavailable- added by lachlan (Posted a comment)
  • -unavailable- added by rik5
  • -unavailable- added by rik5 (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 14 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 29 Apr 2016 04:16:48 PM UTCrik5StatusPatch Submitted=>Fixed
      Open/ClosedOpen=>Closed
    Fri 29 Apr 2016 08:50:41 AM UTClachlanAttached File-=>Added bug_47699_fltk_close_v4.cset, #37018
    Fri 29 Apr 2016 12:58:35 AM UTCrik5Attached File-=>Added 47699_trycatch.cset, #37015
    Thu 28 Apr 2016 06:44:31 AM UTClachlanAttached File-=>Added bug_47699_fltk_close_v3.cset, #37009
    Mon 25 Apr 2016 03:00:28 AM UTClachlanStatusConfirmed=>Patch Submitted
    Sun 24 Apr 2016 01:00:39 PM UTClachlanAttached File-=>Added bug_47699_fltk_close_v2.cset, #36987
    Sun 24 Apr 2016 11:27:07 AM UTClachlanAttached File-=>Added bug_47699_fltk_close, #36986
      Operating SystemAny=>GNU/Linux
    Sun 24 Apr 2016 11:03:59 AM UTCphilipnienhuisAttached File-=>Added opol.mat, #36985
    Sun 24 Apr 2016 11:02:35 AM UTCphilipnienhuisOperating SystemGNU/Linux=>Any
    Sat 23 Apr 2016 04:28:19 PM UTCrik5Carbon-Copy-=>Added lachlan
    Fri 22 Apr 2016 11:08:44 PM UTCrik5SummaryFLTK can segfault after using copyobj=>FLTK can segfault after using legend
    Fri 22 Apr 2016 04:52:32 PM UTCrik5StatusNone=>Confirmed

    Back to the top


    Powered by Savane 3.1-cleanup1