bugGNU Octave - Bugs: bug #53513, Deleting an uicontextmenu object...

 
 

bug #53513: Deleting an uicontextmenu object does not update uicontexmenu properties immediately

Submitter:  Dmitri A. Sergatskov <dasergatskov>
Submitted:  Thu 29 Mar 2018 05:00:14 PM UTC
   
 
Category:  Plotting with OpenGL Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
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

Sun 01 Nov 2020 12:27:15 PM UTC, comment #37: 

Thanks for clarifying.

I tested the original example 200 times in a loop on Windows with no failure.

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Sat 31 Oct 2020 08:05:50 PM UTC, comment #36: 

@Markus: Thanks for testing and pushing the first patch.

>> "Some more work is needed."

Sorry, it should be read as "disregard this first attempt" :-). Indeed the second patch has nothing to do with this bug, and I'll post eventual updates in the relevant bug reports.

Pantxo Diribarne <pantxo>
Group Member
Sat 31 Oct 2020 11:37:10 AM UTC, comment #35: 

I ran a script with the following commands in Matlab R2020b:

hf = figure();
cm = uicontextmenu(hf);
set(cm, 'DeleteFcn', @(h,e) disp(get(get(h, 'Parent'), 'UiContextMenu')));
set(hf, 'UiContextMenu', cm);

disp(get(hf, 'UiContextMenu'));

delete(cm);


The output is:

>> test_uicontextmenu_delete
  ContextMenu with properties:

    ContextMenuOpeningFcn: ''
                 Children: [0×0 GraphicsPlaceholder]

  Use get to show all properties

  0×0 empty GraphicsPlaceholder array.


So the "uicontextmenu" property of the figure is already cleared when the "deletefcn" executes.

The order in the patch from comment #33 seems to match what Matlab does.

With the patch, I ran the BIST that previously failed intermittently 200 times in a loop and it never failed for me.
So I pushed the patch to stable here:
https://hg.savannah.gnu.org/hgweb/octave/rev/df307ee1fb30

Wrt the second patch: I lost track of the different issues. Is this patch meant to solve this bug or a different bug?
When you write "Some more work is needed.", does that mean we should not use the patch? Or do you mean that we should use the patch, but it doesn't fix all issues, and we'd need additional patches to fix all of them?

Markus Mützel <mmuetzel>
Group administrator
Thu 29 Oct 2020 03:32:13 PM UTC, comment #34: 

@Pantxo: If I understand the patch in comment #33 correctly, the "uicontextmenu" property of its "parent" would be reset before the deletefcn of the uicontextmenu object executes.
With the previous patch, it would have been the other way round.

Is there a correct order in which this should happen?

Markus Mützel <mmuetzel>
Group administrator
Wed 28 Oct 2020 10:48:14 PM UTC, comment #33: 

How about adding an updater to the "beingdeleted" property of uicontextmenu objects and doing the cleanup there (see attached patch)? I think this one is safe to push to stable.

As for the second patch, it should definitely not go to stable:  after testing a bit more I can see that it uncovers many scary warnings about invalid objects (testing bug #48186) ... Some more work is needed.



(file #50157)

Pantxo Diribarne <pantxo>
Group Member
Wed 28 Oct 2020 04:24:13 PM UTC, comment #32: 

I haven't tested yet. But that looks good to me.
I wonder whether it would be cleaner to override the `finalize` method of `uicontextmenu` instead of having the specific code in the general `gh_manager::free` function.
That could probably contain basically the code that you add to `gh_manager::free` followed by `base_graphics_object::finalize (go);`.
Would that also work? From what I can tell, that wouldn't change the order of execution.

I haven't looked at the second patch yet.

A more general question: Should this go to stable? The graphics code is quite complicated and there might be side effects we don't see atm. But I trust your assessment.

Markus Mützel <mmuetzel>
Group administrator
Sun 25 Oct 2020 07:41:37 PM UTC, comment #31: 

Sorry, I have completely misinterpreted your previous post :-). Yes, if one is a duplicate of the other lets close one et check we don't loose info.

The first attached patch seems to work for me. It moves the logic for clearing the uicontextmenu properties of dependent objects from the uicontextmenu destructor to gh_manager::free (don't know if this is the best place).

After the previous change, we no more need the destructor to be called synchronously, and I think we no more need to have synchronous finalize operation in the GUI (no other GUI object had a non default destructor).
So I also attached another patch in which I made ObjectProxy::finalize asynchronous again, which clears bug #58678 (and I can't reproduce its brother bug #48186 anymore).




(file #50096, file #50097)

Pantxo Diribarne <pantxo>
Group Member
Sun 25 Oct 2020 05:29:23 PM UTC, comment #30: 

I agree that bug #58678 is a separate issue.
But bug #58403 seems to be a duplicate of this bug. Maybe we should move the information from that bug over here.

Re-titling this bug following your suggestion.

Markus Mützel <mmuetzel>
Group administrator
Sun 25 Oct 2020 04:57:56 PM UTC, comment #29: 

I think there are two different issues: bug #58678 is about potential deadlock due to our for approach for synchronizing thread, this one is about the same approach failing to synchronize threads in some rare situations. You could probably solve one without solving the other.

Why not just change the title of this bug to something like "Deleting an uicontextmenu object does not update uicontexmenu properties immediately"

Pantxo Diribarne <pantxo>
Group Member
Sun 25 Oct 2020 04:44:39 PM UTC, comment #28: 

The summary of this bug is very unspecific. There is also bug #58403 which seems to be about the original issue that was discussed here.

Should we close one of these bugs in favor of the other?

The test failure from comment #0 still occurs intermittently. E.g.: http://buildbot.octave.org:8010/#/builders/31/builds/166/steps/7/logs/stdio


Markus Mützel <mmuetzel>
Group administrator
Sun 25 Oct 2020 09:56:36 AM UTC, comment #27: 

@Markus: I have not had much time to dig further into this issue lately. Basically, the changes in the patch from comment #24 are already implemented in the current sources but this approach is not reliable due our mutexes being recursive: doing a single "gh_manager::unlock ()" does not leave the mutex unlocked if it had been locked twice. This demonstrated by bug #58678.

Pantxo Diribarne <pantxo>
Group Member
Sun 25 Oct 2020 09:15:06 AM UTC, comment #26: 

@pantxo: Now that we know that the mutexes that we use are recursive, is re-locking the mutex still an issue?

The files in "libgui" have been re-organized. The patch from comment #24 no longer applies.

I still occasionally get the error from comment #22. Do we still need to change something here?

Markus Mützel <mmuetzel>
Group administrator
Fri 27 Apr 2018 08:50:22 PM UTC, comment #25: 

I have now seen this under (Ubuntu) linux. Not sure if it is the same issue, because the test failure is now for "tightinset" and not for "uicontextmenu" any more. The fail happend with "make test", but I could NOT reproduce it from within Octave via a loop on "test graphics.cc-tst".


>>>>> processing /opt/octave-4.3.92/libinterp/corefcn/graphics.cc-tst
***** testif HAVE_OPENGL, HAVE_FLTK; have_window_system
 hf = figure ("visible", "off");
 graphics_toolkit (hf, "fltk");
 fpos = get (hf, "position");
 unwind_protect
   plot (rand (3));
   position = get (gca, "position");
   outerposition = get (gca, "outerposition");
   looseinset = get (gca, "looseinset");
   tightinset = get (gca, "tightinset");
   set (hf, "position", [fpos(1:2), 2*fpos(3:4)]);
   set (hf, "position", fpos);
   assert (get (gca, "outerposition"), outerposition, 0.001);
   assert (get (gca, "position"), position, 0.001);
   assert (get (gca, "looseinset"), looseinset, 0.001);
   assert (get (gca, "tightinset"), tightinset, 0.001);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect
!!!!! test failed
ASSERT errors for:  assert (get (gca, "tightinset"),tightinset,0.001)

  Location  |  Observed  |  Expected  |  Reason
    (1)        0.053571     0.042857     Abs err 0.010714 exceeds tol 0.001 by 0.01


Sorry, I haven't tested the patch from comment #24 so far.

Hartmut <hardy>
Wed 25 Apr 2018 08:14:30 AM UTC, comment #24: 

The attached patch fixes both this bug and failing getframe tests. The idea is to make sure grapĥics_toolkit::initialize/finalize methods are ran synchronously. The patch is not ready to be pushed since I need to find a way to re-lock the mutex in the interpreter thread if it was previously locked (and I don't know how to do that ATM).

Can you test the patch?

(file #44020)

Pantxo Diribarne <pantxo>
Group Member
Mon 23 Apr 2018 03:18:08 PM UTC, comment #23: 

It's not an issue that will be resolved for the 4.4 release.  As your testing shows, it is fairly infrequent (<=2%).  Additionally, it is only a failing BIST test which is not that serious.

The real fix requires a re-architecting of cross-thread operations which is much too big a project just before a release.

Rik <rik5>
Group administrator
Mon 23 Apr 2018 11:57:21 AM UTC, comment #22: 

I get this (same?) failure with Octave 4.3.91 on Win10:


>>>>> processing C:\Octave\OCTAVE~1.91\share\octave\4.3.91\etc\tests\libinterp\corefcn\graphics.cc-tst
***** test
 hf = figure ("visible", "off");
 hax = axes ("parent", hf);
 unwind_protect
   hctx1 = uicontextmenu ("parent", hf);
   hctx2 = uicontextmenu ("parent", hf);
   set (hf, "uicontextmenu", hctx2);
   set (hax, "uicontextmenu", hctx2);
   assert (get (hf, "uicontextmenu"), hctx2);
   assert (get (hax, "uicontextmenu"), hctx2);
   assert (get (hf, "children"), [hctx2; hctx1; hax]);
   delete (hctx2);
   pause (.005);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hax, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
   set (hf, "uicontextmenu", hctx1);
   assert (get (hf, "uicontextmenu"), hctx1);
   set (hf, "uicontextmenu", []);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect;
!!!!! test failed
ASSERT errors for:  assert (get (hf, "uicontextmenu"),[])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(0x0)      Dimensions don't match


I can also reproduce this failure with:

## go to the correct folder beforehand
for n=1:50
  n
  test graphics.cc-tst
endfor


In my case 1 out of the 50 goes produced a test failure.

Is this an issue for the 4.4 release then?

Hartmut <hardy>
Mon 16 Apr 2018 06:37:00 PM UTC, comment #21: 

I agree that this needs a full re-think and re-architecting.  When that task is tackled we should look to use either Qt-provided or C++11 facilities for the actual operations (like mutexes, etc.) rather than writing our own.

Rik <rik5>
Group administrator
Sat 14 Apr 2018 05:00:39 PM UTC, comment #20: 

I agree Pantxo, some fundamental redesign along the lines of hand-shaking of some sort is needed.  That's not a minor project--or at least not one that can be done in a week before a release.  Seems like a good summer project, and it requires coordination between people who know both sides of the threads.

As you mentioned, in this case the asynchronous action is necessary whereas in other instances it may not be.  Another example of the forced-synchronous approach is what I did some time ago with the inputdlg() family of routines.  I forced synchronous timing between interpreter (T2) and GUI (T1) in a really crude way by putting T2 to sleep while the user is entering data in the inputdlg.  Once the user closes the modal dialog, T1 will put the data in the desired location then wake up T2.  So, basically T2 can't do anything when the inputdlgs are busy.  Crude, but it ensures no thread problems.  There wasn't much alternative at the time because no design was in place for communication between the components.  So going forward, if ever Octave GUI-building (i.e., one can build their own GUI within Octave...not the "Octave GUI" we commonly refer to) the inputdlg approach I described is a no-go.  There has to be active threads on both sides of the divide for something like that.  Just something to keep in mind.

Dan Sebald <sebald>
Sat 14 Apr 2018 11:38:27 AM UTC, comment #19: 

I'll try to sketch the way Octave interpreter thread (T2 hereafter) interacts with the Qt's GUI thread (T1 hereafter).

When a figure (or an ui* object) needs to be initialized, updated or finalized, the ObjectProxy (in T2) sends a signal. Depending on the context, we connect the signal to ObjectFactory::createObject, in case of initialization, or to the corresponding Object::update/finalize slots (all in T1). Since signals and slots live in two different threads, they will by default be treated asynchronously in two different Qt event queues.

The first consequence is that all ObjectProxy::initialize/update/finalize methods (in T2)  will generally return before the operations in T1 have been successfully done. This bug and bug #53644 are illustrations of situations where we can't live with this asynchronous behavior.

For printing (and getting pixels) we have solved the above issue by forcing the signaling methods and corresponding slots to run synchronously. Basically we use a Qt::BlockingQueuedConnection which locks the emitting thread (T2) until the slot have been processed in T1.

Unfortunately we cannot apply this method to ObjectProxy::initialize/update/finalize since the corresponding slots rely on the interpreter thread T2 being awake:

  • we need to be able to change some values in the graphics_object properties (instanciated from T2). This is either done directly (we lock a mutex and call graphics_objet::properties::set_xxx) or we post a graphics event which will be processed later in T2.
  • we need to be able to ask the interpreter to run graphics callbacks. This is done by posting a graphics event.


For the sake of completeness, the graphics event queue is implemented using readline and is supposed to be flushed only when "drawnow" (and a few other functions) is requested.

To summarize, I think the question is "how do we make sure signaling methods in ObjectProxy don't return before their corresponding slots actually do?". ATM I only can say what we cannot do: lock T2 using a mutex or a "while (condition_not_met) {}".

Pantxo Diribarne <pantxo>
Group Member
Sat 14 Apr 2018 06:11:01 AM UTC, comment #18: 

Have a look at this ASAN analysis

https://savannah.gnu.org/bugs/?53644

I think that bug is also a timing issue.

Dan Sebald <sebald>
Wed 11 Apr 2018 05:34:46 PM UTC, comment #17: 

I put a conditional statement in bold because I don't know how this Qt toolkit and the multithreading is designed (nor do I care to know at this point).  Member functions can be executed from one thread or another (the restriction being that graphics can only be done in Qt's main thread), but if you are saying Figure code is only executed from one thread, then that is fine.  The point still holds about semaphores, though.  Even in the graphics thread, using Qt's signal/slots rather than blocking the execution of code might be better.

From Canvas.cc:


            // FIXME: should we use signal/slot mechanism instead of
            //        directly calling parent fig methods


From ObjectProxy.cc:


    // The ObjectProxy is generally ran from the interpreter thread
    // while the actual Figure (Object) lives in the gui thread. The
    // following ensures synchronous execution of the Figure method and
    // allows retrieving a return value.



    // FIXME: The following may fail for obscure reasons, see bug #53328.
    //        In absence of a solution, we retry twice before calling error().


Then there are callback methods:


    void close_figure_callback (void);
    void copy_figure_callback (const std::string& format);
    void save_figure_callback (const std::string& file);


which I presume are coming from the worker thread, not the main thread.

The Figure::updateFigureToolBarAndMenuBar() code I listed below:  Can you see how difficult it is to tell that "m_blockUpdates = true;" is meant to stop some recursion?  Where is the "if (m_blockUpdates) return;"?  It's in some other function.  One has to infer there is a possible recursion.  The alternative interpretation is that there is something asynchronous occurring that might do an update if that m_blockUpdates is not set.

Folks want to ditch other toolkits, but Qt toolkit hasn't become robust over the course of half a decade and several release versions.  These little added delays, multiple tries, etc. simply are wasting effort in my opinion.

Dan Sebald <sebald>
Wed 11 Apr 2018 07:51:58 AM UTC, comment #16: 

@Dan: I don't understand. m_blockUpdate is a private member variable and is thus only accessible from the Object itself (a Figure in your example). In this regard I don't see how the scenarios you describe could ever happen and how there could be any asynchronicity here (recursion at worst, and it is what m_blockUpdates is supposed to be preventing).



Pantxo Diribarne <pantxo>
Group Member
Tue 10 Apr 2018 04:15:13 PM UTC, comment #15: 

That makes sense.  Eventually we need to work out a good architectural solution to all of the cross-thread work.

One theoretical solution would be some sort of semaphor so that each side could be certain the other side had completed all of its actions.

Just for testing purposes, what happens if you put in a call to sleep somewhere in the delete() function stack so that the GUI or the CLI has a chance to complete all of its operations?  Does that also fix bug #53604?

Rik <rik5>
Group administrator
Tue 10 Apr 2018 04:10:00 PM UTC, comment #14: 

What Pantxo is describing in Comment #13 and Rik in Comment #8 is what I meant by Comment #4.  As an example, I look at the code and see pretty extensive use of the variable "m_blockUpdates" throughout.  That variable is used in Figure::update (int pId) as follows:


    if (m_blockUpdates)
      return;


and I see a process ID as an input, so I'm wondering (rhetorically) if this routine is asynchronous.  If that is the case, that m_blockUpdates used all about won't do much.  The m_blockUpdates variable approach will prevent recursive calls, but not asynchronous issues.  For example, hypothetically speaking, imagine if this short routine is called asynchronously:


  void
  Figure::updateFigureToolBarAndMenuBar (void)
  {
    if (m_mouseModeGroup)
      {
        m_blockUpdates = true;
        m_mouseModeGroup->setMode (mouseMode ());
        m_blockUpdates = false;
      }
  }


and some other thread is presently doing some sort of update for which it has already set m_blockUpdates to true.  Well, in theory this routine could set m_blockUpdates "true", finish quickly and then a few machine cycles later set m_blockUpdates to "false" while the other thread is still working and has not yet set m_blockUpdates to "false".  So m_blockUpdates would be reset prematurely and there is a little window of time where m_blockUpdates is "false" when it really should be "true".  This is why I mentioned "max load".  When the CPU is chugging away, a "short window of time" becomes a "big window of time".

Sometimes one can get away with this approach if there is only, say, one location where m_blockUpdates is set to "false" while maybe multiple places the variable can be set "true".  Or use a MUTEX to hold off executing these routines until the data is free to be modified without consequence.  Years ago I had talked about "comm link" where I prototyped communication between core/GUI at one finely controlled location--carefully analyzed reference count modifications and so on to test.  That's a lot of work, and spreading this throughout the code seems even more difficult.  I've only glanced at this code, so I don't know what the asynchronous issues are, if any, but I'm just saying if this is meant to address asynchronous issues, a redesign might be due.

Dan Sebald <sebald>
Tue 10 Apr 2018 02:47:43 PM UTC, comment #13: 

I think I identified the issue. Before the following cset, the test used to pass

http://hg.savannah.gnu.org/hgweb/octave/rev/b6aea95a7bf9

The problem is that we rely on ref counting for graphics_object deletion (and further cleanup). Before the above cset, the toolkit Object, which lives in the GUI thread, did not have a reference to its grapĥics_object counterpart and gh_manager::free took care of deleting the last reference to the graphics_object. Now the asynchronous nature of the "graphics_toolkit::finalize" method makes the object eventually be deleted after "delete" has returned, hence the cleanup (remove the uicontextmenu from its dependent objects) not being already done when delete returns.

I don't have a solution at the moment but we now have an interpretation.

Pantxo Diribarne <pantxo>
Group Member
Wed 04 Apr 2018 04:26:21 PM UTC, comment #12: 

Marking bug as Postponed.  The delay works to have the test suite pass for the 4.4 release.  But we should really determine the root cause of the problem.

Rik <rik5>
Group administrator
Tue 03 Apr 2018 12:36:52 AM UTC, comment #11: 

Yes. it works on my desktop as well.
Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 03 Apr 2018 12:22:38 AM UTC, comment #10: 

Okay, this isn't the best but I put in a 5ms delay after the delete() call.  The buildbots seem much happier now.  Does it work on your home system now?

Rik <rik5>
Group administrator
Mon 02 Apr 2018 04:42:12 PM UTC, comment #9: 

I tried to repeat the test on command line 10 times and I got 10 failures...


!!!!! test failed
ASSERT errors for:  assert (get (hf, "uicontextmenu"),[])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(0x0)      Dimensions don't match
octave:9> test libinterp/corefcn/graphics.cc-tst
warning: axis: omitting non-positive data in log plot
warning: Non-negative limit for logarithmic axis ignored
warning: Non-positive limit for logarithmic axis ignored
warning: axis: omitting non-positive data in log plot
warning: Non-positive limit for logarithmic axis ignored
***** test
 hf = figure ("visible", "off");
 hax = axes ("parent", hf);
 unwind_protect
   hctx1 = uicontextmenu ("parent", hf);
   hctx2 = uicontextmenu ("parent", hf);
   set (hf, "uicontextmenu", hctx2);
   set (hax, "uicontextmenu", hctx2);
   assert (get (hf, "uicontextmenu"), hctx2);
   assert (get (hax, "uicontextmenu"), hctx2);
   assert (get (hf, "children"), [hctx2; hctx1; hax]);
   delete (hctx2);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hax, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
   set (hf, "uicontextmenu", hctx1);
   assert (get (hf, "uicontextmenu"), hctx1);
   set (hf, "uicontextmenu", []);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect;
!!!!! test failed
ASSERT errors for:  assert (get (hf, "uicontextmenu"),[])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(0x0)      Dimensions don't match



Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 02 Apr 2018 04:38:29 PM UTC, comment #8: 

There is something intricate going on.  I see that the buildbots are failing, but I can run 'test graphics.cc' in a loop without failure.  However, if I run 'make check' from a command line I do get an overall failure.

Rik <rik5>
Group administrator
Mon 02 Apr 2018 04:36:41 PM UTC, comment #7: 

My workstation 34a5a420208e (stable) tip as well as  buildbots
(dev) are still failing...


octave:1> test libinterp/corefcn/graphics.cc-tst
warning: axis: omitting non-positive data in log plot
warning: Non-negative limit for logarithmic axis ignored
warning: Non-positive limit for logarithmic axis ignored
warning: axis: omitting non-positive data in log plot
warning: Non-positive limit for logarithmic axis ignored
***** test
 hf = figure ("visible", "off");
 hax = axes ("parent", hf);
 unwind_protect
   hctx1 = uicontextmenu ("parent", hf);
   hctx2 = uicontextmenu ("parent", hf);
   set (hf, "uicontextmenu", hctx2);
   set (hax, "uicontextmenu", hctx2);
   assert (get (hf, "uicontextmenu"), hctx2);
   assert (get (hax, "uicontextmenu"), hctx2);
   assert (get (hf, "children"), [hctx2; hctx1; hax]);
   delete (hctx2);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hax, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
   set (hf, "uicontextmenu", hctx1);
   assert (get (hf, "uicontextmenu"), hctx1);
   set (hf, "uicontextmenu", []);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect;
!!!!! test failed
ASSERT errors for:  assert (get (hf, "uicontextmenu"),[])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(0x0)      Dimensions don't match
octave:2> __octave_config_info__ ("hg_id")
ans = 34a5a420208e


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 02 Apr 2018 04:19:10 PM UTC, comment #6: 

I just fixed another issue related to lint checking which appears to have resolved this bug for me.


~/wip/Projects_Mine/octave-dev: hlog -p -r 25081
changeset:   25081:d3baac355a2c
branch:      stable
user:        Rik <rik@octave.org>
date:        Sun Apr 01 22:16:24 2018 -0700
summary:     GLCanvas.cc: Catch exception by reference to silence -Wcatch-value warning (bug #53545).

diff -r 699fe9cd51ca -r d3baac355a2c libgui/graphics/GLCanvas.cc
--- a/libgui/graphics/GLCanvas.cc       Sun Apr 01 22:06:24 2018 -0700
+++ b/libgui/graphics/GLCanvas.cc       Sun Apr 01 22:16:24 2018 -0700
@@ -138,7 +138,7 @@ namespace QtHandles
             octave::gl2ps_print (figObj, file_cmd.toStdString (),
                                  term.toStdString ());
           }
-        catch (octave::execution_exception e)
+        catch (octave::execution_exception& e)
           {
             octave_link::post_exception (std::current_exception ());
             end_rendering ();


I ran the test script in comment #3 50 times with no failures.

Marking bug as Ready for Test.


Rik <rik5>
Group administrator
Fri 30 Mar 2018 10:06:52 PM UTC, comment #5: 

Agreed.  The delays are not a solution for the root problem.  Typically, I think, we've had issues cross-thread issues between the GUI and Octave thread.  But I don't think that is the case here.  This looks related to the delete procedure which we should be able to make deterministic.

Rik <rik5>
Group administrator
Fri 30 Mar 2018 09:52:33 PM UTC, comment #4: 

Adding arbitrary delays like this or like

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

are merely "papering-over" solutions to timing issues.  I could give lengthy details of specific examples, but suffice it to say one is never sure what happens when a system is at max load or whether re-pinging an operation is going to affect data integrity of the process one is waiting on to complete.  Unfortunately, it turns a bug into something rare and obscure but never total confidence something won't fail.

Dan Sebald <sebald>
Fri 30 Mar 2018 09:24:22 PM UTC, comment #3: 

A little debugging shows that I do not have this problem, even in the GUI, if I am using the FLTK toolkit.  This is something unique to the Qt toolkit.  I extracted the one failing test in to a new file tst_ui.m which is attached.  You can use 'test tst_ui' to run just this one problem test.

At least part of the issue is related to the new QTOFFSCREEN rendering that we are using for invisible figures.  If I change the first line of the file to make the figure visible then 75% of the time the test passes.  That still sucks, but it is an indication that whatever it is gets far worse when the figure is hidden.  Just guessing, but when the figure is hidden graphics operations take place much quicker which might be showing what race condition exists.

Finally, it only takes one very small delay, I used a millisecond, after the call to delete to get this to work.


%!test
%! hf = figure ("visible", "off");
%! #hf = figure ("visible", "on");
%! hax = axes ("parent", hf);
%! unwind_protect
%!   hctx1 = uicontextmenu ("parent", hf);
%!   hctx2 = uicontextmenu ("parent", hf);
%!   set (hf, "uicontextmenu", hctx2);
%!   set (hax, "uicontextmenu", hctx2);
%!   assert (get (hf, "uicontextmenu"), hctx2);
%!   assert (get (hax, "uicontextmenu"), hctx2);
%!   assert (get (hf, "children"), [hctx2; hctx1; hax]);
%!   delete (hctx2);
%!   pause (0.001);   # <-- Required for Qt toolkit
%!   assert (get (hf, "uicontextmenu"), []);
%!   assert (get (hax, "uicontextmenu"), []);
%!   assert (get (hf, "children"), [hctx1; hax]);
%!   set (hf, "uicontextmenu", hctx1);
%!   assert (get (hf, "uicontextmenu"), hctx1);
%!   set (hf, "uicontextmenu", []);
%!   assert (get (hf, "uicontextmenu"), []);
%!   assert (get (hf, "children"), [hctx1; hax]);
%! unwind_protect_cleanup
%!   close (hf);
%! end_unwind_protect;


The assert which fires is saying that the uicontextmenu entry for the figure is not empty, as it should be after the delete, but still contains a 1x1 entry (presumably this is hctx2).

This leads me to believe that there is something amiss with the way we implement delete.  The handle that we are deleting, hctx2, is destroyed immediately.  But this will cause other callbacks to fire which then remove hctx2 from the "children" property of the parent of hctx2.  I think control must be returning to the command line before those updates have had a chance to complete.

How is the event queue flushed?  Is only one cycle done each time Octave returns to the command prompt?  Or is the event queue fully emptied before returning to the command prompt.  The second choice seems better because operations, like delete, could spawn new events which need to be processed.


(file #43733)

Rik <rik5>
Group administrator
Fri 30 Mar 2018 07:32:28 PM UTC, comment #2: 

I wonder if we have an issue with the way the schema database for graphics properties is implemented.  Maybe updates are not atomic?

Rik <rik5>
Group administrator
Thu 29 Mar 2018 09:03:36 PM UTC, comment #1: 

Similarly to "getframe" issue, if I add pause(0.1) after each
set() and delete() then I do not get any failures.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 29 Mar 2018 05:00:14 PM UTC, original submission:  

With
d298a0734d85 (stable)

(also with dev -- see buildbots)

octave:1> test libinterp/corefcn/graphics.cc-tst
warning: axis: omitting non-positive data in log plot
warning: Non-negative limit for logarithmic axis ignored
warning: Non-positive limit for logarithmic axis ignored
warning: axis: omitting non-positive data in log plot
warning: Non-positive limit for logarithmic axis ignored
*** test
 hf = figure ("visible", "off");
 hax = axes ("parent", hf);
 unwind_protect
   hctx1 = uicontextmenu ("parent", hf);
   hctx2 = uicontextmenu ("parent", hf);
   set (hf, "uicontextmenu", hctx2);
   set (hax, "uicontextmenu", hctx2);
   assert (get (hf, "uicontextmenu"), hctx2);
   assert (get (hax, "uicontextmenu"), hctx2);
   assert (get (hf, "children"), [hctx2; hctx1; hax]);
   delete (hctx2);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hax, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
   set (hf, "uicontextmenu", hctx1);
   assert (get (hf, "uicontextmenu"), hctx1);
   set (hf, "uicontextmenu", []);
   assert (get (hf, "uicontextmenu"), []);
   assert (get (hf, "children"), [hctx1; hax]);
 unwind_protect_cleanup
   close (hf);
 end_unwind_protect;
!!!!! test failed
ASSERT errors for:  assert (get (hf, "uicontextmenu"),[])

  Location  |  Observed  |  Expected  |  Reason
     .          O(1x1)       E(0x0)      Dimensions don't match


I get it with ~100% reproducibility.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50157:  bug53513_3.patch added by pantxo (3KiB - text/x-patch)
file #50096:  bug53513.patch added by pantxo (5KiB - text/x-patch)
file #50097:  bug58678.patch added by pantxo (3KiB - text/x-patch)
file #44020:  blocking_init_final2.patch added by pantxo (6KiB - text/x-patch)
file #43733:  tst_ui.m added by rik5 (909B - text/x-matlab)

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by pantxo (Posted a comment)
  • -email is unavailable- added by sebald (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by dasergatskov (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 20 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-11-01 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-10-31 mmuetzel StatusPatch Submitted Ready For Test
        Operating SystemGNU/Linux Any
    2020-10-28 pantxo Attached File- Added bug53513_3.patch, #50157
    2020-10-26 mmuetzel Dependencies- bugs #58403 is dependent
    2020-10-25 pantxo Attached File- Added bug53513.patch, #50096
        Attached File- Added bug58678.patch, #50097
    2020-10-25 mmuetzel Summarygraphics.cc-tst FAIL Deleting an uicontextmenu object does not update uicontexmenu properties immediately
    2020-10-25 pantxo CategoryInterpreter Plotting with OpenGL
    2018-04-25 pantxo Attached File- Added blocking_init_final2.patch, #44020
        StatusPostponed Patch Submitted
    2018-04-10 rik5 Dependencies- bugs #53604 is dependent
    2018-04-04 rik5 StatusReady For Test Postponed
    2018-04-02 rik5 StatusConfirmed Ready For Test
    2018-03-30 rik5 Attached File- Added tst_ui.m, #43733
        Carbon-Copy- Added pantxo
        Carbon-Copy- Added sebald
    2018-03-30 rik5 Carbon-Copy- Added jwe
    2018-03-29 rik5 StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code