Re: [lmi] [lmi-commits] master 3b168ba 2/3: Use warning() + alarum() for schema validation failure

[Greg Chicares]

On 2018-07-20 11:03, Vadim Zeitlin wrote:
> On Thu, 19 Jul 2018 13:07:01 -0400 (EDT) Greg Chicares <address@hidden> wrote:
[...]
> GC>     warning() uses wxMessageBox(), which does not necessarily work during
> GC>     lmi startup or shutdown,
> 
>  wxMessageBox() is supposed to always work, including during startup and
> shutdown, with the only exception of Unix when using X11 and there is no
> available display. Do we have any example of a situation in which it
> doesn't work?

// WX !! An exception thrown anywhere in this function, even right
// before the 'return' statement at the end, either causes a crash
// (wx-2.5.1) or gets caught by OnUnhandledException() (which loses
// exception information) instead of by OnExceptionInMainLoop().
// Therefore, exceptions must be trapped explicitly.
//
bool Skeleton::OnInit()

This is one of four such problems marked inline:

skeleton.cpp:// WX !! An exception thrown here does not get caught gracefully, 
e.g.
skeleton.cpp:// WX !! An exception thrown anywhere in this function, even right
skeleton.cpp:// WX !! The wx exception-handling code doesn't seem to permit
view_ex.tpp:// WX !! Exceptions thrown here seem to be regenerated ad infinitum.

Notably, I find no direct evidence of any problem with wxMessageBox()
per se: these problems might have been caused by exception handling
in really old wx versions.

I'm not sure where this comment on safe_message_alert() came from:
  /// Show a message reliably, even before initialization has finished
  /// or after termination has begun.
AFAIK, it could have been either ancient wx or ancient msw documentation.
Even though it seems to suggest that wxMessageBox() may not work in such
circumstances, it might actually mean that even the native ::MessageBox()
wouldn't work. A long time ago, msw documentation gave very particular
instructions for calling ::MessageBox() in a way that was guaranteed to
work during startup and termination, and even in case of complete resource
exhaustion, and I believe I implemented safe_message_alert() for msw in
accordance with those instructions.

> GC>     and has been observed to segfault with
> GC>     extremely long strings, such as schema validation might emit (see
> GC>       https://savannah.nongnu.org/bugs/?20240
> GC>     and the code removed 20180213T1240Z by commit e3c3d922).
> 
>  I thought this bug was closed because it couldn't be reproduced and so it
> seems a bit strange to take it into account.

I thought no one had spent the time to investigate it thoroughly,
which doesn't prove that there never was a problem. I certainly
did observe an abrupt termination no later than November 2005,
which is why I made this change:

http://cvs.savannah.gnu.org/viewvc/lmi/lmi/census_view.cpp?view=diff&r1=1.36&r2=1.37

The offending line 1226:
  throw std::range_error(error.str());
was a direct 'throw', not an indirect 'fatal_error() << long_string',
so the cause cannot have been the "alert" classes. Furthermore, it
seems that wxMessageBox() cannot have been the problem, because the
solution was 'warning() << long_string', which calls wxMessageBox().
The problem must have been somewhere in this chain:
  throw long_string;
  exception propagated by ancient compiler, probably gcc-2.95
  exception caught somehow, by ancient lmi+wx code
  exception displayed by ::MessageBoxA
all running under msw-2000 or msw-xp.

> I really don't think it's
> acceptable for wxMessageBox() to segfault, for any input length (it might
> truncate the string, in the worst case, but still not segfault), so, again,
> I'd like to know if we have any way to reproduce this problem (which would
> help to fix it)?

I'd accept that no error is reproducible if we demonstrate that
calling code like this:
  std::string s = {some initializer of size at least one megabyte);
  throw std::runtime_error(s);
  warning() << s << LMI_FLUSH;
  alarum() << s << LMI_FLUSH;
at each of the "WX !!" points above, and at both the earliest and
the latest possible moment in startup and termination, always
behaves as desired.

If (ideally) we had that in the automated GUI test, then maybe we
could safely reconsider the implementation of safe_message_alert()
as well.

>  I think showing 2 message boxes in a row like this is a usability bug and
> would very much prefer to avoid it. If the problem with wxMessageBox() can
> be reproduced, then it can probably be fixed as well. If it can't, I think
> the best would be to use wxRichMessageDialog for warning() and should just
> a short excerpt of the message (until the separating blank line?) in the
> main part of the dialog and the long text in the part that can be expanded
> by the user to see the details.

http://docs.wxwidgets.org/3.1/classwx_rich_message_dialog.html
| Notice that currently the native dialog is used only under MSW when using
| Vista or later Windows version. Elsewhere, or for older versions of Windows,
| a generic implementation which is less familiar to the users is used. Because
| of this it's recommended to use this class only if you do need its extra
| functionality and use wxMessageDialog which does have native implementation
| under all platforms otherwise.

Apparently msw is favored, and a program that uses wxRichMessageDialog
would work less well on a free operating system: that doesn't seem
appropriate for free software hosted by the FSF.

>  Please let me know if I can help with really fixing the problem, instead
> of using this (IMNSHO pretty ugly) workaround with 2 dialogs.

Not this month. The code-freeze date for this release is Monday, so
  ugly + safe = releasable with confidence
  prettier + unproven = unwise risk

PDF pagination is much more important and urgent. But we can
certainly return to this later.