[PATCH] Fix for plasma crashes in Kubuntu

[David Edmundson]

On Tue, Feb 18, 2014 at 5:53 PM, Alexey Borzenkov <snaury at gmail.com> wrote:
> Hi,
>
> I've recently installed Kubuntu 13.10 (haven't tried it in a while)
> and while looking at what's new in KDE almost immediately got some
> plasma crashes. While researching how to best report this bug I found
> numerous bug reports, which ultimately collapsed into single bug
> #302931 marked resolved. So I tried trusty alpha 2, updated, but even
> with qt 4.8.5 it still crashed for me. And it's quite easily
> reproducible for me:
>
> - Install Kubuntu under VMWare Player
> - Change resolution to 1280x800 (not all resolutions seem to trigger this bug!)
> - Click on "Show activities" and then "Add widgets"
>
> Starting with a stack trace, which shows the bug happening during
> deferred destruction of QDeclarativeElement and then QDeclarativeItem,
> I immediately recognized that this happens during traversion of item
> change listeners. Long story short I started to look at kubuntu
> patches and found that the only candidate is
> kubuntu_97_a11y_qt_and_qml_backport.diff, since it seems to add a very
> pervasive QDeclarativeAccessibilityUpdater to declarative items, but
> the actual instance is located in QDeclarativeEnginePrivate. My hunch
> is that QDeclarativeEngine gets destroyed before QDeclarativeItem that
> uses its listener and then later when deferred destructor for
> QDeclarativeItem kicks in the memory belonging to
> QDeclarativeAccessibilityUpdater might be already filled with garbage
> and hence the crash during vtable dereference.
>
> I confirmed my hunch by applying the attached patch, which uses a
> single global QDeclarativeAccessibilityUpdater instance (it's not
> pretty, but very short), which is possible because
> QDeclarativeAccessibilityUpdater is basically stateless, all it does
> is forwarding to QAccessible functions (btw, why does it even inherit
> from QObject? It doesn't have to, not without all that commented
> code!).
>
> Sure enough, after 9 hours of compiling (so I would have a patched and
> simply recompiled kubuntu 13.10 libqt4-declarative packages) I see no
> crashes with my patch and 100% reproducible crashes without my patch.
>
> If the patch is not acceptable (I'm not sure it won't trip valgrind or
> something like that), what would be a good place for
> QDeclarativeAccessibilityUpdate instance? One idea is
> QDeclarativeItemPrivate, but wouldn't it be weird for QDeclarativeItem
> to basically listen to itself? :)
>
> In case attachment doesn't go thru here's the patch inline (might get
> garbled by gmail):
>
> Index: qt4-x11-4.8.4+dfsg/src/declarative/qml/qdeclarativeengine.cpp
> ===================================================================
> --- qt4-x11-4.8.4+dfsg.orig/src/declarative/qml/qdeclarativeengine.cpp
> 2012-11-23 14:09:53.000000000 +0400
> +++ qt4-x11-4.8.4+dfsg/src/declarative/qml/qdeclarativeengine.cpp
> 2014-02-18 15:43:05.510776633 +0400
> @@ -2555,4 +2555,10 @@
>      return true;
>  }
>
> +QDeclarativeAccessibilityUpdater
> *QDeclarativeEnginePrivate::getAccessibilityUpdater(QDeclarativeEngine
> *e)
> +{
> +    static QDeclarativeAccessibilityUpdater accessibilityUpdater;
> +    return &accessibilityUpdater;
> +}
> +
>  QT_END_NAMESPACE
> Index: qt4-x11-4.8.4+dfsg/src/declarative/qml/qdeclarativeengine_p.h
> ===================================================================
> --- qt4-x11-4.8.4+dfsg.orig/src/declarative/qml/qdeclarativeengine_p.h
> 2014-02-18 00:21:38.000000000 +0400
> +++ qt4-x11-4.8.4+dfsg/src/declarative/qml/qdeclarativeengine_p.h
> 2014-02-18 15:41:20.606780904 +0400
> @@ -238,8 +238,6 @@
>
>      mutable QMutex mutex;
>
> -    QDeclarativeAccessibilityUpdater accessibilityUpdater;
> -
>      QDeclarativeTypeLoader typeLoader;
>      QDeclarativeImportDatabase importDatabase;
>
> @@ -314,7 +312,7 @@
>      static QScriptValue formatTime(QScriptContext*, QScriptEngine*);
>      static QScriptValue formatDateTime(QScriptContext*, QScriptEngine*);
>  #endif
> -    static QDeclarativeAccessibilityUpdater
> *getAccessibilityUpdater(QDeclarativeEngine *e) {  return
> &e->d_func()->accessibilityUpdater; }
> +    static QDeclarativeAccessibilityUpdater
> *getAccessibilityUpdater(QDeclarativeEngine *e);
>      static QScriptEngine *getScriptEngine(QDeclarativeEngine *e) {
> return &e->d_func()->scriptEngine; }
>      static QDeclarativeEngine *getEngine(QScriptEngine *e) { return
> static_cast<QDeclarativeScriptEngine*>(e)->p->q_func(); }
>      static QDeclarativeEnginePrivate *get(QDeclarativeEngine *e) {
> return e->d_func(); }
>
>
Whoa. It's really nice to see people get down and dirty with Qt internals.

The best place for these patches is https://codereview.qt-project.org/#mine.
It's a bit intimidating but worth it:
http://qt-project.org/wiki/Gerrit-Introduction

There is probably going to be another Qt4.x at some point and the
original developers normally know best.

Making something static because of a race condition/double delete does
seem like a bit of a bodge.
You're now sharing the QDeclarativeAccessibilityUpdater between
engines which from what you described would also be bad.

Do you have the original backtrace?

David

> Best regards,
> Alexey.
>
> --
> kubuntu-devel mailing list
> kubuntu-devel at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/kubuntu-devel
>