Split QProcessPrivate::_q_processDied()

The completion of the child process can take place asynchronously or in
one of the waitFor...() functions. In both cases, we used the same
handler (_q_processDied()), which caused several problems:

  a. technically, waitForReadyRead() should have taken into account the
     result of the calls to _q_canRead...() slots inside the
     _q_processDied() function:

       - the user calls waitForReadyRead();
       - forkfd descriptor becomes signaled, while a grandchild
         process is still alive;
       - as readyRead() signal has not been emitted, _q_processDied()
         is called;
       - the grandchild process writes to stdout pipe;
       - now data arrives, and _q_processDied() will collect it, but
         won't report it.

  b. we had a bug with recursions on Unix:

       - death notification comes asynchronously;
       - waitForDeadChild() closes forkfd;
       - _q_canRead...() emits readyRead();
       - a slot connected to readyRead() calls waitForFinished();
       - waitForFinished() hangs (forkfd == -1).

   c. for blocking functions, drainOutputPipes() was called twice on
      Windows.

By introducing a new processFinished() function, we leave the read
operations in the _q_processDied() slot, while the process completion
code is guaranteed to run only once.

Change-Id: I5f9d09bc68a058169de4d9e490b48fc0b35e94cd
Reviewed-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

4 files changed, 48 insertions, 40 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 5da0df8a4e..48db7cdd54 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -1133,56 +1133,62 @@ bool QProcessPrivate::_q_canWrite()
 */
 void QProcessPrivate::_q_processDied()
 {
-    Q_Q(QProcess);
 #if defined QPROCESS_DEBUG
     qDebug("QProcessPrivate::_q_processDied()");
 #endif
+
+    // in case there is data in the pipeline and this slot by chance
+    // got called before the read notifications, call these functions
+    // so the data is made available before we announce death.
+#ifdef Q_OS_WIN
+    drainOutputPipes();
+#endif
+    _q_canReadStandardOutput();
+    _q_canReadStandardError();
+
+    // Slots connected to signals emitted by the functions called above
+    // might call waitFor*(), which would synchronously reap the process.
+    // So check the state to avoid trying to reap a second time.
+    if (processState != QProcess::NotRunning)
+        processFinished();
+}
+
+/*!
+    \internal
+*/
+void QProcessPrivate::processFinished()
+{
+    Q_Q(QProcess);
+#if defined QPROCESS_DEBUG
+    qDebug("QProcessPrivate::processFinished()");
+#endif
+
 #ifdef Q_OS_UNIX
     waitForDeadChild();
 #endif
 #ifdef Q_OS_WIN
     if (processFinishedNotifier)
         processFinishedNotifier->setEnabled(false);
-    drainOutputPipes();
 #endif
-
-    if (dying) {
-        // at this point we know the process is dead. prevent
-        // reentering this slot recursively by calling waitForFinished()
-        // or opening a dialog inside slots connected to the readyRead
-        // signals emitted below.
-        return;
-    }
-    dying = true;
-
-    // in case there is data in the pipe line and this slot by chance
-    // got called before the read notifications, call these two slots
-    // so the data is made available before the process dies.
-    _q_canReadStandardOutput();
-    _q_canReadStandardError();
-
     findExitCode();
 
+    cleanup();
+
     if (crashed) {
         exitStatus = QProcess::CrashExit;
         setErrorAndEmit(QProcess::Crashed);
     }
 
-    bool wasRunning = (processState == QProcess::Running);
+    // we received EOF now:
+    emit q->readChannelFinished();
+    // in the future:
+    //emit q->standardOutputClosed();
+    //emit q->standardErrorClosed();
 
-    cleanup();
-
-    if (wasRunning) {
-        // we received EOF now:
-        emit q->readChannelFinished();
-        // in the future:
-        //emit q->standardOutputClosed();
-        //emit q->standardErrorClosed();
+    emit q->finished(exitCode, exitStatus);
 
-        emit q->finished(exitCode, exitStatus);
-    }
 #if defined QPROCESS_DEBUG
-    qDebug("QProcessPrivate::_q_processDied() process is dead");
+    qDebug("QProcessPrivate::processFinished(): process is dead");
 #endif
 }
 
@@ -2253,7 +2259,6 @@ void QProcessPrivate::start(QIODevice::OpenMode mode)
     stderrChannel.closed = false;
 
     exitCode = 0;
-    dying = false;
     exitStatus = QProcess::NormalExit;
     processError = QProcess::UnknownError;
     errorString.clear();
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index a2d14d690a..4364d99c9b 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -309,7 +309,6 @@ public:
     qint64 pid = 0;
 #endif
 
-    bool dying = false;
     bool emittedReadyRead = false;
     bool emittedBytesWritten = false;
 
@@ -354,6 +353,7 @@ public:
     void execChild(const char *workingDirectory, char **argv, char **envp);
 #endif
     bool processStarted(QString *errorMessage = nullptr);
+    void processFinished();
     void terminateProcess();
     void killProcess();
     void findExitCode();
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index 1ca4a0d65e..17a1425d3f 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -754,8 +754,10 @@ bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline)
         if (processState == QProcess::NotRunning)
             return false;
 
+        // We do this after checking the pipes, so we cannot reach it as long
+        // as there is any data left to be read from an already dead process.
         if (qt_pollfd_check(poller.forkfd(), POLLIN)) {
-            _q_processDied();
+            processFinished();
             return false;
         }
     }
@@ -796,7 +798,7 @@ bool QProcessPrivate::waitForBytesWritten(const QDeadlineTimer &deadline)
             return false;
 
         if (qt_pollfd_check(poller.forkfd(), POLLIN)) {
-            _q_processDied();
+            processFinished();
             return false;
         }
     }
@@ -837,7 +839,7 @@ bool QProcessPrivate::waitForFinished(const QDeadlineTimer &deadline)
             return true;
 
         if (qt_pollfd_check(poller.forkfd(), POLLIN)) {
-            _q_processDied();
+            processFinished();
             return true;
         }
     }
@@ -850,8 +852,7 @@ void QProcessPrivate::findExitCode()
 
 void QProcessPrivate::waitForDeadChild()
 {
-    if (forkfd == -1)
-        return; // child has already been reaped
+    Q_ASSERT(forkfd != -1);
 
     // read the process information from our fd
     forkfd_info info;
diff --git a/src/corelib/io/qprocess_win.cpp b/src/corelib/io/qprocess_win.cpp
index 6ab854de16..2a5b8fd4fd 100644
--- a/src/corelib/io/qprocess_win.cpp
+++ b/src/corelib/io/qprocess_win.cpp
@@ -685,7 +685,7 @@ bool QProcessPrivate::waitForReadyRead(const QDeadlineTimer &deadline)
         if (WaitForSingleObjectEx(pid->hProcess, 0, false) == WAIT_OBJECT_0) {
             bool readyReadEmitted = drainOutputPipes();
             if (pid)
-                _q_processDied();
+                processFinished();
             return readyReadEmitted;
         }
 
@@ -743,7 +743,9 @@ bool QProcessPrivate::waitForBytesWritten(const QDeadlineTimer &deadline)
         // Wait for the process to signal any change in its state,
         // such as incoming data, or if the process died.
         if (WaitForSingleObjectEx(pid->hProcess, 0, false) == WAIT_OBJECT_0) {
-            _q_processDied();
+            drainOutputPipes();
+            if (pid)
+                processFinished();
             return false;
         }
 
@@ -782,7 +784,7 @@ bool QProcessPrivate::waitForFinished(const QDeadlineTimer &deadline)
         if (WaitForSingleObject(pid->hProcess, timer.nextSleepTime()) == WAIT_OBJECT_0) {
             drainOutputPipes();
             if (pid)
-                _q_processDied();
+                processFinished();
             return true;
         }