Stuff the "Couldn't find executable" message box into a queued lambda
ClosedPublic

Authored by hein on Feb 9 2018, 1:52 PM.

Details

Summary

This fixes a Plasma shell crash in the Task Manager applet: We
use KRun from inside the onReleased handler of a MouseArea. In
this error case this leads to a KMessageBox spinning the event
loop. The delegate hosting the MouseArea is in the meanwhile
deleted because it's replaced by a different one for the startup
creation. After closing the dialog we're back in the destroyed
delegate and crash.

The patch instead opens the message box from a queued-up lambda
run in the context of qGuiApp (guarded by a test for qGuiApp)
so we escape the caller before starting the nested event loop.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
hein created this revision.Feb 9 2018, 1:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 9 2018, 1:52 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
hein requested review of this revision.Feb 9 2018, 1:52 PM
hein added a comment.Feb 9 2018, 1:53 PM

Argh, sorry for the noisy diff, I didn't do commit -a but arc did I guess ... I'll clean this up, one sec.

hein updated this revision to Diff 26821.Feb 9 2018, 1:55 PM

Remove unrelated changes.

mart added a comment.Feb 9 2018, 1:57 PM

+1 from me

ngraham accepted this revision.Feb 10 2018, 1:11 PM
This revision is now accepted and ready to land.Feb 10 2018, 1:11 PM
dfaure requested changes to this revision.Feb 11 2018, 8:07 PM

If only it was that simple, I would have done this years ago.

Download http://www.davidfaure.fr/2018/weirdcommand.desktop and then running that desktop file, without and then with your patch.
If works for me without, and I bet it fails with your patch, because "if" isn't an executable.
(https://commits.kde.org/kio/0e28930e6748fb45d152e7c3023a034b7db23854 shows what executablePath() will return for that Exec line).

The reason this code was in slotProcessExited is that at that point we know something went wrong, and we can try to find out what happened (and if we get the diagnosis slightly wrong, the harm is minimal, an error did happen in any case). OTOH your patch will fail with any sort of shell command in the Exec line, that doesn't start with an executable name.

I think the actual fix is to call a virtual method for that messagebox, so that it can be implemented differently in apps that don't like message boxes (not just plasma but also e.g. dolphin).

This revision now requires changes to proceed.Feb 11 2018, 8:07 PM

(FWIW I tested my desktop file in dolphin, not in plasma)

hein added a comment.Feb 12 2018, 7:45 AM

Yeah, I had a feeling there were other design considerations here, but they're not documented/commented in the code so the only way to find out was to show you a patch :)

You want this virtual method where? In KRun? And then Plasma should subclass KRun?

Yes. Well, you can't add new virtual methods, but these two already exist:

virtual void handleInitError(int kioErrorCode, const QString &errorMsg);
virtual void handleError(KJob *job);

I think you should be able to (ab)use the first one for this purpose.

(It's called "init" as opposed to "when running a kjob, later", with the assumption that these are the only two cases that can happen but it really means "when we don't have a kjob"; should have been called something more generic, I see now)

hein added a comment.Feb 12 2018, 11:53 AM

@dfaure I don't see how the virtual thing is going to work - there's numerous static affected codepaths so there's no 'this' to pass down to call the non-static error handler on.

I could add a new "silent" RunFlag and pass that around to quiet the message box altogether, but this would mean zero error handling. I'm unhappy about that.

The statics can be refactored, if they're internal ;)

What's more troublesome is the KRun instance might have gone away by the time KProcessRunner ::slotProcessExited is called, though (which is why it's done in a different class).
So maybe what's needed is rather some interface, like defaultJobUiDelegateExtension()->requestMessageBox().

hein added a comment.Feb 12 2018, 12:14 PM

/o\ I don't like life anymore

Come on, this is fun! Well, unless you feel like I took the fun out of it by providing a solution...

hein added a comment.Feb 13 2018, 10:18 AM

Thanks for the solution for sure, but - it requires writing a million lines of boilerplate, extensively refactoring KRun and adding reams of new overloads to its API (there would need to be some way to pass an instance of that interface to all these - not internal - statics). It's more than I have time/energy for currently and I'm not really sure it's viable without a KIO6. I have another idea, though, let me see if it's viable ...

What? no no, KIO::defaultJobUiDelegateExtension()->requestMessageBox() is a kind of singleton, no refactoring needed.

hein added a comment.Feb 13 2018, 10:24 AM

There's a Job::setUiDelegateExtension though in addition to KIO::setDefaultJobUiDelegateExtension. A global would wreak havoc with stuff like plugins, no?

Which plugins?

This stuff is for the application to decide how it wants to handle user interactions like the rename dialog, the skip dialog, the confirm-deletion dialog, and messageboxes.
KIOWidgets provides a default implementation (with modal dialogs), but it can be overriden by the app if it wants to do this differently.
This sounds like something Plasma might want to do, if it wants to avoid modal dialogs (although I'm not sure what it could do instead, since these methods return what the user selected).
OK if the real need is for this interface to be async, that's more work indeed.

hein added a comment.Feb 13 2018, 10:38 AM

It's totally conceivable for e.g. a KPart or a Plasma plugin to open a QWidget-based window where the KMessageBox is appropriate, and if there's only a global interface instance and Plasma overrides it to hide all message boxes it's going to break KIO users in plugins. So I don't think going to a single global is good enough, it would have to be able to set more narrowly. This also bubbles up actually - the KRun calls we're talking about are in libtaskmanager which is technically meant to be UI-agnostic, so it'd have to also expose some way to set the interface instance. I agree all of this is in theory good (we need to make KIO more toolkit-agnostic - I have another giant patch sitting around that's almost-unfinishable that adds QWindow support to some APIs that currently only accept QWidgets I don't even have), but I think it's KIO 6 material and not the quick fix I'm looking for.

I found a cludge that's ugly but works to fix the crash - I'll update this in a moment to see how much you hate it. :)

hein updated this revision to Diff 27046.Feb 13 2018, 10:40 AM

New approach.

hein retitled this revision from Don't proceed in runCommandInternal if the executable doesn't exit to Stuff the "Couldn't find executable" message box into a queued lambda.Feb 13 2018, 10:41 AM
hein edited the summary of this revision. (Show Details)
dfaure added inline comments.Feb 13 2018, 10:55 AM
src/widgets/krun.cpp
1616

if (qGuiApp) { your code } else { qWarning }
so we don't eat a possible error message completely.

Looks OK otherwise.

[we have KDialogJobUiDelegate::showErrorMessage which implements a mesagebox queue, but that goes back to the issue of not having a job or a uidelegate in the first place]
[Every 5 years I'm thinking KRun should be a KJob, we just found another reason why...]

hein added a comment.Feb 13 2018, 11:02 AM

There's a problem with my patch :(. QMetaObject::invokeMethod(context, functor) is new in Qt 5.10. I don't think we can depend on that yet, right?

Ah, but QTimer::singleShot added an overload w/ context in 5.4 ..

hein updated this revision to Diff 27048.Feb 13 2018, 11:06 AM
  • Fall back to qWarning if !qGuiApp
  • Use QTimer::singleShot to work on older Qt
hein added a comment.Feb 13 2018, 11:22 AM

(Side note: I like the idea of a KRun job - something like that could even be added without needing a KIO 6.)

dfaure accepted this revision.Feb 15 2018, 6:40 PM
dfaure added inline comments.
src/widgets/krun.cpp
1618 ↗(On Diff #26821)

Actually, KMessageBox requires qApp, not just qGuiApp.

(Then again, all of KRun requires widgets, so I'm not even sure when this could ever be null, currently)

This revision is now accepted and ready to land.Feb 15 2018, 6:40 PM
This revision was automatically updated to reflect the committed changes.