Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-30251

QThread finished() signal is raised before running and finished attributes are set; thread mutex is unlocked prematurely causing crash due to user finished() slot executing and deleting the thread while it is still running.

    XMLWordPrintable

Details

    • Bug
    • Resolution: Done
    • Not Evaluated
    • 4.8.5
    • 4.8.3, 4.8.4
    • Core: Threads
    • None
    • Environment: hardware--8 core CPU @ 3.6 MHz, 16GB RAM; software--Linux Mint 14.1 using Qt 4.8.3 and Qt 4.8.4, and Windows 7 using Qt 4.8.4 MinGW.
    • 82566b811c2d0714f4a2bd23ef040b6e8a5f18bd

    Description

      This is a Qt 4.8.x regression bug. Qt 4.6.2 (Ubuntu 10.04 LTS) and Qt 4.7.4 (Linux Mint 14.1) and Qt 4.7.4 MinGW (Windows 7) work OK. My application runs 1,000 or more sub-processes asynchronously, served by a pool of 8 QThreads. Each process loads a QImage, scales it downward to thumbnail size, then creates and paints a QPixmap. The app works flawlessly in both Linux and Windows 7 using Qt 4.7.4 (and using Qt 4.6.2 in Linux). The app fails using Qt 4.8.3 in Mint 14.1, and fails using Qt 4.8.4 in both Mint 14.1 and Windows 7. The nature of the failure is that my QThread::finished() slot can no longer find a thread with the finished attribute set (in order to identify which thread emitted finished()).

      The issue is to be found in QThreadPrivate::finish() in both qthread_unix.cpp and qthread_win.cpp. In Qt 4.7.4, the running and finished attributes are correctly set prior to emitting finished(), and the thread mutex is unlocked at the end of the functions. The Qt 4.7.4 version of QThreadPrivate::finish(), which works correctly, is shown below:

      #ifdef Q_OS_SYMBIAN
      void QThreadPrivate::finish(void *arg, bool lockAnyway, bool closeNativeHandle)
      #else
      void QThreadPrivate::finish(void *arg)
      #endif
      {
          QThread *thr = reinterpret_cast<QThread *>(arg);
          QThreadPrivate *d = thr->d_func();
      #ifdef Q_OS_SYMBIAN
          if (lockAnyway)
      #endif
              d->mutex.lock();
      
          d->priority = QThread::InheritPriority;
          d->running = false;
          d->finished = true;
          if (d->terminated)
              emit thr->terminated();
          d->terminated = false;
          emit thr->finished();
      
          if (d->data->eventDispatcher) {
              d->data->eventDispatcher->closingDown();
              QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
              d->data->eventDispatcher = 0;
              delete eventDispatcher;
          }
      
          void *data = &d->data->tls;
          QThreadStorageData::finish((void **)data);
      
          d->thread_id = 0;
      #ifdef Q_OS_SYMBIAN
          if (closeNativeHandle)
              d->data->symbian_thread_handle.Close();
      #endif
          d->thread_done.wakeAll();
      #ifdef Q_OS_SYMBIAN
          if (lockAnyway)
      #endif
              d->mutex.unlock();
      }
      

      Again, note that the thread attributes are set before emitting finished(), and that the thread is unlocked at the very end of the function.

      Compare with the Qt 4.8.4 version below:

      void QThreadPrivate::finish(void *arg)
      {
          QThread *thr = reinterpret_cast<QThread *>(arg);
          QThreadPrivate *d = thr->d_func();
      
          QMutexLocker locker(&d->mutex);
      
          d->isInFinish = true;
          d->priority = QThread::InheritPriority;
          bool terminated = d->terminated;
          void *data = &d->data->tls;
          locker.unlock();
          if (terminated)
              emit thr->terminated();
          emit thr->finished();
          QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
          QThreadStorageData::finish((void **)data);
          locker.relock();
          d->terminated = false;
      
          QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
          if (eventDispatcher) {
              d->data->eventDispatcher = 0;
              locker.unlock();
              eventDispatcher->closingDown();
              delete eventDispatcher;
              locker.relock();
          }
      
          d->thread_id = 0;
          d->running = false;
          d->finished = true;
      
          d->isInFinish = false;
          d->thread_done.wakeAll();
      }
      

      Note that the thread attributes running and finished are incorrectly set AFTER finish() is emitted, and the thread mutex is incorrectly unlocked before these attributes are set and while the thread is still running. Even if I determine which thread finished by some alternate means, my finished() slot is allowed to run and to delete a still-running thread.

      My initial attempt at a fix simply placed setting running and finished prior to emitting finished(). This resulted in my being able to detect which thread emitted finished(), but caused a crash due to deleting a running thread (because the thread mutex is unlocked prematurely in 4.8.4). I then modified the code to keep the thread mutex locked until the bottom of QThreadPrivate::finished(). My app then ran normally as it had in Qt 4.6.2 and Qt 4.7.4. Below is suggested corrective code for qthread_unix.cpp (similar changes would be required for qthread_win.cpp):

      void QThreadPrivate::finish(void *arg)
      {
          QThread *thr = reinterpret_cast<QThread *>(arg);
          QThreadPrivate *d = thr->d_func();
      
          QMutexLocker locker(&d->mutex);
      
          d->isInFinish = true;
          d->priority = QThread::InheritPriority;
          bool terminated = d->terminated;
          void *data = &d->data->tls;
          if (terminated)
              emit thr->terminated();
          d->running = false;
          d->finished = true;
          emit thr->finished();
          QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
          QThreadStorageData::finish((void **)data);
      
          QAbstractEventDispatcher *eventDispatcher = d->data->eventDispatcher;
          if (eventDispatcher) {
              d->data->eventDispatcher = 0;
              eventDispatcher->closingDown();
              delete eventDispatcher;
          }
      
          d->isInFinish = false;
      
          d->thread_done.wakeAll();
      
          d->thread_id = 0;
       
         locker.unlock();
      }
      

      Again, the above changes enable my app, which is heavily multithreaded, to run on Qt 4.8.4.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            thiago Thiago Macieira
            feralurchin Jim Jensen
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes