Bug 219787

Created attachment 39277 [details]
Patch for unittest.py and TestObject showing the bug

Here it is a unit test showing the problem.

I have added some attributes and slots to TestObject. There is a slot to connect the Python signal from C++, connectCallTestFunction. It adapts the pure signal name to a name understood by connect (signals start by "2").

The signal is connected to another slot, callTestFunction, which calls the function in the script (managed by the Kross Action stored in the attribute) named "testFunction" with parameter "42".

Finally, another slot returns the value returned by "testFunction" when it was called, that should be a QVariantList with just one integer element, 42 (as testFunction returns the argument it received). This isn't needed to show the crash, as it happens when the function is called, but I added it to have something to assert once the crash was fixed.

The test method in unittest.py first checks that PyQt is installed and its version is 4.5 or greater. Otherwise, the test is skipped.

Then it just sets the scenario that triggers the crash, as explained before.

Created attachment 39278 [details]
Patch for pythonscript.cpp with a workaround to the bug

Although the crash itself happens in Python libraries, the backtrace goes through pythonscript.cpp before diving into Python libraries. I take a look to the las Kross executed method (PythonScript::callFunction(const QString& name, const QVariantList& args)) and found "//TODO do we need to acquire interpreter lock here?". So I tried to lock the interpreter where the crash happened (not in the whole method, as the TODO suggested).

I wrapped PyErr_Clear() with a lock, and then the method was executed further before crashing again. So then I locked the next place where it happened (Py::Object pyresult = funcobject.apply(PythonType<QVariantList,Py::Tuple>::toPyObject(args));), and then again in QVariant result = PythonType<QVariant>::toVariant(pyresult); (which uses the same lock as the previous sentence because they were one after the other).

After locking the interpreter in those places, all was fine. No more crashes, and the script behaved as expected. Note, however, that some locks may be needed in the catch clause (I haven't tested it with code that launched exceptions).

And also, please take into account that I have no true idea of what I did. I mean, I locked the interpreter using the code found somewhere else in pythonscript.cpp, and I locked it because there was a TODO suggesting it. But I don't know if it is the best fix, or if it is a fix at all and not just a workaround. I only know that locking the interpreter no crash happens ;)

Any chance to get this crash fixed (or at least workarounded like shown in the patch) before KDE SC 4.4.0 release? :)

Created attachment 40061 [details]
Patch for unittest.py and TestObject showing the bug

I have extended the unit tests shown in comment #1 to check also what happens when the Python function called raises an exception.

The assert in this case checks that no value was returned by the function, as the return value is set by Kross to a QVariant() when there is a problem execution a Python function.

The rest of the things explained in comment #1 still apply here.

Created attachment 40062 [details]
Patch for pythonscript.cpp with a workaround to the bug

I have updated the workaround/fix shown in comment #2 to also handle when exceptions are raised in the Python function.

For what I could see, all the statements (but the krosswarning and "QStringList trace;") in the catch clause had to be locked, or the test crashed.

Also, the lock acquired in the try clause before calling the Python function had to be released. I opted to wrap the call with another try/catch, release the lock in the catch and throw again the exception, so it got caught again in the outer catch.

As there are several other statements in the outer try clause, I assume that the Py::Exception can be thrown by some of them (although I have no idea, I suppose it based on that code).

That's why I have added the inner try/catch to release the lock. If the lock was released in the outer catch, it could be executed by exceptions thrown when no lock had to be released.

Created attachment 71993 [details]
Patch for unittest.py and TestObject showing the bug

The bug, and the unit tests that show it, are still valid. I have updated the patch to apply cleanly using git (just removed two trailing whitespaces). It must be applied in "kdelibs" project.

Created attachment 71994 [details]
Patch for pythonscript.cpp with a workaround to the bug

The proposed fix is still valid. I have updated the patch to apply cleanly using git (just changed the path to the modified file). It must be applied in "kross-interpreters" project (which is a subproject of "kdebindings").

Git commit c17f808e3bac78d20f75b075ba3b6d9b5921e3df by Daniel Calviño Sánchez.
Committed on 21/09/2012 at 21:06.
Pushed by danxuliu into branch 'KDE/4.10'.

Add unit tests for crash in krosspython when a PyQt signal is connected to a C++ method that calls a function in the script.

M  +43   -0    kross/test/testobject.cpp
M  +10   -0    kross/test/testobject.h
M  +46   -0    kross/test/unittest.py

http://commits.kde.org/kdelibs/c17f808e3bac78d20f75b075ba3b6d9b5921e3df

Git commit c74c9899d4c2e42ad17855bb4eb470ae188c2180 by Daniel Calviño Sánchez.
Committed on 21/09/2012 at 17:26.
Pushed by danxuliu into branch 'master'.

Fix crash when a PyQt signal is connected to a C++ method that calls a function in the script.

I would have prefered that someone with experience in the code had
reviewed the fix. Unfortunately, it seems that krosspython is currently
unmaintained, so I have decided to push the fix myself.

M  +31   -3    python/pythonscript.cpp

http://commits.kde.org/kross-interpreters/c74c9899d4c2e42ad17855bb4eb470ae188c2180