Fix crash when interrupting QBluetoothSocket's input stream thread

The previous QThread did not always properly resume when
InputStream.read() was interrupted by BluetoothSocket.close().
This patch converts the QThread to a Java thread which works
as the Android API docs suggested.

Task-number: QTBUG-37061
Change-Id: Id6ac9b57a28f3b532cbe49ff1dfdc9d1e6432aaa
Reviewed-by: Nedim Hadzic <nedimhadzija@gmail.com>
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@digia.com>

5 files changed, 148 insertions, 100 deletions

diff --git a/src/bluetooth/android/inputstreamthread.cpp b/src/bluetooth/android/inputstreamthread.cpp
index df32ee62..7f5029d9 100644
--- a/src/bluetooth/android/inputstreamthread.cpp
+++ b/src/bluetooth/android/inputstreamthread.cpp
@@ -40,6 +40,7 @@
 **
 ****************************************************************************/
 
+#include <QtCore/QLoggingCategory>
 #include <QtAndroidExtras/QAndroidJniEnvironment>
 
 #include "android/inputstreamthread_p.h"
@@ -47,34 +48,29 @@
 
 QT_BEGIN_NAMESPACE
 
+Q_DECLARE_LOGGING_CATEGORY(QT_BT_ANDROID)
 
 InputStreamThread::InputStreamThread(QBluetoothSocketPrivate *socket)
-    : QThread(), m_stop(false)
+    : QObject(), m_socket_p(socket), expectClosure(false)
 {
-    m_socket_p = socket;
 }
 
-void InputStreamThread::run()
+bool InputStreamThread::run()
 {
-    qint32 byte;
-    Q_UNUSED(byte)
-    while (1) {
-        {
-            QMutexLocker locker(&m_mutex);
-            if (m_stop)
-                break;
-        }
-        readFromInputStream();
-    }
+    QMutexLocker lock(&m_mutex);
 
-    QAndroidJniEnvironment env;
-    if (m_socket_p->inputStream.isValid())
-        m_socket_p->inputStream.callMethod<void>("close");
+    javaInputStreamThread = QAndroidJniObject("org/qtproject/qt5/android/bluetooth/QtBluetoothInputStreamThread");
+    if (!javaInputStreamThread.isValid() || !m_socket_p->inputStream.isValid())
+        return false;
+
+    javaInputStreamThread.callMethod<void>("setInputStream", "(Ljava/io/InputStream;)V",
+                                           m_socket_p->inputStream.object<jobject>());
+    javaInputStreamThread.setField<jlong>("qtObject", reinterpret_cast<long>(this));
+    javaInputStreamThread.setField<jboolean>("logEnabled", QT_BT_ANDROID().isDebugEnabled());
+
+    javaInputStreamThread.callMethod<void>("start");
 
-    if (env->ExceptionCheck()) {
-        env->ExceptionDescribe();
-        env->ExceptionClear();
-    }
+    return true;
 }
 
 bool InputStreamThread::bytesAvailable() const
@@ -83,68 +79,42 @@ bool InputStreamThread::bytesAvailable() const
     return m_socket_p->buffer.size();
 }
 
-//This runs inside the thread.
-void InputStreamThread::readFromInputStream()
+qint64 InputStreamThread::readData(char *data, qint64 maxSize)
 {
-    QAndroidJniEnvironment env;
-
-    int bufLen = 1000; // Seems to magical number that also low-end products can survive.
-    jbyteArray nativeArray = env->NewByteArray(bufLen);
-
+    QMutexLocker locker(&m_mutex);
 
-    jint ret = m_socket_p->inputStream.callMethod<jint>("read", "([BII)I", nativeArray, 0, bufLen);
+    if (!m_socket_p->buffer.isEmpty())
+        return m_socket_p->buffer.read(data, maxSize);
 
-    if (env->ExceptionCheck() || ret < 0) {
-        if (env->ExceptionCheck()) {
-            env->ExceptionDescribe();
-            env->ExceptionClear();
-        }
-        env->DeleteLocalRef(nativeArray);
-        QMutexLocker lock(&m_mutex);
-        m_stop = true;
+    return 0;
+}
 
-        /*
-         * We cannot distinguish IOException due to valid closure or due to other error
-         * Therefore we always have to throw an error and a disconnect signal
-         * A genuine disconnect wouldn't need the error signal.
-         * For now we always signal error which implicitly emits disconnect() too.
-         */
+//inside the java thread
+void InputStreamThread::javaThreadErrorOccurred(int errorCode)
+{
+    QMutexLocker lock(&m_mutex);
 
-        emit error();
-        return;
-    }
+    if (!expectClosure)
+        emit error(errorCode);
+    else
+        emit error(-1); //magic error, -1 means error was expected due to expected close()
+}
 
-    if (ret == 0) {
-        qDebug() << "Nothing to read";
-        env->DeleteLocalRef(nativeArray);
-        return;
-    }
+//inside the java thread
+void InputStreamThread::javaReadyRead(jbyteArray buffer, int bufferLength)
+{
+    QAndroidJniEnvironment env;
 
     QMutexLocker lock(&m_mutex);
-    char *writePtr = m_socket_p->buffer.reserve(bufLen);
-    env->GetByteArrayRegion(nativeArray, 0, ret, reinterpret_cast<jbyte*>(writePtr));
-    env->DeleteLocalRef(nativeArray);
-    m_socket_p->buffer.chop(bufLen - ret);
+    char *writePtr = m_socket_p->buffer.reserve(bufferLength);
+    env->GetByteArrayRegion(buffer, 0, bufferLength, reinterpret_cast<jbyte*>(writePtr));
     emit dataAvailable();
 }
 
-void InputStreamThread::stop()
-{
-    QMutexLocker locker(&m_mutex);
-    m_stop = true;
-}
-
-qint64 InputStreamThread::readData(char *data, qint64 maxSize)
+void InputStreamThread::prepareForClosure()
 {
-    QMutexLocker locker(&m_mutex);
-
-    if (m_stop)
-        return -1;
-
-    if (!m_socket_p->buffer.isEmpty())
-        return m_socket_p->buffer.read(data, maxSize);
-
-    return 0;
+    QMutexLocker lock(&m_mutex);
+    expectClosure = true;
 }
 
 QT_END_NAMESPACE
diff --git a/src/bluetooth/android/inputstreamthread_p.h b/src/bluetooth/android/inputstreamthread_p.h
index 85852534..8b565cff 100644
--- a/src/bluetooth/android/inputstreamthread_p.h
+++ b/src/bluetooth/android/inputstreamthread_p.h
@@ -43,34 +43,39 @@
 #ifndef INPUTSTREAMTHREAD_H
 #define INPUTSTREAMTHREAD_H
 
-#include <QtCore/QThread>
+#include <QtCore/QObject>
 #include <QtCore/QMutex>
+#include <QtAndroidExtras/QAndroidJniObject>
+#include <jni.h>
 
 QT_BEGIN_NAMESPACE
 
 class QBluetoothSocketPrivate;
 
-class InputStreamThread : public QThread
+class InputStreamThread : public QObject
 {
     Q_OBJECT
 public:
     explicit InputStreamThread(QBluetoothSocketPrivate *socket_p);
-    virtual void run();
 
     bool bytesAvailable() const;
-    void stop();
+    bool run();
 
     qint64 readData(char *data, qint64 maxSize);
+    void javaThreadErrorOccurred(int errorCode);
+    void javaReadyRead(jbyteArray buffer, int bufferLength);
+
+    void prepareForClosure();
+
 signals:
     void dataAvailable();
-    void error();
+    void error(int errorCode);
 
 private:
-    void readFromInputStream();
-
     QBluetoothSocketPrivate *m_socket_p;
+    QAndroidJniObject javaInputStreamThread;
     mutable QMutex m_mutex;
-    bool m_stop;
+    bool expectClosure;
 };
 
 QT_END_NAMESPACE
diff --git a/src/bluetooth/android/jni_android.cpp b/src/bluetooth/android/jni_android.cpp
index ce0f19ca..1e907fdd 100644
--- a/src/bluetooth/android/jni_android.cpp
+++ b/src/bluetooth/android/jni_android.cpp
@@ -47,6 +47,7 @@
 #include <QtAndroidExtras/QAndroidJniObject>
 #include "android/androidbroadcastreceiver_p.h"
 #include "android/serveracceptancethread_p.h"
+#include "android/inputstreamthread_p.h"
 
 Q_DECLARE_LOGGING_CATEGORY(QT_BT_ANDROID)
 
@@ -68,6 +69,19 @@ static void QtBluetoothSocketServer_newSocket(JNIEnv */*env*/, jobject /*javaObj
     reinterpret_cast<ServerAcceptanceThread*>(qtObject)->javaNewSocket(socket);
 }
 
+static void QtBluetoothInputStreamThread_errorOccurred(JNIEnv */*env*/, jobject /*javaObject*/,
+                                           jlong qtObject, jint errorCode)
+{
+    reinterpret_cast<InputStreamThread*>(qtObject)->javaThreadErrorOccurred(errorCode);
+}
+
+static void QtBluetoothInputStreamThread_readyData(JNIEnv */*env*/, jobject /*javaObject*/,
+                                       jlong qtObject, jbyteArray buffer, jint bufferLength)
+{
+    reinterpret_cast<InputStreamThread*>(qtObject)->javaReadyRead(buffer, bufferLength);
+}
+
+
 static JNINativeMethod methods[] = {
     {"jniOnReceive", "(JLandroid/content/Context;Landroid/content/Intent;)V",
                 (void *) QtBroadcastReceiver_jniOnReceive},
@@ -80,6 +94,13 @@ static JNINativeMethod methods_server[] = {
                     (void *) QtBluetoothSocketServer_newSocket},
 };
 
+static JNINativeMethod methods_inputStream[] = {
+        {"errorOccurred", "(JI)V",
+                    (void *) QtBluetoothInputStreamThread_errorOccurred},
+        {"readyData", "(J[BI)V",
+                    (void *) QtBluetoothInputStreamThread_readyData},
+};
+
 static const char logTag[] = "QtBluetooth";
 static const char classErrorMsg[] = "Can't find class \"%s\"";
 
@@ -107,6 +128,13 @@ static bool registerNatives(JNIEnv *env)
         return false;
     }
 
+    FIND_AND_CHECK_CLASS("org/qtproject/qt5/android/bluetooth/QtBluetoothInputStreamThread");
+    if (env->RegisterNatives(clazz, methods_inputStream,
+                             sizeof(methods_inputStream) / sizeof(methods_inputStream[0])) < 0) {
+        __android_log_print(ANDROID_LOG_FATAL, logTag, "RegisterNatives for InputStreamThread failed");
+        return false;
+    }
+
     return true;
 }
 
diff --git a/src/bluetooth/qbluetoothsocket_android.cpp b/src/bluetooth/qbluetoothsocket_android.cpp
index 9f04c69e..5c84e627 100644
--- a/src/bluetooth/qbluetoothsocket_android.cpp
+++ b/src/bluetooth/qbluetoothsocket_android.cpp
@@ -170,9 +170,7 @@ void QBluetoothSocketPrivate::connectToServiceConc(const QBluetoothAddress &addr
     }
 
     if (inputThread) {
-        inputThread->stop();
-        inputThread->wait();
-        delete inputThread;
+        inputThread->deleteLater();
         inputThread = 0;
     }
 
@@ -200,10 +198,29 @@ void QBluetoothSocketPrivate::connectToServiceConc(const QBluetoothAddress &addr
     }
 
     inputThread = new InputStreamThread(this);
-    QObject::connect(inputThread, SIGNAL(dataAvailable()), q, SIGNAL(readyRead()), Qt::QueuedConnection);
+    QObject::connect(inputThread, SIGNAL(dataAvailable()),
+                     q, SIGNAL(readyRead()), Qt::QueuedConnection);
     QObject::connect(inputThread, SIGNAL(error()),
                      this, SLOT(inputThreadError()), Qt::QueuedConnection);
-    inputThread->start();
+
+    if (!inputThread->run()) {
+        //close socket again
+        socketObject.callMethod<void>("close");
+        if (env->ExceptionCheck()) {
+            env->ExceptionDescribe();
+            env->ExceptionClear();
+        }
+
+        socketObject = inputStream = outputStream = remoteDevice = QAndroidJniObject();
+
+        delete inputThread;
+        inputThread = 0;
+
+        errorString = QBluetoothSocket::tr("Input stream thread cannot be started");
+        q->setSocketError(QBluetoothSocket::NetworkError);
+        q->setSocketState(QBluetoothSocket::UnconnectedState);
+        return;
+    }
 
     q->setSocketState(QBluetoothSocket::ConnectedState);
     emit q->connected();
@@ -232,6 +249,10 @@ void QBluetoothSocketPrivate::abort()
          * new state, error and emits relevant signals.
          * See QBluetoothSocketPrivate::inputThreadError() for details
          */
+
+        if (inputThread)
+            inputThread->prepareForClosure();
+
         //triggers abort of input thread as well
         socketObject.callMethod<void>("close");
         if (env->ExceptionCheck()) {
@@ -242,9 +263,9 @@ void QBluetoothSocketPrivate::abort()
         }
 
         if (inputThread) {
-            inputThread->stop();
-            inputThread->wait();
-            delete inputThread;
+            //don't delete here as signals caused by Java Thread are still
+            //going to be emitted
+            //delete occurs in inputThreadError()
             inputThread = 0;
         }
 
@@ -306,7 +327,7 @@ qint64 QBluetoothSocketPrivate::writeData(const char *data, qint64 maxSize)
     //TODO check that readData and writeData return -1 on error (on all platforms)
     Q_Q(QBluetoothSocket);
     if (state != QBluetoothSocket::ConnectedState || !outputStream.isValid()) {
-        qCWarning(QT_BT_ANDROID) << "Socket::writeData: " << (int)state << outputStream.isValid() ;
+        qCWarning(QT_BT_ANDROID) << "Socket::writeData: " << state << outputStream.isValid();
         errorString = QBluetoothSocket::tr("Cannot write while not connected");
         q->setSocketError(QBluetoothSocket::OperationError);
         return -1;
@@ -334,7 +355,7 @@ qint64 QBluetoothSocketPrivate::readData(char *data, qint64 maxSize)
 {
     Q_Q(QBluetoothSocket);
     if (state != QBluetoothSocket::ConnectedState || !inputThread) {
-        qCWarning(QT_BT_ANDROID) << "Socket::writeData: " << (int)state << outputStream.isValid() ;
+        qCWarning(QT_BT_ANDROID) << "Socket::readData: " << state << inputThread ;
         errorString = QBluetoothSocket::tr("Cannot write while not connected");
         q->setSocketError(QBluetoothSocket::OperationError);
         return -1;
@@ -343,13 +364,38 @@ qint64 QBluetoothSocketPrivate::readData(char *data, qint64 maxSize)
     return inputThread->readData(data, maxSize);
 }
 
-void QBluetoothSocketPrivate::inputThreadError()
+void QBluetoothSocketPrivate::inputThreadError(int errorCode)
 {
     Q_Q(QBluetoothSocket);
 
-    //any error from InputThread is a NetworkError
-    errorString = QBluetoothSocket::tr("Network error during read");
-    q->setSocketError(QBluetoothSocket::NetworkError);
+    if (errorCode != -1) { //magic error which is expected and can be ignored
+        errorString = QBluetoothSocket::tr("Network error during read");
+        q->setSocketError(QBluetoothSocket::NetworkError);
+    }
+
+    //finally we can delete the InputStreamThread
+    InputStreamThread *client = qobject_cast<InputStreamThread *>(sender());
+    if (client)
+        client->deleteLater();
+
+    if (socketObject.isValid()) {
+        //triggered when remote side closed the socket
+        //cleanup internal objects
+        //if it was call to local close()/abort() the objects are cleaned up already
+
+        bool stillConnected = socketObject.callMethod<jboolean>("isConnected");
+        if (stillConnected) {
+            QAndroidJniEnvironment env;
+            socketObject.callMethod<void>("close");
+            if (env->ExceptionCheck()) {
+                env->ExceptionDescribe();
+                env->ExceptionClear();
+            }
+        }
+
+        inputStream = outputStream = remoteDevice = socketObject = QAndroidJniObject();
+    }
+
     q->setSocketState(QBluetoothSocket::UnconnectedState);
     emit q->disconnected();
 }
@@ -414,16 +460,15 @@ bool QBluetoothSocketPrivate::setSocketDescriptor(const QAndroidJniObject &socke
     remoteDevice = socketObject.callObjectMethod("getRemoteDevice", "()Landroid/bluetooth/BluetoothDevice;");
 
     if (inputThread) {
-        inputThread->stop();
-        inputThread->wait();
-        delete inputThread;
+        inputThread->deleteLater();
         inputThread = 0;
     }
     inputThread = new InputStreamThread(this);
-    QObject::connect(inputThread, SIGNAL(dataAvailable()), q, SIGNAL(readyRead()), Qt::QueuedConnection);
-    QObject::connect(inputThread, SIGNAL(error()),
-                     this, SLOT(inputThreadError()), Qt::QueuedConnection);
-    inputThread->start();
+    QObject::connect(inputThread, SIGNAL(dataAvailable()),
+                     q, SIGNAL(readyRead()), Qt::QueuedConnection);
+    QObject::connect(inputThread, SIGNAL(error(int)),
+                     this, SLOT(inputThreadError(int)), Qt::QueuedConnection);
+    inputThread->run();
 
 
     q->setSocketState(socketState);
diff --git a/src/bluetooth/qbluetoothsocket_p.h b/src/bluetooth/qbluetoothsocket_p.h
index 2e1bb66f..2fabeba1 100644
--- a/src/bluetooth/qbluetoothsocket_p.h
+++ b/src/bluetooth/qbluetoothsocket_p.h
@@ -170,7 +170,7 @@ public:
     InputStreamThread *inputThread;
 
 private Q_SLOTS:
-    void inputThreadError();
+    void inputThreadError(int errorCode);
 
 #endif