From 987b5a1cdcdddf4d3abb765324ee43c6fd48dab2 Mon Sep 17 00:00:00 2001 From: Gioacchino Mazzurco Date: Sat, 15 Apr 2017 02:17:27 +0200 Subject: [PATCH] QML app fix: crash closing, notification deadlock The crash was introduced at 533dbef0c79b37b6c50c9176235d2e5d0193918b This has been particurarly tricky as lot of different parts contributed in causing unexpected behaviours When the activity is created onNewIntent is not called and we have to get the intent data from C++ bu other means, but C++ code is running in a different thread so there is no guarantee that the intent data is reacheable yet on starting, so the C++ code has to wait for the intent data being ready, but paying attention to not cause a deadlock beetween the two thread (the android ui thread may be waiting for some operation performed by Qt) Because of notification intent flags not properly set the activity was recreated also if it was already on top, this caused a nasty interaction between android ui thread and qt thread that derived in a deadlock, to avoid this lot of try/error has been made until the proper soup of manifest and intent flags has been found At this point link handling, notification handling, and Activity closing should work as expected without any deadlock or crash --- .../src/notificationsbridge.h | 4 +- retroshare-qml-app/src/android/android.iml | 16 ++-- .../src/android/src/NativeCalls.cpp | 25 +++--- .../RetroShareAndroidNotifyService.java | 23 ++--- .../qml_app/RetroShareQmlActivity.java | 11 +-- retroshare-qml-app/src/main.cpp | 86 +++++++++++++++++-- retroshare-qml-app/src/main.qml | 26 +++++- retroshare-qml-app/src/retroshare-qml-app.pro | 8 +- ...ingletonqmlengine.h => rsqmlappengine.cpp} | 32 ++++--- retroshare-qml-app/src/rsqmlappengine.h | 67 +++++++++++++++ 10 files changed, 236 insertions(+), 62 deletions(-) rename retroshare-qml-app/src/{singletonqmlengine.h => rsqmlappengine.cpp} (56%) create mode 100644 retroshare-qml-app/src/rsqmlappengine.h diff --git a/retroshare-android-notify-service/src/notificationsbridge.h b/retroshare-android-notify-service/src/notificationsbridge.h index 480b9da1f..de7829325 100644 --- a/retroshare-android-notify-service/src/notificationsbridge.h +++ b/retroshare-android-notify-service/src/notificationsbridge.h @@ -22,8 +22,8 @@ #include #ifdef __ANDROID__ - #include - #include +# include +# include #endif // __ANDROID__ struct NotificationsBridge : QObject diff --git a/retroshare-qml-app/src/android/android.iml b/retroshare-qml-app/src/android/android.iml index 722af3989..b499a4598 100644 --- a/retroshare-qml-app/src/android/android.iml +++ b/retroshare-qml-app/src/android/android.iml @@ -64,14 +64,6 @@ - - - - - - - - @@ -80,6 +72,14 @@ + + + + + + + + diff --git a/retroshare-qml-app/src/android/src/NativeCalls.cpp b/retroshare-qml-app/src/android/src/NativeCalls.cpp index 368a34794..ac2a5e1dd 100644 --- a/retroshare-qml-app/src/android/src/NativeCalls.cpp +++ b/retroshare-qml-app/src/android/src/NativeCalls.cpp @@ -17,30 +17,27 @@ */ #include "NativeCalls.h" -#include "singletonqmlengine.h" +#include "rsqmlappengine.h" -#include -#include #include #include -#include JNIEXPORT void JNICALL Java_org_retroshare_android_qml_1app_jni_NativeCalls_notifyIntentUri (JNIEnv* env, jclass, jstring uri) { + qDebug() << __PRETTY_FUNCTION__; + const char *uriBytes = env->GetStringUTFChars(uri, NULL); QString uriStr(uriBytes); env->ReleaseStringUTFChars(uri, uriBytes); - QQmlApplicationEngine& engine(SingletonQmlEngine::instance()); - QObject* rootObj = engine.rootObjects()[0]; - QQuickWindow* mainWindow = qobject_cast(rootObj); - - if(mainWindow) - { - QMetaObject::invokeMethod(mainWindow, "handleIntentUri", - Q_ARG(QVariant, uriStr)); - } - else qCritical() << __PRETTY_FUNCTION__ << "Root object is not a window!"; + RsQmlAppEngine* engine = RsQmlAppEngine::mainInstance(); + if(engine) + QMetaObject::invokeMethod( + engine, "handleUri", + Qt::QueuedConnection, // BlockingQueuedConnection, AutoConnection + Q_ARG(QString, uriStr)); + else qCritical() << __PRETTY_FUNCTION__ << "RsQmlAppEngine::mainInstance()" + << "not initialized yet!"; } diff --git a/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareAndroidNotifyService.java b/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareAndroidNotifyService.java index d10dfe56d..5557aa14c 100644 --- a/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareAndroidNotifyService.java +++ b/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareAndroidNotifyService.java @@ -21,7 +21,6 @@ package org.retroshare.android.qml_app; import android.app.Notification; import android.app.NotificationManager; import android.app.PendingIntent; -import android.app.TaskStackBuilder; import android.content.Context; import android.content.Intent; import android.net.Uri; @@ -39,19 +38,23 @@ public class RetroShareAndroidNotifyService extends QtService .setContentText(text) .setAutoCancel(true); - Intent resultIntent = new Intent(this, RetroShareQmlActivity.class); - if(!uri.isEmpty()) resultIntent.setData(Uri.parse(uri)); + Intent intent = new Intent(this, RetroShareQmlActivity.class); + intent.setFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP); + intent.addFlags(Intent.FLAG_ACTIVITY_SINGLE_TOP); - TaskStackBuilder stackBuilder = TaskStackBuilder.create(this); - stackBuilder.addParentStack(RetroShareQmlActivity.class); - stackBuilder.addNextIntent(resultIntent); - PendingIntent resultPendingIntent = - stackBuilder.getPendingIntent( 0, - PendingIntent.FLAG_UPDATE_CURRENT ); + if(!uri.isEmpty()) intent.setData(Uri.parse(uri)); - mBuilder.setContentIntent(resultPendingIntent); + PendingIntent pendingIntent = PendingIntent.getActivity( + this, NOTIFY_REQ_CODE, intent, + PendingIntent.FLAG_UPDATE_CURRENT); + + mBuilder.setContentIntent(pendingIntent); NotificationManager mNotificationManager = (NotificationManager) this.getSystemService(Context.NOTIFICATION_SERVICE); mNotificationManager.notify(0, mBuilder.build()); } + + /** Must not be 0 otherwise a new activity may be created when should not + * (ex. the activity is already visible/on top) and deadlocks happens */ + private static final int NOTIFY_REQ_CODE = 2173; } diff --git a/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareQmlActivity.java b/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareQmlActivity.java index 59f142044..389ee1448 100644 --- a/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareQmlActivity.java +++ b/retroshare-qml-app/src/android/src/org/retroshare/android/qml_app/RetroShareQmlActivity.java @@ -33,6 +33,8 @@ public class RetroShareQmlActivity extends QtActivity @Override public void onCreate(Bundle savedInstanceState) { + Log.i("RetroShareQmlActivity", "onCreate()"); + if (!isMyServiceRunning(RetroShareAndroidService.class)) { Log.i("RetroShareQmlActivity", "onCreate(): RetroShareAndroidService is not running, let's start it by Intent"); @@ -55,13 +57,12 @@ public class RetroShareQmlActivity extends QtActivity @Override public void onNewIntent(Intent intent) { + Log.i("RetroShareQmlActivity", "onNewIntent(Intent intent)"); + super.onNewIntent(intent); + String uri = intent.getDataString(); - if (uri != null) - { - Log.d("RetroShareQmlActivity", "onNewIntent() " + uri); - NativeCalls.notifyIntentUri(uri); - } + if (uri != null) NativeCalls.notifyIntentUri(uri); } private boolean isMyServiceRunning(Class serviceClass) diff --git a/retroshare-qml-app/src/main.cpp b/retroshare-qml-app/src/main.cpp index a5442b4ab..e1fb4ce13 100644 --- a/retroshare-qml-app/src/main.cpp +++ b/retroshare-qml-app/src/main.cpp @@ -16,15 +16,22 @@ * along with this program. If not, see . */ +#include #include #include #include #include #include +#ifdef Q_OS_ANDROID +# include +# include +# include +#endif // Q_OS_ANDROID + #include "libresapilocalclient.h" #include "retroshare/rsinit.h" -#include "singletonqmlengine.h" +#include "rsqmlappengine.h" int main(int argc, char *argv[]) { @@ -43,17 +50,82 @@ int main(int argc, char *argv[]) LibresapiLocalClient rsApi; rsApi.openConnection(sockPath); - QQmlApplicationEngine& engine(SingletonQmlEngine::instance()); + RsQmlAppEngine engine(true); + QQmlContext& rootContext = *engine.rootContext(); + + QStringList mainArgs = app.arguments(); + +#ifdef Q_OS_ANDROID + rootContext.setContextProperty("Q_OS_ANDROID", true); + + /* Add Activity Intent data to args, because onNewIntent is called only if + * the Intet was triggered when the Activity was already created, so only in + * case onCreate is not called. + * The solution exposed in http://stackoverflow.com/a/36942185 is not + * adaptable to our case, because when onCreate is called the RsQmlAppEngine + * is not ready yet. + */ + uint waitCount = 0; + std::atomic waitIntent(true); + QString uriStr; + do + { + QtAndroid::runOnAndroidThread( + [&waitIntent, &uriStr]() + { + QAndroidJniObject activity = QtAndroid::androidActivity(); + if(!activity.isValid()) + { + qDebug() << "QtAndroid::runOnAndroidThread(...)" + << "activity not ready yet"; + return; + } + + QAndroidJniObject intent = activity.callObjectMethod( + "getIntent", "()Landroid/content/Intent;"); + if(!intent.isValid()) + { + qDebug() << "QtAndroid::runOnAndroidThread(...)" + << "intent not ready yet"; + return; + } + + QAndroidJniObject intentData = intent.callObjectMethod( + "getDataString", "()Ljava/lang/String;"); + if(intentData.isValid()) uriStr = intentData.toString(); + + waitIntent = false; + }); + + if(waitIntent) + { + qWarning() << "uriStr not ready yet after waiting" + << waitCount << "times"; + app.processEvents(); + ++waitCount; + usleep(10000); + } + } + while (waitIntent); + + qDebug() << "Got uriStr:" << uriStr; + + if(!uriStr.isEmpty()) mainArgs.append(uriStr); +#else + rootContext.setContextProperty("Q_OS_ANDROID", false); +#endif + + rootContext.setContextProperty("mainArgs", mainArgs); #ifdef QT_DEBUG - engine.rootContext()->setContextProperty("QT_DEBUG", true); + rootContext.setContextProperty("QT_DEBUG", true); #else - engine.rootContext()->setContextProperty("QT_DEBUG", false); + rootContext.setContextProperty("QT_DEBUG", false); #endif // QT_DEBUG - engine.rootContext()->setContextProperty("apiSocketPath", sockPath); - engine.rootContext()->setContextProperty("rsApi", &rsApi); - engine.load(QUrl(QLatin1String("qrc:/main.qml"))); + rootContext.setContextProperty("apiSocketPath", sockPath); + rootContext.setContextProperty("rsApi", &rsApi); + engine.load(QUrl(QLatin1String("qrc:/main-app.qml"))); return app.exec(); } diff --git a/retroshare-qml-app/src/main.qml b/retroshare-qml-app/src/main.qml index 410e701ce..82722f4ed 100644 --- a/retroshare-qml-app/src/main.qml +++ b/retroshare-qml-app/src/main.qml @@ -35,9 +35,21 @@ ApplicationWindow property bool coreReady: stackView.state === "running_ok" || stackView.state === "running_ok_no_full_control" - Component.onCompleted: addUriHandler("/certificate", certificateLinkHandler) + Component.onCompleted: + { + addUriHandler("/certificate", certificateLinkHandler) + + var argc = mainArgs.length + for(var i=0; i 1) { @@ -181,6 +195,9 @@ ApplicationWindow coreStateCheckTimer.stop() stackView.clear() stackView.push("qrc:/Contacts.qml") + while(mainWindow.pendingUriRegister.length > 0) + mainWindow.handleIntentUri( + mainWindow.pendingUriRegister.shift()) } } }, @@ -256,7 +273,12 @@ ApplicationWindow { console.log("certificateLinkHandler(uriStr)", coreReady) - if(!coreReady) return + if(!coreReady) + { + // Save cert uri for later processing as we need core to examine it + pendingUriRegister.push(uriStr) + return + } var uri = new UriJs.URI(uriStr) var uQuery = uri.search(true) diff --git a/retroshare-qml-app/src/retroshare-qml-app.pro b/retroshare-qml-app/src/retroshare-qml-app.pro index cf42339b9..338935419 100644 --- a/retroshare-qml-app/src/retroshare-qml-app.pro +++ b/retroshare-qml-app/src/retroshare-qml-app.pro @@ -4,12 +4,16 @@ QT += core network qml quick CONFIG += c++11 -HEADERS += libresapilocalclient.h singletonqmlengine.h -SOURCES += main.cpp libresapilocalclient.cpp +HEADERS += libresapilocalclient.h \ + rsqmlappengine.h +SOURCES += main-app.cpp \ + libresapilocalclient.cpp \ + rsqmlappengine.cpp RESOURCES += qml.qrc android-g++ { + QT += androidextras SOURCES += NativeCalls.cpp HEADERS += NativeCalls.h } diff --git a/retroshare-qml-app/src/singletonqmlengine.h b/retroshare-qml-app/src/rsqmlappengine.cpp similarity index 56% rename from retroshare-qml-app/src/singletonqmlengine.h rename to retroshare-qml-app/src/rsqmlappengine.cpp index 8239daf00..f6b70cf2c 100644 --- a/retroshare-qml-app/src/singletonqmlengine.h +++ b/retroshare-qml-app/src/rsqmlappengine.cpp @@ -1,6 +1,5 @@ -#pragma once /* - * libresapi local socket client + * RetroShare Qml App * Copyright (C) 2017 Gioacchino Mazzurco * * This program is free software: you can redistribute it and/or modify @@ -17,16 +16,25 @@ * along with this program. If not, see . */ -#include +#include "rsqmlappengine.h" -struct SingletonQmlEngine +#include +#include + + +/*static*/ RsQmlAppEngine* RsQmlAppEngine::mMainInstance = nullptr; + +void RsQmlAppEngine::handleUri(QString uri) { - static QQmlApplicationEngine& instance() - { - static QQmlApplicationEngine engine; - return engine; - } + QObject* rootObj = rootObjects()[0]; + QQuickWindow* mainWindow = qobject_cast(rootObj); -private: - SingletonQmlEngine(); -}; + if(mainWindow) + { + QMetaObject::invokeMethod(mainWindow, "handleIntentUri", + Qt::AutoConnection, + Q_ARG(QVariant, uri)); + } + else qCritical() << __PRETTY_FUNCTION__ + << "Root object is not a window!"; +} diff --git a/retroshare-qml-app/src/rsqmlappengine.h b/retroshare-qml-app/src/rsqmlappengine.h new file mode 100644 index 000000000..c9a29ce00 --- /dev/null +++ b/retroshare-qml-app/src/rsqmlappengine.h @@ -0,0 +1,67 @@ +#pragma once +/* + * RetroShare Qml App + * Copyright (C) 2017 Gioacchino Mazzurco + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +#include +#include + + +class RsQmlAppEngine : public QQmlApplicationEngine +{ + Q_OBJECT + +public: + + RsQmlAppEngine(bool isMainInstance = false, QObject* parent = nullptr) : + QQmlApplicationEngine(parent) + { if(isMainInstance) mMainInstance = this; } + + ~RsQmlAppEngine() { if(mMainInstance == this) mMainInstance = nullptr; } + + static RsQmlAppEngine* mainInstance() { return mMainInstance; } + +public slots: + void handleUri(QString uri); + +private: + + /** + * Using a static variable as QQmlApplicationEngine singleton caused crash + * on application termination, while using a dinamically allocated one + * causes deadlock on termination in QML WorkerScript destructor object + * with the following stacktrace + * #0 0x00007f0a8c3e4c67 in sched_yield () from /lib64/libc.so.6 + * #1 0x00007f0a8db5191c in QQuickWorkerScriptEngine::~QQuickWorkerScriptEngine() () from /usr/lib64/libQt5Qml.so.5 + * #2 0x00007f0a8db51949 in QQuickWorkerScriptEngine::~QQuickWorkerScriptEngine() () from /usr/lib64/libQt5Qml.so.5 + * #3 0x00007f0a8d61359c in QObjectPrivate::deleteChildren() () from /usr/lib64/libQt5Core.so.5 + * #4 0x00007f0a8d61ab83 in QObject::~QObject() () from /usr/lib64/libQt5Core.so.5 + * #5 0x00007f0a8da7d27d in QQmlEngine::~QQmlEngine() () from /usr/lib64/libQt5Qml.so.5 + * #6 0x00007f0a8dafeb39 in QQmlApplicationEngine::~QQmlApplicationEngine() () from /usr/lib64/libQt5Qml.so.5 + * #7 0x00007f0a8d61359c in QObjectPrivate::deleteChildren() () from /usr/lib64/libQt5Core.so.5 + * #8 0x00007f0a8d61ab83 in QObject::~QObject() () from /usr/lib64/libQt5Core.so.5 + * #9 0x00007f0a8d5ede58 in QCoreApplication::~QCoreApplication() () from /usr/lib64/libQt5Core.so.5 + * #10 0x00007f0a8dd2d97b in QGuiApplication::~QGuiApplication() () from /usr/lib64/libQt5Gui.so.5 + * #11 0x000000000041b6b4 in main (argc=1, argv=0x7fff218dd1a8) at ../../../../Development/rs-develop/retroshare-qml-app/src/main-app.cpp:58 + * + * To avoid this we leave the creation of the instance to the user (main) + * and to store the static pointer to that, the pointer can be null at early + * stage of execution (or if the user forget to initialize it properly) so + * early user (JNI intent handler) should take this in account + */ + static RsQmlAppEngine* mMainInstance; +};