Mumble: Crash when hyperlink clicked with no default browser under Windows

[Ytmndissue]

When you click a hyperlink in a user comment (the ones which you can pin next to your name, not the chat log,) and you have a version of Windows that does not have a default browser set, this causes Mumble to crash.

Perhaps a check can be done for all hyperlinks that sees if a browser is set on the system before Mumble tries opening the link with it - if no browser is set, clicking the links does nothing and prevents a crash.

[Ytmndissue]

Just as clarification, it only seems to happen with the "next to user message comments," clicking a hyperlink in the textbox will still generate an error "this file does not have an action associated with it," but no crash will occur.

Interestingly enough, this might apply to any "protocol" style message, such as ftp or other "protocol://" style links - which could cause a crash with any system if you could craft a URL with a protocol that Windows is guaranteed to not know.

However: QT (or Mumble) seems to be smart enough to know not to create clickable links for those other protocols - ftp or http links work, but madeup protocols are not (or are filtered though.) Perhaps they are always filtered out, or something checks to see if that protocol is associated with the system before making it clickable.

[davidebeatrici]

Thank you very much for reporting the issue!

Console log:

<W>2019-01-27 00:10:45.611 ShellExecuteEx 'https://mumble.info' failed (error 0x4c7).
<W>2019-01-27 00:10:50.730 Qt has caught an exception thrown from an event handler. Throwing
exceptions from an event handler is not supported in Qt.
You must not let any exception whatsoever propagate through Qt code.
If that is not possible, in Qt 5 you must at least reimplement
QCoreApplication::notify() and catch all exceptions there.

[trudnorx]

Why is there an exception if QDesktopServices::openUrl to my understanding simply reports false without throwing an exception when ShellExecuteEx fails?

[Ytmndissue]

Looks like the builds are failing due to QT4 not supporting the try/catch that was used?

[davidebeatrici]

Correct.

QException was introduced in Qt 5: https://doc.qt.io/qt-5/qexception.html

[Ytmndissue]

Yeah, I'm seeing the error "expected type-specifier before ‘QException’" with the compiler implying that it isn't defined or something.

Wish I could help with this but I don't know the first thing when working with QT to come up with alternative error handling. Maybe instead of trying to use QDesktopServices() to open it and fall back on an error as a "wholeistic" approach, could there be some way to use some sort of other built in QT function to figure out what protocol "url" is and then check to see if the system has the protocol as non null or something?

If no default is returned, just don't call the function.

[Ytmndissue]

Or maybe a more sloppy way to do it... I'm assuming all Windows builds are statically done with QT5 now?

Maybe the code fix section could be encapsulated with an ifdef to check for QT5 is present/defined, else just compile the older code section.

[davidebeatrici]

Or maybe a more sloppy way to do it... I'm assuming all Windows builds are statically done with QT5 now?

Maybe the code fix section could be encapsulated with an ifdef to check for QT5 is present/defined, else just compile the older code section.

Exactly.

Also, we're going to remove support for Qt 4 immediately after 1.3 is released.

[Ytmndissue]

I might try to mock up the code based on the contribute above - any examples/files you can point me towards where you used a QT5 check and a less version fallback? Sounds like I might be using "#if QT_VERSION >= 0x050000"

[davidebeatrici]

https://github.com/mumble-voip/mumble/blob/4976c1ad2e0802755c15d051783403afc440b173/src/mumble/MumbleApplication.h#L12-L14

I can push the change to #3607.

[Ytmndissue]

Thanks, I'll try fussing with it and come up with a quick fix.

I suppose when 1.3 is released, you'll just grep through the code and get rid of all of the QT "patches" like this for 5 as it will just assumed to be running 5 at that point.

[davidebeatrici]

Thanks, I'll try fussing with it and come up with a quick fix.

Thank you.

I suppose when 1.3 is released, you'll just grep through the code and get rid of all of the QT "patches" like this for 5 as it will just assumed to be running 5 at that point.

Absolutely, see #3602.

[Ytmndissue]

Okay, I don't know your rules regarding indentation regarding preprocessor things so I left those on line one, nor am I quite sure on your comment rules so I just left this as is for a quick tweak. Just a quick notepad job.

fix.txt

Haven't done much in C++ so hopefully I didn't miss anything.

[davidebeatrici]

No worries, the fix is correct.

I'm going to create a commit with you as author.

[Ytmndissue]

Credit "trudnorx," I wouldn't have ever though to use (or know enough about QT) to do a try catch with it to begin with, and this is just an anonymous BS account.

Thank you, I'll be asking questions about my scaling issue in the other thread.

[davidebeatrici]

Credit "trudnorx," I wouldn't have ever though to use (or know enough about QT) to do a try catch with it to begin with, and this is just an anonymous BS account.

No problem.

Thank you, I'll be asking questions about my scaling issue in the other thread.

You're welcome.

[davidebeatrici]

I tried to reproduce the issue after building Mumble with Qt 5.12.1 from MSYS2:

<W>2019-03-11 04:09:06.317 ShellExecute 'https://mumble.info' failed (error 5).

No exceptions thrown and ShellExecute() is called instead of ShellExecuteEx() according to the log.

I decided to investigate whether the issue was fixed in a recent Qt version by checking the commits history: https://github.com/qt/qtbase/commits/5.12.1/src/plugins/platforms/windows/qwindowsservices.cpp

But no significant changes have been made to the internal shellExecute() function since Qt 5.6.2 (the version we use to build snapshots with).

It was at that moment I realized that the issue had to be related to one of our patches, which I found immediately: https://github.com/mumble-voip/mumble-releng/blob/master/buildenv/1.3.x/common/qt5/patches/qt-5.6.2-windows-platform-plugin-use-ShellExecuteEx.patch

--- ./qtbase/src/plugins/platforms/windows/qwindowsservices.cpp
+++ ./qtbase/src/plugins/platforms/windows/qwindowsservices.cpp
@@ -54,13 +54,15 @@ static inline bool shellExecute(const QU
     const QString nativeFilePath = url.isLocalFile() && !url.hasFragment() && !url.hasQuery()
         ? QDir::toNativeSeparators(url.toLocalFile())
         : url.toString(QUrl::FullyEncoded);
-    const quintptr result =
-        reinterpret_cast<quintptr>(ShellExecute(0, 0,
-                                                reinterpret_cast<const wchar_t *>(nativeFilePath.utf16()),
-                                                0, 0, SW_SHOWNORMAL));
-    // ShellExecute returns a value greater than 32 if successful
-    if (result <= 32) {
-        qWarning("ShellExecute '%s' failed (error %s).", qPrintable(url.toString()), qPrintable(QString::number(result)));
+    SHELLEXECUTEINFO sei;
+    memset(&sei, 0, sizeof(sei));
+    sei.cbSize = sizeof(sei);
+    sei.lpFile = (wchar_t*)nativeFilePath.utf16();
+    sei.nShow = SW_SHOWNORMAL;
+
+    if (!ShellExecuteEx(&sei)) {
+        quintptr error = (quintptr) GetLastError();
+        qWarning("ShellExecuteEx '%s' failed (error 0x%x).", qPrintable(url.toString()), error);
         return false;
     }
     return true;

The patch was introduced in mumble-voip/mumble-releng@25b0509ed0ef35fe4f0a7ca59fb5b9d47bbfef59.

[Ytmndissue]

So (if I'm interpreting this correctly,) this patch for handling file paths somehow is causing the crashes?

[davidebeatrici]

That's correct, without the patch the exception is not thrown and Qt doesn't cause Mumble to crash.

[Ytmndissue]

Maybe I'll try to bash my head into it and figure it out if I get the time, but chances are there are far more capable sources to figure it out.

Thanks for checking and reopening.

[davidebeatrici]

You're welcome.

[trudnorx]

Does this mean my "fix" should be reverted?

[davidebeatrici]

Your pull request was not merged.