Bug 385413

Sorry Rene, but this is not a solution we can maintain. If you want to maintain a patch yourself for using QtScript, that's fine. But it does not make much sense to share this here as bug report. I'll close as invalid, since the particular bugs are tracked in other reports (and also being fixed there).

Exactly what's being fixed (and where)? If there's a fix for the upstream bug that doesn't involve doing the impossible I'd love to know it.

Keeping the old code afloat isn't an option (in my experience that only becomes a burden when it's become just about impossible) AND going back to a solution that just worked on all platforms isn't an option either? Then you could at least introduce a workaround that makes the component(s) triggering the bug optional (supposing you can't be more specific and disable only the scripts triggering the bug). A somewhat crippled editor component is better than no editor at all, or one that crashes randomly.

It may not have occurred to you, but forcing the same version number on all frameworks also means that one burnt bean ruins the whole pot. There's a reason why the minimum Qt version number is being kept as low as possible, you're essentially overriding that here for all frameworks.

We can't maintain both variants.

That old Qt versions have evil bugs hidden inside the script engine is bad, but we waited for "years" after the initial V4 release before we ported.

And now QtScript is really "dead" and we need to switch.

To be honest: On most distros you will not get today's frameworks but no new Qt.
I can understand that this is not nice, but realistic.

I agree that the code doesn't appear to be designed to swap backends easily (like the rendering backend in KDevelop's documentation feature, for instance). It looks like the transition could have been prepared by first introducing some sort of backend interface class (apparently you had "years" to do that) but you probably realised that yourselves by now.

I don't get what's dead about QtScript that would oblige you to switch. It's still in recent Qt versions, and it's not like you're using it for rocket science or security related things here. I guess I'll see what happens when I upgrade to 5.40.0 or so, but 5.38.0 still works perfectly fine with QtScript (= that need to switch isn't so urgent at all).

Anyway, I guess I'll end up spoofing an newer version number on patched old KTE code at some point, crossing my fingers that none of the applications I rely on start requiring 5.38+ during my lifetime (or until I get to upgrade all my systems to Qt 5.9+).

> I don't get what's dead about QtScript that would oblige you to switch.

QtScript is deprecated since Qt 5.5. It is only still included because it has some features that are not available in QtQML.

http://wiki.qt.io/New-Features-in-Qt-5.5

Rene, if you want to invest your time effectively, fix the remaining bugs in the current implementation: Look into the indentation scripts and see what line exactly crashes, and then find a workaround.

Everything else is wasted time.

I agree that would be the proper approach. It's what I tried to do initially, when I noticed that I know exactly how to trigger the crash (pretty certain I described that in the bug I filed, too). But I couldn't find any scripts, nor does the backtrace give me any indication about a script line or expression that crashes.

If somebody can give me some pointers how to figure this out I'll be happy to give it a try (preferably without having to build a full-debug Qt install though ;) )

See: https://docs.kde.org/trunk5/en/applications/katepart/dev-scripting.html

Scripts on disk have higher priority that scripts compiled into the katepart library. That means: If you copy the cstyle.js indenter into your $XDG_DATA_HOME/katepart5/script/indentation/cstyle.js, it should be picked up. To be sure, increase the revision of the cstyle.js file in the json header.

Then, you can use debug("string") to dump text to the console. Additionally, you can set the variable in var debugMode = false; to true to get more debug output, see https://github.com/KDE/ktexteditor/blob/master/src/script/data/indentation/cstyle.js

The same holds also for other indenters that crash KTextEditor. I think some combination with CSS was also an issue.

Does that help?

Yes, thanks. That should at least help in identifying where things go wrong.

(In reply to Dominik Haumann from comment #8)

> To be sure, increase the revision of the cstyle.js file in the json
> header.

Sorry, which json header would that be?

https://bugreports.qt.io/browse/QTBUG-63045 now presents a workaround implemented by Gentoo: disabling the QML JIT compiler. That seems a bit overkill (performance implications?), but could be a viable workaround if it can be done transiently just where/when necessary. I haven't yet look if that's the case. 

Come to think of it, maybe that in this case it's not the JIT compiler that causes the crash, but the generator that translates the script into C++ . In that case I'd just have to figure out how to install the .js file in a system-wide location that's takes precedence over the builtin code.

The json header in the cstyle.js file itself.

> Come to think of it, maybe that in this case it's not the JIT compiler that causes the crash, but the generator that translates the script into C++ . In that case I'd just have to figure out how to install the .js file in a system-wide location that's takes precedence over the builtin code.

There is no such generated, the only difference is that bundled files are put into qresources. But that has zero impact on the later parsing/execution

Ok, so you're not doing exactly as described at http://doc.qt.io/QtQuickCompiler/? (I only took a casual look, to me this falls under "everything you never wanted to know but were not too afraid to ask" ... if you get the pun ;))

No, we don't use a QtQuickCompiler.

@Rene: Any news? As said, we don't use any Qt Quick Compiler. To drive this forward, simply copy the cstyle.js file into the respective file and add debugging output. This essentially is printf-debugging. Don't make it more complicated, you have all details you need to go forward here. Also you don't need any backtraces into Qt, no debug build is required. That said, you can start right away as is, without any modifications and what not.

Sorry, I just haven't had time. Hopefully next week.

Created attachment 108500 [details]
test cstyle.js

I had some time over my morning coffee. When using QtScript and just the stock cstyle.js with debug mode activated I get output lines for tryComment and tryStatement. When using QML, I only get the tryComment debug output, and none of the probes I added print, including the one in indent(). 

Indent() seems to be the module's entry point (note that I know nothing of javascript). If that's correct, I'll probably need to look at another file?

Created attachment 108501 [details]
test cstyle.js

Doh, my bad, I didn't modify the right version of the file. (I had tried first in $prefix/share/katepart5/script/indentation, not realising it's overridden by the copy in the resource. I think that should be the opposite, btw., builtin copies should always be the fallback option).

I also should have waited until after I discovered and tried the QV4_FORCE_INTERPRETER env. var.

Gentoo's trick of disabling the JIT works, and can also be obtained with said env. var. That was helpful getting my probes to work... and could possibly be used as a workaround when building against affected Qt versions (set and restore the env.var around calls into the QML script engine, if all else fails).

Here's what I'm seeing with the current cstyle.js when I hit enter at the end of a function's final line that says `return decl;` :

indent line=84 indentWidth=4 ch=

indent linenr=850
indent linenr=852
indent linenr=
indentLine line=84 linenr=731
isComment: Check mode @ Cursor(83,4): ISO C++:Control Flow
Process 26907 stopped
* thread #1, name = 'kate', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)

And now you have to add more debug("xyz...") statements in the js file to identify exactly the line of the crash. Which line in the js code crashes Qt?

Created attachment 108507 [details]
test cstyle.js

Prints

indent line=84 indentWidth=4 ch=

indent linenr=880
indent linenr=882
indent linenr=886
indentLine line=84 linenr=746
isComment: Check mode @ Cursor(83,4): ISO C++:Control Flow
tryCComment linenr=292
indentLine filler=-1 linenr=757
indentLine filler=-1 linenr=761
indentLine filler=-1 linenr=765
indentLine filler=-1 linenr=769
indentLine filler=-1 linenr=773
indentLine filler=-1 linenr=777
tryCondition currentLine=83 linenr=488
tryCondition currentString="    return decl;" lastPos=15 lastChar=; linenr=495
tryCondition currentIndentation=4 linenr=509
tryCondition decr currentLine=83 lineDelimiter=10 linenr=513
tryCondition firstPosVirtual=4 linenr=519
tryCondition decr currentLine=82 lineDelimiter=9 linenr=513
tryCondition firstPosVirtual=8 linenr=519
tryCondition decr currentLine=81 lineDelimiter=8 linenr=513
tryCondition firstPosVirtual=12 linenr=519
tryCondition decr currentLine=80 lineDelimiter=7 linenr=513
tryCondition firstPosVirtual=8 linenr=519
tryCondition decr currentLine=79 lineDelimiter=6 linenr=513
tryCondition firstPosVirtual=8 linenr=519
tryCondition decr currentLine=78 lineDelimiter=5 linenr=513
tryCondition firstPosVirtual=8 linenr=519
tryCondition decr currentLine=77 lineDelimiter=4 linenr=513
tryCondition firstPosVirtual=8 linenr=519
tryCondition decr currentLine=76 lineDelimiter=3 linenr=513
tryCondition firstPosVirtual=4 linenr=519
tryCondition decr currentLine=75 lineDelimiter=2 linenr=513
tryCondition firstPosVirtual=4 linenr=519
tryCondition decr currentLine=74 lineDelimiter=1 linenr=513
tryCondition firstPosVirtual=4 linenr=519
Process 28235 stopped
* thread #1, name = 'kate', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)

The crashing line would be either the `if (firstPosVirtual < currentIndentation)` expression, or else something happens unwinding the stack when coming out of the while loop.

If it helps, this is the C++ function in which I'm testing (lines 66-85):

```
 66 Declaration* usefulDeclaration(Declaration* decl)
 67 {
 68     if (!decl)
 69         return nullptr;
 70 
 71     // First: Attempt to find the declaration of a definition
 72     decl = DUChainUtils::declarationForDefinition(decl);
 73 
 74     // Convenience feature: Retrieve the type declaration of instances,
 75     // it makes no sense to pass the declaration pointer of instances of types
 76     if (decl->kind() == Declaration::Instance) {
 77         AbstractType::Ptr type = TypeUtils::targetTypeKeepAliases(decl->abstractType(), decl->topContext());
 78         IdentifiedType* idType = dynamic_cast<IdentifiedType*>(type.data());
 79         Declaration* idDecl = idType ? idType->declaration(decl->topContext()) : nullptr;
 80         if (idDecl) {
 81             decl = idDecl;
 82         }
 83     }
 84     return decl;
 85 }
```

> The crashing line would be either the `if (firstPosVirtual <
> currentIndentation)` expression, or else something happens unwinding the
> stack when coming out of the while loop.

Probably the latter: decreasing lineDelimiter doesn't prevent the crash.

Would there be any possibility to include/access node.js features from KTE scripts, so we can deactivate the V4 JIT from the QML code itself, where and when needed? (https://stackoverflow.com/questions/4870328/read-environment-variables-in-node-js)

Using a for instead of a while also crashes:

//         var lineDelimiter = 10; // 10 limit search, hope this is a sane value
        for ( var lineDelimiter = 9 ; currentLine > 0 && lineDelimiter >= 0 ; --lineDelimiter) {
    dbg("tryCondition decr currentLine=" + currentLine + " lineDelimiter=" + lineDelimiter + " linenr=" + (new Error()).lineNumber);
            --currentLine;
//             --lineDelimiter;

Created attachment 108512 [details]
alternative workaround patch

This is a much easier (maintainable) workaround pending a proper solution. It disables QML's V4 JIT for all KTextEditor scripts when using an older Qt version. Care is taken to restore the environment value. This is as fine-grained a control over the JIT compiler that I've been able to obtain.

Created attachment 108514 [details]
polished up workaround patch

Qt prints a single warning when the JIT is disabled; print a single explanation under this warning.

Created attachment 108516 [details]
tryCondition() version that seems to avoid the stack unwind crash

I tried a bit of a kludge to avoid popping the "while stack" just before returning from the function. Lo and behold, returning from within the while loop seems to something sufficiently differently internally that the crash is avoided.

That "break" does cause processing to continue after the while, right?

Leaving the break in place and just adding the `else if` check of the 2 loop variables works too, btw.

If this patch works, I'm all for it.

Could anyone else test this already? 

René, could you post this on Phabricator?

Done: https://phabricator.kde.org/D8544

Git commit e427f6d4bdf95d426b6ba9c49114dcb09af0d6e0 by René J.V. Bertin.
Committed on 03/11/2017 at 12:35.
Pushed by rjvbb into branch 'master'.

Avoid (certain) crashes while executing QML scripts

Certain of KTextEditor's indentation scripts trigger a bug in Qt versions
before 5.9.1 . Disabling the QML JIT compiler for those Qt versions
prevents that.

Differential Revision: https://phabricator.kde.org/D8544

M  +10   -0    src/utils/kateglobal.cpp

https://commits.kde.org/ktexteditor/e427f6d4bdf95d426b6ba9c49114dcb09af0d6e0