D8544: KTextEditor : avoiding QML crashes
This revision was automatically updated to reflect the committed changes. Closed by commit R39:e427f6d4bdf9: Avoid (certain) crashes while executing QML scripts (authored by rjvbb). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D8544?vs=21793=21833#toc REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8544?vs=21793=21833 REVISION DETAIL https://phabricator.kde.org/D8544 AFFECTED FILES src/utils/kateglobal.cpp To: rjvbb, #frameworks Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, sars
D8544: KTextEditor : avoiding QML crashes
rjvbb updated this revision to Diff 21793. rjvbb marked an inline comment as done. rjvbb added a comment. Updated as requested. CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8544?vs=21724=21793 REVISION DETAIL https://phabricator.kde.org/D8544 AFFECTED FILES src/utils/kateglobal.cpp To: rjvbb, #frameworks Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, sars
D8544: KTextEditor : avoiding QML crashes
rjvbb set the repository for this revision to R39 KTextEditor. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, sars
D8544: KTextEditor : avoiding QML crashes
rjvbb marked 2 inline comments as done. rjvbb added inline comments. INLINE COMMENTS > kfunk wrote in kateglobal.cpp:106 > Minor: "... set to 1" would be sufficient. You're setting it yourself right > before, so you can presume it is set (=> no need for `qgetenv(...)`. I know that of course, but figured that if I was adding a debug output I could just as well have it verify that we got what we asked for. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, sars
D8544: KTextEditor : avoiding QML crashes
dhaumann added a comment. By limited amount of time I mean we can remove it once the required Qt version of KDE Frameworks is 5.9. (I thought this was obvious :)) REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, sars
D8544: KTextEditor : avoiding QML crashes
kfunk added inline comments.
INLINE COMMENTS
> kateglobal.cpp:104
> +// disable the QML JIT compiler as a protection against an unknown bug
> +// in Qt's V4 engine which can provoke a crash in certain of our scripts.
> +qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));
Please add a reference to the bug reports (Qt JIRA, KDE Bugzilla) here.
> kateglobal.cpp:106
> +qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));
> +qCDebug(LOG_KTE) << "QV4_FORCE_INTERPRETER set to" <<
> qgetenv("QV4_FORCE_INTERPRETER");
> +#endif
Minor: "... set to 1" would be sufficient. You're setting it yourself right
before, so you can presume it is set (=> no need for `qgetenv(...)`.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: kfunk, dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking,
head7, sars
D8544: KTextEditor : avoiding QML crashes
rjvbb added a comment. Good, I'll push this sometime tonight (I'm waiting for some feedback via the Qt bug report ticket, but won't mind if someone beats me to it0. > I agree with this patch. It will not hurt and is clearly only relevant for a limited amount of time. So +1 from me. By limited you mean "until we start requiring Qt 5.9"? That won't be too soon, I hope (it's already a pity support for 5.6LTS had to be dropped)! REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars
D8544: KTextEditor : avoiding QML crashes
dhaumann added a comment. I agree with this patch. It will not hurt and is clearly only relevant for a limited amount of time. So +1 from me. And thanks a lot René and Christoph for this work. It will make the next Frameworks release better, which is alraedy tagged in 2 days. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: dhaumann, cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars
D8544: KTextEditor : avoiding QML crashes
cullmann added a comment. I could live with that change. It at least will avoid random crashs for the time being until we perhaps have a better solution. Other opinions? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb set the repository for this revision to R39 KTextEditor. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb updated this revision to Diff 21724. rjvbb added a comment. This simpler version also seems to do the trick for me (read: I haven't been able to get "it" to crash). CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D8544?vs=21519=21724 REVISION DETAIL https://phabricator.kde.org/D8544 AFFECTED FILES src/utils/kateglobal.cpp To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
cullmann added a comment.
In Qt 5.6.0 it was already:
static const bool forceMoth =
!qEnvironmentVariableIsEmpty("QV4_FORCE_INTERPRETER");
in Qt 5.7.0 it is:
static const bool forceMoth =
!qEnvironmentVariableIsEmpty("QV4_FORCE_INTERPRETER") ||
!OSAllocator::canAllocateExecutableMemory();
The warning
if (jitDisabled) {
qWarning("JIT is disabled for QML. Property bindings and
animations will be "
"very slow. Visit https://wiki.qt.io/V4 to learn about
possible "
"solutions for your platform.");
}
is not guarded with the static, just with !factory, perhaps that can occur
multiple times.
In any case, the setting is fixed after the first call of
ExecutionEngine::ExecutionEngine.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars,
dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb added a comment. > Before we got to excessive solutions, I assume you have a build were you can reproduce the crashs. Evidently... I'll check your proposal later today, but a priori I'd say that we should be fine with setting QV4_FORCE_INTERPRETER from any location that has a wider scope than the "jit" approach I've been using until now. FWIW: if you look through the comments on the linked Qt bug ticket you'll see how a Qt 5.7.1 user gets the "JIT is disabled" warning multiple times. Without having dug up the source I'd say that means the older Qt version doesn't cache the result of the QV4_FORCE_INTERPRETER lookup. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
cullmann added a comment.
Before we got to excessive solutions, I assume you have a build were you can
reproduce the crashs.
If you add to your version the line:
qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));
into KTextEditor::EditorPrivate::EditorPrivate constructor in kateglobal.cpp,
does that solve the crashs for you?
If that works one could add that there with some dynamic check with
qVersion() inside a
#if QT_VERSION < QT_VERSION_CHECK(5, 9, 1)
block.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars,
dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb added a comment.
> static const bool forceMoth =
!qEnvironmentVariableIsEmpty("QV4_FORCE_INTERPRETER") ||
>
!OSAllocator::canAllocateExecutableMemory();
>
> That means on the first invocation that is cached for ever.
Damn, I must have missed the `static`, I was certain that the check was
performed each time. Wishful thinking, maybe.
> in the editor singleton on first instantiation and even then this only
helps if no other part of the application did execute a script in advance.
There must be a way to do this with a shared library initialisation routine
that's executed when the library is loaded. I've used that feature on Mac and
Linux and it's possible on MSWin too.
The statement could be repeated in the editor singleton for cases where the
library is deployed in a static build.
Both approaches (and the combined approach) will be easier to implement than
my current patch.
> And it will degenerate performance for the complete application if it
works at all :/
Lesser performance is always better than no ("fully degraded") performance at
all, but apparently the penalty isn't so big. It's not noticeable for KTE
scripts, and the Gentoo guys apparently deem it an acceptable trade-off as well.
"Applications using KTextEditor won't support the fastest possible QML
performance with older Qt versions". Sounds reasonable to me (and we could
always document how to patch QtDeclarative to make that statement above
non-static.)
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars,
dhaumann
D8544: KTextEditor : avoiding QML crashes
cullmann added a comment.
Hi, first: thanks for working on getting the crashs for older versions away.
To the changes:
1. if the small change to cstyle.js helps to workaround bugs, feel free to
commit that part.
2. for the disabler: I think that is not usable in the way it is done, sorry.
If you look at the code of qt, the check for the env var works like:
qtdeclarative/src/qml/jsruntime/qv4engine.cpp
static const bool forceMoth =
!qEnvironmentVariableIsEmpty("QV4_FORCE_INTERPRETER") ||
!OSAllocator::canAllocateExecutableMemory();
That means on the first invocation that is cached for ever.
If we want to use this approach, we can try to e.g. do a
qputenv("QV4_FORCE_INTERPRETER", QByteArrayLiteral("1"));
in the editor singleton on first instantiation and even then this only helps
if no other part of the application did execute a script in advance.
And it will degenerate performance for the complete application if it works
at all :/
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: cullmann, kde-frameworks-devel, kevinapavew, demsking, head7, kfunk, sars,
dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb marked an inline comment as done. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 To: rjvbb, #frameworks Cc: kde-frameworks-devel, kevinapavew, demsking, head7, cullmann, kfunk, sars, dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb added inline comments.
INLINE COMMENTS
> cstyle.js:518
> +} else if (currentLine == 0 || lineDelimiter == 0) {
> +return indentation;
> }
`indentation` should always equal -1 here (so no need to add the dbg()
statement).
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D8544
To: rjvbb, #frameworks
Cc: kde-frameworks-devel, kevinapavew, demsking, head7, cullmann, kfunk, sars,
dhaumann
D8544: KTextEditor : avoiding QML crashes
rjvbb created this revision. rjvbb added a reviewer: Frameworks. rjvbb added a project: KTextEditor. Restricted Application added projects: Kate, Frameworks. REVISION SUMMARY The transition from QtScript to QML introduced a propensity to crashing somewhere deep in Qt (in the V4 JIT engine to be exact), at often unexpected moments while editing texts, for users of certain versions of Qt5. It seems these crashes do not occur with Qt 5.9.1 and newer, but not everyone can update (readily) to that version. Upstream bug report: https://bugreports.qt.io/browse/QTBUG-63045 I have tried to trace the JavaScript expressions that trigger the crashes I've seen myself, come up with a fix or at least a suitable and acceptable workaround (see https://bugs.kde.org/show_bug.cgi?id=385413). This review is for a patch that contains a fix for a specific crash as well as a general workaround. As far as I can tell the crashes I get (when hitting enter at the end of a line in documents using C style indentation) occur when unwinding the script interpreter stack, for instance when exiting from a `while` loop (or the equivalent `for` loop). This particular crash can be avoided by returning early from the procedure containing the loop, instead of exiting from the loop and returning via the shared return statement; see the patch to `cstyle.js`. Gentoo have come up with a blunt-force "solution": build QtDeclarative with the V4 JIT disabled. It works just as well to launch applications that are susceptible to the crash with the `QV4_FORCE_INTERPRETER` env. variable set which has less undesirable effects but is also more cumbersome. My patch explores an even less invasive approach: it uses the env. variable to disable the JIT when KTextEditor scripts are loaded/parsed, resetting (or unsetting) the variable when the crucial operation is done. The env.var manipulation is done in a dedicated KateScript subclass and is a noop for Qt version 5.9.1 and up. BUG: 385413 TEST PLAN Tested on Mac and Linux with Qt 5.8.0 . This works for me (read: I haven't seen any other crashes - yet!) but apparently does not prevent crashing with Qt 5.7.1 (see the Qt bug report referenced in the summary). If necessary we can of course disable the JIT proactively in a KTextEditor initialiser routine (if possible reenabling it for plugins). REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D8544 AFFECTED FILES src/script/data/indentation/cstyle.js src/script/katecommandlinescript.cpp src/script/kateindentscript.cpp src/script/katescript.cpp src/script/katescript.h src/script/katescripthelpers.cpp To: rjvbb, #frameworks Cc: kde-frameworks-devel, kevinapavew, demsking, head7, cullmann, kfunk, sars, dhaumann
