Improve handling of applications that are stopping or getting killed

- Differentiate between an app being killed and an app closing itself nicely.
  In the first case we may keep it around because it might have been killed
  by the OutOfMemory daemon. But in the second case we remove it from the
  app list.

- Only allow a Session to delete itself after it has been both released and
  stopped. This allows the application to always know when the session stops,
  narrowing down the possible combination (order) of events the Application
  state machine has to deal with.

- Break the cyclic reference between Application and Session before making the
  former delete the latter.

- Remove the unused Sesssion::aboutToBeDestroyed() signal

Change-Id: I7c5cd8da08fb98fa4e819024ab2d717577f4580d
Reviewed-by: Paul Olav Tvete <paul.tvete@theqtcompany.com>

6 files changed, 44 insertions, 15 deletions

diff --git a/src/modules/Unity/Application/application.cpp b/src/modules/Unity/Application/application.cpp
index 06471f7..5104894 100644
--- a/src/modules/Unity/Application/application.cpp
+++ b/src/modules/Unity/Application/application.cpp
@@ -79,7 +79,10 @@ Application::~Application()
         wipeQMLCache();
     }
 
-    delete m_session;
+    if (m_session) {
+        m_session->setApplication(nullptr);
+        delete m_session;
+    }
     delete m_desktopData;
 }
 
@@ -426,6 +429,9 @@ void Application::setSession(SessionInterface *newSession)
 
         if (oldFullscreen != fullscreen())
             Q_EMIT fullscreenChanged(fullscreen());
+    } else {
+        // this can only happen after the session has stopped and QML code called Session::release()
+        Q_ASSERT(m_state == InternalState::Stopped || m_state == InternalState::StoppedUnexpectedly);
     }
 
     Q_EMIT sessionChanged(m_session);
@@ -521,16 +527,32 @@ void Application::setProcessState(ProcessState newProcessState)
         Q_ASSERT(m_state == InternalState::SuspendingWaitProcess);
         setInternalState(InternalState::Suspended);
         break;
-    case ProcessStopped:
+    case ProcessKilled:
         // we assume the session always stop before the process
         Q_ASSERT(!m_session || m_session->state() == Session::Stopped);
+
         if (m_state == InternalState::Starting) {
+            // that was way too soon. let it go away
             setInternalState(InternalState::Stopped);
         } else {
             Q_ASSERT(m_state == InternalState::Stopped
                     || m_state == InternalState::StoppedUnexpectedly);
         }
         break;
+    case ProcessStopped:
+        // we assume the session always stop before the process
+        Q_ASSERT(!m_session || m_session->state() == Session::Stopped);
+
+        if (m_state == InternalState::Starting) {
+            // that was way too soon. let it go away
+            setInternalState(InternalState::Stopped);
+        } else if (m_state == InternalState::StoppedUnexpectedly) {
+            // the application stopped nicely, likely closed itself. Thus not really unexpected after all.
+            setInternalState(InternalState::Stopped);
+        } else {
+            Q_ASSERT(m_state == InternalState::Stopped);
+        }
+        break;
     }
 
     applyRequestedState();
diff --git a/src/modules/Unity/Application/application.h b/src/modules/Unity/Application/application.h
index 9344621..24a7a7f 100644
--- a/src/modules/Unity/Application/application.h
+++ b/src/modules/Unity/Application/application.h
@@ -61,6 +61,7 @@ public:
         ProcessUnknown,
         ProcessRunning,
         ProcessSuspended,
+        ProcessKilled, // it stopped, but because it was killed.
         ProcessStopped
     };
 
@@ -103,7 +104,7 @@ public:
 
     void setStage(Stage stage);
 
-
+    ProcessState processState() const { return m_processState; }
     void setProcessState(ProcessState value);
 
     QStringList arguments() const { return m_arguments; }
diff --git a/src/modules/Unity/Application/application_manager.cpp b/src/modules/Unity/Application/application_manager.cpp
index ba8bc9a..1aa5b82 100644
--- a/src/modules/Unity/Application/application_manager.cpp
+++ b/src/modules/Unity/Application/application_manager.cpp
@@ -465,7 +465,7 @@ void ApplicationManager::onProcessFailed(const QString &appId, const bool during
     }
 
     Q_UNUSED(duringStartup); // FIXME(greyback) upstart reports app that fully started up & crashes as failing during startup??
-    application->setProcessState(Application::ProcessStopped);
+    application->setProcessState(Application::ProcessKilled);
     application->setPid(0);
 }
 
@@ -481,8 +481,12 @@ void ApplicationManager::onProcessStopped(const QString &appId)
         return;
     }
 
-    application->setProcessState(Application::ProcessStopped);
-    application->setPid(0);
+    // if an application gets killed, onProcessFailed is called first, followed by onProcessStopped.
+    // we don't want to override what onProcessFailed already set.
+    if (application->processState() != Application::ProcessKilled) {
+        application->setProcessState(Application::ProcessStopped);
+        application->setPid(0);
+    }
 }
 
 void ApplicationManager::onProcessSuspended(const QString &appId)
diff --git a/src/modules/Unity/Application/session.cpp b/src/modules/Unity/Application/session.cpp
index 033e188..8908b09 100644
--- a/src/modules/Unity/Application/session.cpp
+++ b/src/modules/Unity/Application/session.cpp
@@ -52,6 +52,7 @@ Session::Session(const std::shared_ptr<ms::Session>& session,
     , m_fullscreen(false)
     , m_state(State::Starting)
     , m_live(true)
+    , m_released(false)
     , m_suspendTimer(new QTimer(this))
     , m_promptSessionManager(promptSessionManager)
 {
@@ -96,15 +97,10 @@ void Session::doSuspend()
 void Session::release()
 {
     qCDebug(QTMIR_SESSIONS) << "Session::release " << name();
-    Q_EMIT aboutToBeDestroyed();
 
-    if (m_parentSession) {
-        m_parentSession->removeChildSession(this);
-    }
-    if (m_application) {
-        m_application->setSession(nullptr);
-    }
-    if (!parent()) {
+    m_released = true;
+
+    if (m_state == Stopped) {
         deleteLater();
     }
 }
@@ -304,6 +300,9 @@ void Session::stop()
         });
 
         setState(Stopped);
+        if (m_released) {
+            deleteLater();
+        }
     }
 }
 
@@ -314,6 +313,9 @@ void Session::setLive(const bool live)
         Q_EMIT liveChanged(m_live);
         if (!live) {
             setState(Stopped);
+            if (m_released) {
+                deleteLater();
+            }
         }
     }
 }
diff --git a/src/modules/Unity/Application/session.h b/src/modules/Unity/Application/session.h
index f80d36e..1f9fbb6 100644
--- a/src/modules/Unity/Application/session.h
+++ b/src/modules/Unity/Application/session.h
@@ -105,6 +105,7 @@ private:
     bool m_fullscreen;
     State m_state;
     bool m_live;
+    bool m_released;
     QTimer* m_suspendTimer;
     QList<std::shared_ptr<mir::scene::PromptSession>> m_promptSessions;
     std::shared_ptr<mir::scene::PromptSessionManager> const m_promptSessionManager;
diff --git a/src/modules/Unity/Application/session_interface.h b/src/modules/Unity/Application/session_interface.h
index 819b581..be00893 100644
--- a/src/modules/Unity/Application/session_interface.h
+++ b/src/modules/Unity/Application/session_interface.h
@@ -101,7 +101,6 @@ Q_SIGNALS:
     void surfaceChanged(MirSurfaceItemInterface*);
     void parentSessionChanged(SessionInterface*);
     void applicationChanged(unity::shell::application::ApplicationInfoInterface* application);
-    void aboutToBeDestroyed();
     void stateChanged(State state);
     void fullscreenChanged(bool fullscreen);
     void liveChanged(bool live);