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 :/

  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.)

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.

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.

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.

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?

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.

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)!

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 :))