[PATCH] qtapp: fix a startup crash with an unparsable config file (fixes #5453)

[Matt Harbison]

# HG changeset patch
# User Matt Harbison <matt_h...@yahoo.com>
# Date 1658517491 14400
# Fri Jul 22 15:18:11 2022 -0400
# Branch stable
# Node ID b9c0b7bffdb6773f339068b1790039cf1b66c9f8
# Parent 65359680e4dbfd5e320d0de744f7f21fdb82f4dc
qtapp: fix a startup crash with an unparsable config file (fixes #5453)

It looks like this broke way back in dbc574601ef9. I didn't bother to look to
see where this used to be called, but as of now it is only called for a
`ParseError`, so it is way more elaborate than needed. I'm also not sure how it
used to get the second argument, because even iterating over the exception in
py2 only yielded the message as if cast to `str`. So since this is the only
exception passed, reach into the location field.

This may all be dead code. I tried injecting config file errors, and everything
got reported as a `ConfigError` with the normal crashreport dialog. I had to
manually raise a `ParseError` to ensure that the dialog with the option to fix
the config file is properly displayed.

diff --git a/tortoisehg/hgqt/qtapp.py b/tortoisehg/hgqt/qtapp.py
--- a/tortoisehg/hgqt/qtapp.py
+++ b/tortoisehg/hgqt/qtapp.py
@@ -107,9 +107,11 @@
}

def earlyExceptionMsgBox(e):
+ # type: (error.ParseError) -> None
"""Show message for recoverable error before the QApplication is started"""
opts = {'cmd': ' '.join(sys.argv[1:]),
- 'values': e,
+ 'values': [hglib.tounicode(str(e)),
+ hglib.tounicode(e.location) if e.location else u''],
'error': traceback.format_exc(),
'nofork': True}
errstring = _recoverableexc[e.__class__]
diff --git a/tortoisehg/hgqt/run.py b/tortoisehg/hgqt/run.py
--- a/tortoisehg/hgqt/run.py
+++ b/tortoisehg/hgqt/run.py
@@ -138,7 +138,7 @@
pdb.set_trace()
return _runcatch(u, args)
except error.ParseError as e:
- qtapp.earlyExceptionMsgBox(hglib.tounicode(str(e)))
+ qtapp.earlyExceptionMsgBox(e)
except SystemExit as e:
return e.code
except Exception as e:

[Yuya Nishihara]

Perhaps, we should instead do 'values': hglib.tounicode(bytes(e)) or
tounicode(forcebytestr(e)). Even though this function is kind of dead code,
it doesn't make sense that earlyExceptionMsgBox() requires a specific exception
type.

[Matt Harbison]

On Friday, July 22, 2022 at 9:16:50 PM UTC-4 Yuya Nishihara wrote:

On Fri, 22 Jul 2022 17:18:03 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_h...@yahoo.com>
> # Date 1658517491 14400
> # Fri Jul 22 15:18:11 2022 -0400
> # Branch stable
> # Node ID b9c0b7bffdb6773f339068b1790039cf1b66c9f8
> # Parent 65359680e4dbfd5e320d0de744f7f21fdb82f4dc
> qtapp: fix a startup crash with an unparsable config file (fixes #5453)
>

> diff --git a/tortoisehg/hgqt/qtapp.py b/tortoisehg/hgqt/qtapp.py
> --- a/tortoisehg/hgqt/qtapp.py
> +++ b/tortoisehg/hgqt/qtapp.py
> @@ -107,9 +107,11 @@
> }
>
> def earlyExceptionMsgBox(e):
> + # type: (error.ParseError) -> None
> """Show message for recoverable error before the QApplication is started"""
> opts = {'cmd': ' '.join(sys.argv[1:]),
> - 'values': e,
> + 'values': [hglib.tounicode(str(e)),
> + hglib.tounicode(e.location) if e.location else u''],

Perhaps, we should instead do 'values': hglib.tounicode(bytes(e)) or
tounicode(forcebytestr(e)). Even though this function is kind of dead code,
it doesn't make sense that earlyExceptionMsgBox() requires a specific exception
type.

I agree that it's silly to special case this, but I see at least 2 problems with this being generic.  First is, the location (filename and line number) used to open the bad file is lost when converting to bytes.  Second, it enumerates every character in the exception message in bugreport.ExceptionMsgBox.__init__ when it sees "%(arg" in the `_recoverableexc` value:

$ /c/Program\ Files/TortoiseHg/hg.exe debugshell
Python 3.9.12 (tags/v3.9.12:b28265d, Mar 23 2022, 23:52:46) [MSC v.1929 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from mercurial import error
>>> e = error.ParseError(b"msg goes here", location=b"foo/ba r:80")

# qtapp.earlyExceptionMsgBox()

>>> from tortoisehg.util import hglib

>>> vals = hglib.tounicode(bytes(e))
>>> print(vals)
msg goes here

# bugreport.ExceptionMsgBox.__init__()
>>> for i, v in enumerate(vals):
...     print("%d: %r" % (i, v))
...
0: 'm'
1: 's'
2: 'g'
3: ' '
4: 'g'
5: 'o'
6: 'e'
7: 's'
8: ' '
9: 'h'
10: 'e'
11: 'r'
12: 'e'

I know I've seen the second problem in the dialog on py2 (I forget exactly how it displayed, though it was clearly bogus), but never realized where it came from.

It seems like the error classes in hg evolved a lot, and this didn't keep up.  I'm not sure why this function apparently handled so many exceptions before, but now is only called for ParseError.  I do think it's useful to open the bad config file, so I don't think we should drop this completely.  But maybe there's no reason to have the other exception logic in there anymore.

I suspect we'll need to change this to handle ConfigError instead.  IDR when the change from ConfigError to ParseError happened, but maybe it was part of the exception cleanup Martin did.  Even if this was changed from `bytes(e)` to e.format() (which is only available on some subclasses of Abort), it would be difficult to extract the file from the returned `format()` value, which has the message and an optional hint string too.

[Yuya Nishihara]

Oh my

Maybe we can add `if isinstance(e, ParseError)` there? I think it's more
readable than expecting e is a ParseError in a function that looks like
general purpose.

We can also factor out ConfigErrorDialog and remove a tiny bit of
the existing mess.

> It seems like the error classes in hg evolved a lot, and this didn't keep
> up. I'm not sure why this function apparently handled so many exceptions
> before, but now is only called for ParseError. I do think it's useful to
> open the bad config file, so I don't think we should drop this completely.
> But maybe there's no reason to have the other exception logic in there
> anymore.

That's probably because we've migrated away from threaded dispatch.dispatch()
to command server.

> I suspect we'll need to change this to handle ConfigError instead.

Yep. ParseError is too broad since it may be raised by revset parser.