QProcess/Unix: consolidate process state tracking socket notifiers

There is no reason to have the startup notifier and the death notifier
be active at the same time, as the former will detect death as well.

Previously, these notifiers were racing, but _q_processDied() ordered
signals by calling _q_startupNotification() manually. Thus, the
started()/finished() sequence was always emitted if the child process
was killed anywhere. Now this ordering is simply not necessary anymore.

This makes it possible to reuse the startup notifier for death
notification.

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

3 files changed, 30 insertions, 34 deletions

diff --git a/src/corelib/io/qprocess.cpp b/src/corelib/io/qprocess.cpp
index 2e3bd9cf59..5da0df8a4e 100644
--- a/src/corelib/io/qprocess.cpp
+++ b/src/corelib/io/qprocess.cpp
@@ -879,13 +879,9 @@ void QProcessPrivate::cleanup()
         delete stdinChannel.notifier;
         stdinChannel.notifier = nullptr;
     }
-    if (startupSocketNotifier) {
-        delete startupSocketNotifier;
-        startupSocketNotifier = nullptr;
-    }
-    if (deathNotifier) {
-        delete deathNotifier;
-        deathNotifier = nullptr;
+    if (stateNotifier) {
+        delete stateNotifier;
+        stateNotifier = nullptr;
     }
     closeChannel(&stdoutChannel);
     closeChannel(&stderrChannel);
@@ -1150,14 +1146,6 @@ void QProcessPrivate::_q_processDied()
     drainOutputPipes();
 #endif
 
-    // the process may have died before it got a chance to report that it was
-    // either running or stopped, so we will call _q_startupNotification() and
-    // give it a chance to emit started() or errorOccurred(FailedToStart).
-    if (processState == QProcess::Starting) {
-        if (!_q_startupNotification())
-            return;
-    }
-
     if (dying) {
         // at this point we know the process is dead. prevent
         // reentering this slot recursively by calling waitForFinished()
diff --git a/src/corelib/io/qprocess_p.h b/src/corelib/io/qprocess_p.h
index 58d68f83cf..a2d14d690a 100644
--- a/src/corelib/io/qprocess_p.h
+++ b/src/corelib/io/qprocess_p.h
@@ -339,8 +339,7 @@ public:
     Q_PIPE childStartedPipe[2] = {INVALID_Q_PIPE, INVALID_Q_PIPE};
     void destroyPipe(Q_PIPE pipe[2]);
 
-    QSocketNotifier *startupSocketNotifier = nullptr;
-    QSocketNotifier *deathNotifier = nullptr;
+    QSocketNotifier *stateNotifier = nullptr;
 
     int forkfd = -1;
 
diff --git a/src/corelib/io/qprocess_unix.cpp b/src/corelib/io/qprocess_unix.cpp
index c9ede9c3e9..1ca4a0d65e 100644
--- a/src/corelib/io/qprocess_unix.cpp
+++ b/src/corelib/io/qprocess_unix.cpp
@@ -378,9 +378,12 @@ void QProcessPrivate::startProcess()
     }
 
     if (threadData.loadRelaxed()->hasEventDispatcher()) {
-        startupSocketNotifier = new QSocketNotifier(childStartedPipe[0],
-                                                    QSocketNotifier::Read, q);
-        QObject::connect(startupSocketNotifier, SIGNAL(activated(QSocketDescriptor)),
+        // Set up to notify about startup completion (and premature death).
+        // Once the process has started successfully, we reconfigure the
+        // notifier to watch the fork_fd for expected death.
+        stateNotifier = new QSocketNotifier(childStartedPipe[0],
+                                            QSocketNotifier::Read, q);
+        QObject::connect(stateNotifier, SIGNAL(activated(QSocketDescriptor)),
                          q, SLOT(_q_startupNotification()));
     }
 
@@ -523,12 +526,6 @@ void QProcessPrivate::startProcess()
     }
     if (stderrChannel.pipe[0] != -1)
         ::fcntl(stderrChannel.pipe[0], F_SETFL, ::fcntl(stderrChannel.pipe[0], F_GETFL) | O_NONBLOCK);
-
-    if (threadData.loadRelaxed()->eventDispatcher.loadAcquire()) {
-        deathNotifier = new QSocketNotifier(forkfd, QSocketNotifier::Read, q);
-        QObject::connect(deathNotifier, SIGNAL(activated(QSocketDescriptor)),
-                         q, SLOT(_q_processDied()));
-    }
 }
 
 struct ChildError
@@ -582,13 +579,14 @@ report_errno:
 
 bool QProcessPrivate::processStarted(QString *errorMessage)
 {
+    Q_Q(QProcess);
+
     ChildError buf;
     int ret = qt_safe_read(childStartedPipe[0], &buf, sizeof(buf));
 
-    if (startupSocketNotifier) {
-        startupSocketNotifier->setEnabled(false);
-        startupSocketNotifier->deleteLater();
-        startupSocketNotifier = nullptr;
+    if (stateNotifier) {
+        stateNotifier->setEnabled(false);
+        stateNotifier->disconnect(q);
     }
     qt_safe_close(childStartedPipe[0]);
     childStartedPipe[0] = -1;
@@ -597,11 +595,22 @@ bool QProcessPrivate::processStarted(QString *errorMessage)
     qDebug("QProcessPrivate::processStarted() == %s", i <= 0 ? "true" : "false");
 #endif
 
+    if (ret <= 0) {  // process successfully started
+        if (stateNotifier) {
+            QObject::connect(stateNotifier, SIGNAL(activated(QSocketDescriptor)),
+                             q, SLOT(_q_processDied()));
+            stateNotifier->setSocket(forkfd);
+            stateNotifier->setEnabled(true);
+        }
+
+        return true;
+    }
+
     // did we read an error message?
-    if (ret > 0 && errorMessage)
+    if (errorMessage)
         *errorMessage = QLatin1String(buf.function) + QLatin1String(": ") + qt_error_string(buf.code);
 
-    return ret <= 0;
+    return false;
 }
 
 qint64 QProcessPrivate::bytesAvailableInChannel(const Channel *channel) const
@@ -852,8 +861,8 @@ void QProcessPrivate::waitForDeadChild()
     exitCode = info.status;
     crashed = info.code != CLD_EXITED;
 
-    delete deathNotifier;
-    deathNotifier = nullptr;
+    delete stateNotifier;
+    stateNotifier = nullptr;
 
     EINTR_LOOP(ret, forkfd_close(forkfd));
     forkfd = -1; // Child is dead, don't try to kill it anymore