[Octave-bug-tracker] [bug #52904] "mesh" with input array of "logical" causes error

[Dan Sebald]

Follow-up Comment #11, bug #52904 (project octave):

The changeset works here and looks like the proper thing to do (but may need
adjustment); if the initialization fails, delete the graphics object and
error.  And there is no longer a graphics t.k. redraw error:


octave:1> h = mesh(zeros(25)==0);
error: invalid value for array property "zdata"
error: __go_surface__: unable to create graphics handle
error: called from
    surface>__surface__ at line 189 column 5
    surface at line 63 column 19
    mesh at line 77 column 12


and there are empty axes, which I believe was not happening prior to your
changeset, so nice work.

There are a couple other functions after the change that aren't "sure-thing"
use of the handle h (the h.value() is pretty innocuous).


  xcreatefcn (h);
  xinitialize (h);

  retval = h.value ();


Do those need some checks and cleanup as well?  As I understand it, I would
say, "Yes, these need consideration as well."

Please clarify that my understanding of the try/catch is correct.  So,
something that is within the try/catch will attempt the code, and if there is
an error() somewhere within that attempt, the error() is queued rather than
immediately executed.  Otherwise, there could be a problem if the error() path
is taken immediately and doesn't come back to the calling location, i.e., the
code won't have a chance to clean up any memory that has been assigned and
remove things from lists, etc.  (Typically, one doesn't put things on
container lists until all is verified correct, rather than put something on a
list then take it off if there is a problem, but that's fine.)  I do see an
error() inside do_make_graphics_handle():


  if (! bgo)
    error ("gh_manager::do_make_graphics_handle: invalid object type '%s'",
           go_name.c_str ());


which is an example of what I'm pointing out: if error() were to not return,
there'd potentially be some memory unaccounted for and leak...maybe not in
this particular case, but generally speaking.

OK, so the xset() calls the base properties initialization, and that is where
the current example fails, i.e., that "logical" class is not acceptable.  Your
changeset covers that case, good.

Now, what about xcreatefcn()?  I assume that routine will look at what the
user-set property


     createfcn = [](0x0)


is and if there is a valid function there, execute it.  Given that is a
user-defined function, we can reasonably expect that to fail in the course of
a user's debugging.  In fact, I can implement a fail:


octave:1> function [a] = badfunction()
>   this = isbad();
> end
octave:2> set(groot,"defaultaxescreatefcn", @badfunction);
octave:3> axes()
error: 'isbad' undefined near line 2 column 10
execution error in graphics callback function


This looks to have no apparent crash problems, but understand that it's likely
that what is after this, i.e., 


  xinitialize (h);
  retval = h.value ();


aren't run, so we are still left with the question of whether we should have
an incompletely initialized graphics object hanging around.  So, is this
another case in which we destroy the graphics object at creation if the
createfcn property is not successful?

And xinitialize(); how about that one?  Is that one pretty safe?  I see that
xinitialize() is the following:


static void
xinitialize (const graphics_handle& h)
{
  graphics_object go = gh_manager::get_object (h);

  if (go)
    go.initialize ();
}


and the only non-base-graphics-object I see with an initialize is


void
axes::initialize (const graphics_object& go)
{
  base_graphics_object::initialize (go);

  xinitialize (xproperties.get_title ());
  xinitialize (xproperties.get_xlabel ());
  xinitialize (xproperties.get_ylabel ());
  xinitialize (xproperties.get_zlabel ());

  xproperties.sync_positions ();
}


Is it possible that the user could pass into the routine properties for
"title", "xlabel", "ylabel" and "zlabel" and those might fail along the way if
they are poorly formated strings or have bad non-ascii characters?

Anyway, my thinking right now is we should likely try/catch at least the
xcreatefcn(h) as well.

Thoughts?


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?52904>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/