QtMir

Merge lp:~nick-dedekind/qtmir/polite-close into lp:qtmir

  1. polite-close
  2. Merge into trunk
Proposed by Nick Dedekind on 2015-06-17
Status: Merged
Approved by: Daniel d'Andrada on 2015-09-29
Approved revision: 359
Merged at revision: 424
Proposed branch: lp:~nick-dedekind/qtmir/polite-close
Merge into: lp:qtmir
Prerequisite: lp:~dandrader/qtmir/detach-state-from-focus
Diff against target: 726 lines (+343/-24)
17 files modified
debian/changelog (+7/-0)
src/modules/Unity/Application/application.cpp (+42/-9)
src/modules/Unity/Application/application.h (+7/-0)
src/modules/Unity/Application/application_manager.cpp (+52/-12)
src/modules/Unity/Application/application_manager.h (+3/-0)
src/modules/Unity/Application/mirsurface.cpp (+7/-0)
src/modules/Unity/Application/mirsurface.h (+2/-0)
src/modules/Unity/Application/mirsurfaceinterface.h (+2/-0)
src/modules/Unity/Application/session.cpp (+40/-2)
src/modules/Unity/Application/session.h (+1/-0)
src/modules/Unity/Application/session_interface.h (+1/-0)
tests/modules/Application/CMakeLists.txt (+5/-0)
tests/modules/ApplicationManager/application_manager_test.cpp (+155/-0)
tests/modules/common/fake_mirsurface.h (+7/-0)
tests/modules/common/fake_session.h (+4/-0)
tests/modules/common/mock_session.h (+1/-0)
tests/modules/common/qtmir_test.h (+7/-1)
To merge this branch: bzr merge lp:~nick-dedekind/qtmir/polite-close
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration 2015-06-17 Needs Fixing on 2015-12-04
Daniel d'Andrada (community) Approve on 2015-09-29
Gerry Boland 2015-06-17 Pending
Review via email: mp+262188@code.launchpad.net

This proposal supersedes a proposal from 2015-06-09.

Commit Message

Politely asks processes to close before resorting to killing

Description of the Change

Politely close applications.

 * Are there any related MPs required for this MP to build/function as expected? Please list.
https://code.launchpad.net/~nick-dedekind/platform-api/lp1351109.destroy-haptic-instance/+merge/273190
https://code.launchpad.net/~nick-dedekind/qtubuntu-sensors/lp1351109.destroy-haptic-instance/+merge/272755

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Can I get you to rebase this on top of
https://code.launchpad.net/~dandrader/qtmir/detach-state-from-focus/+merge/258648
as I expect collisions. Daniel re-worked the Application class to more clearly manage process state, which this branch would benefit from.

Something to keep in mind is we may be treating applications which don't close when we ask them differently on desktop & mobile platforms. It's not uncommon for desktop apps to pop up a "Save" dialog to prevent user loosing their work. For mobile, we may have this issue too, though I've no idea how we'll present it to the user.

As a result I think the "kill after 3 seconds" logic should belong to unity8.

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:348
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/295/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/28/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/28/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/295/rebuild

review: Needs Fixing (continuous-integration)
Nick Dedekind (nick-dedekind) wrote :

Logic for how a close request reacts (popup for confirm/error notification for hard exit/etc) will be dealt with by subsequent sprint cards. This is just a only a first pass.

PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:353
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/299/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/32/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/32/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/299/rebuild

review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:354
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~nick-dedekind/qtmir/polite-close/+merge/262188/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/qtmir-ci/300/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/33/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/33/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/300/rebuild

review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~nick-dedekind/qtmir/polite-close updated on 2015-07-03
346. By Daniel d'Andrada <dandrader@panzer> on 2015-06-30

merge detach-state-from-focus

347. By Daniel d'Andrada <dandrader@panzer> on 2015-06-30

Merge lp:~nick-dedekind/qtmir/polite-close

348. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Modifications from review

349. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Format is EXPECT_EQ(expected, actual)

350. By Daniel d'Andrada <dandrader@panzer> on 2015-07-01

Test change that makes it hang

Daniel d'Andrada (dandrader) wrote :

I think you're missing a clear definition of the Closing state and of the transitions from and to it. In the code I see it can be in the Closing state even if the process is suspended. If so, you resume it. This leads to uncertainty in the code as it can be in Closing but you don't know if it's suspended or running. This compromises the idea of tracking the internal state through this variable as it no longer clearly reflects what's going on.

In my opinion, an app is in Closing state *only* when:
 - Its process is Running (or Unknown, which amounts to the same)
 - The closing timer is ticking

So if someone calls Application::close() while the app is Suspended. You should first resume() it and then set to Closing. Think of the state transitions. Suspended -> Running -> Closing. It can only enter Closing from Running. Not straight from Suspended.

The related code changes for this and some other small issues are here:
http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/348

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote :

Format is EXPECT_EQ(expected, actual), not the other way around (yeah, it's the opposite of most other test APIs).

http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/349

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote :

ApplicationManagerTests.applicationStartQueuedOnStartStopStart is stopping the app while is still in Starting state. The more common case, which is what we should be testing, is closing an app in Running state. I changed the test accordingly:

http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/350

It needs more work though, as you will have to mock/fake more stuff to make this code path function properly, like adding some fake implementation to the TaskController. so that AppMan calling TaskController->stopProcess() causes the app session to end and then TaskController::processStopped to get emited.

Daniel d'Andrada (dandrader) wrote :

1 - Launch some app (e.g. filemanager)
2 - $ sudo gdb -p $(pidof filemanager)
3 - (gdb) cont
4 - do right-edge drag to show the spread and swipe up filemanager to close it.

expected outcome:
You see in gdb that app exits nicely

Actual outcome:
You get: "Program received signal SIGSEGV, Segmentation fault."

Comments:
Without this branch I get "Program received signal SIGTERM, Terminated." instead.

It might not be our fault be we should at least know why it's crashing.

review: Needs Fixing
lp:~nick-dedekind/qtmir/polite-close updated on 2015-09-07
351. By Nick Dedekind on 2015-09-07

merged with trunk

352. By Nick Dedekind on 2015-09-07

fixed up a few resume on closing things

353. By Nick Dedekind on 2015-09-07

fixed up some logs

Nick Dedekind (nick-dedekind) wrote :

> 1 - Launch some app (e.g. filemanager)
> 2 - $ sudo gdb -p $(pidof filemanager)
> 3 - (gdb) cont
> 4 - do right-edge drag to show the spread and swipe up filemanager to close
> it.
>
> expected outcome:
> You see in gdb that app exits nicely
>
> Actual outcome:
> You get: "Program received signal SIGSEGV, Segmentation fault."
>
> Comments:
> Without this branch I get "Program received signal SIGTERM, Terminated."
> instead.
>
> It might not be our fault be we should at least know why it's crashing.

This can be dealt with by another branch. the problem occurs in trunk as well.
tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

lp:~nick-dedekind/qtmir/polite-close updated on 2015-09-07
354. By Nick Dedekind on 2015-09-07

fixed close for sync

355. By Nick Dedekind on 2015-09-07

updated tests

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Nick Dedekind (nick-dedekind) wrote :

> ApplicationManagerTests.applicationStartQueuedOnStartStopStart is stopping the
> app while is still in Starting state. The more common case, which is what we
> should be testing, is closing an app in Running state. I changed the test
> accordingly:
>
> http://bazaar.launchpad.net/~dandrader/qtmir/polite-close/revision/350
>
> It needs more work though, as you will have to mock/fake more stuff to make
> this code path function properly, like adding some fake implementation to the
> TaskController. so that AppMan calling TaskController->stopProcess() causes
> the app session to end and then TaskController::processStopped to get emited.

Thanks for the updates. I've merged in your work.
I've just called the appManager->onProcessStopped directly for now to get the cleanup working. Mocking the TaskController is quite a big job as most of the app manager tests rely the production task controller with a mocked app controller.

I've got another branch in the works for mocking the task controller. Can be merged separately.

Nick Dedekind (nick-dedekind) wrote :

> 1 - Launch some app (e.g. filemanager)
> 2 - $ sudo gdb -p $(pidof filemanager)
> 3 - (gdb) cont
> 4 - do right-edge drag to show the spread and swipe up filemanager to close
> it.
>
> expected outcome:
> You see in gdb that app exits nicely
>
> Actual outcome:
> You get: "Program received signal SIGSEGV, Segmentation fault."
>
> Comments:
> Without this branch I get "Program received signal SIGTERM, Terminated."
> instead.
>
> It might not be our fault be we should at least know why it's crashing.

I'll look into it when I get a minute. Seems to be crashing in libdbus-cpp.

Daniel d'Andrada (dandrader) wrote :

Code looks good. Didn't try it out on a device yet.

Two problems in the tests though:

1 - ApplicationManagerTests.shellStopsAppCorrectlyBeforeSurfaceCreated is failing:
http://paste.ubuntu.com/12418245/

2 - ApplicationManagerTests::failedApplicationCloseEventualyDeletesApplication test description (the comment above it) doesn't match with what the test does. Probably a copy-and-paste left over.

review: Needs Fixing
Daniel d'Andrada (dandrader) wrote :

> > 1 - Launch some app (e.g. filemanager)
> > 2 - $ sudo gdb -p $(pidof filemanager)
> > 3 - (gdb) cont
> > 4 - do right-edge drag to show the spread and swipe up filemanager to close
> > it.
> >
> > expected outcome:
> > You see in gdb that app exits nicely
> >
> > Actual outcome:
> > You get: "Program received signal SIGSEGV, Segmentation fault."
> >
> > Comments:
> > Without this branch I get "Program received signal SIGTERM, Terminated."
> > instead.
> >
> > It might not be our fault be we should at least know why it's crashing.
>
> This can be dealt with by another branch. the problem occurs in trunk as well.
> tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

Just tried three times with both messaging-app and dialer-app and didn't get any SIGSEGV with qtmir trunk.

Gerry Boland (gerboland) wrote :

> > 1 - Launch some app (e.g. filemanager)
> > 2 - $ sudo gdb -p $(pidof filemanager)
> > 3 - (gdb) cont
> > 4 - do right-edge drag to show the spread and swipe up filemanager to close
> > it.
> >
> > expected outcome:
> > You see in gdb that app exits nicely
> >
> > Actual outcome:
> > You get: "Program received signal SIGSEGV, Segmentation fault."
> >
> > Comments:
> > Without this branch I get "Program received signal SIGTERM, Terminated."
> > instead.
> >
> > It might not be our fault be we should at least know why it's crashing.
>
> This can be dealt with by another branch. the problem occurs in trunk as well.
> tested with messaging-app (crash), dialer-app (crash), camera-app (no crash)

Have you this branch prepared? We can't land this change if all apps will now crash on shutdown instead of being killed. It will cause apport to spin up for every app closed!

lp:~nick-dedekind/qtmir/polite-close updated on 2015-09-25
356. By Nick Dedekind on 2015-09-24

merged with trunk

357. By Nick Dedekind on 2015-09-25

version bump

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

Nick found that this bug is causing the crash in clients when trying to shut down cleanly:
https://bugs.launchpad.net/platform-api/+bug/1351109

Nick Dedekind (nick-dedekind) wrote :
lp:~nick-dedekind/qtmir/polite-close updated on 2015-09-29
358. By Nick Dedekind on 2015-09-29

updated comment

359. By Nick Dedekind on 2015-09-29

merged with trunk

Daniel d'Andrada (dandrader) wrote :

I confirm that those branches do fix the crash.

Now the only issue left is that mysterious test failure...

Daniel d'Andrada (dandrader) wrote :

Well, Saviq and CI have the test passing fine as well.

review: Approve
Gerry Boland (gerboland) wrote :

Please strip tags! It's a qtmir problem now to

PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:359
http://jenkins.qa.ubuntu.com/job/qtmir-ci/452/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-amd64-ci/148/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/148
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-armhf-ci/148/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-vivid-i386-ci/30
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-amd64-ci/185
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/qtmir-wily-armhf-ci/185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qtmir-wily-i386-ci/30

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/qtmir-ci/452/rebuild

review: Needs Fixing (continuous-integration)
Gerry Boland (gerboland) wrote :

Please merge qtmir trunk

lp:~nick-dedekind/qtmir/polite-close updated on 2015-11-09
360. By Nick Dedekind on 2015-11-02

merged with trunk

361. By Nick Dedekind on 2015-11-02

removed unnecessary cmake includes

362. By Nick Dedekind on 2015-11-09

merge with trunk

Nick Dedekind (nick-dedekind) wrote :

Fixe conflicts.

PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
lp:~nick-dedekind/qtmir/polite-close updated on 2015-12-07
363. By Nick Dedekind on 2015-12-07

merged with trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2015-12-01 14:14:00 +0000
3+++ debian/changelog 2015-12-07 10:51:38 +0000
4@@ -1,3 +1,10 @@
5+qtmir (0.4.7) UNRELEASED; urgency=medium
6+
7+ * [ Nick Dedekind ]
8+ * Request app closing rather than killing.
9+
10+ -- Nick Dedekind <nick.dedekind@canonical.com> Fri, 25 Sep 2015 09:33:39 +0100
11+
12 qtmir (0.4.6+16.04.20151125-0ubuntu2) xenial; urgency=medium
13
14 * Rebuild against Qt 5.5.1.
15
16=== modified file 'src/modules/Unity/Application/application.cpp'
17--- src/modules/Unity/Application/application.cpp 2015-10-01 17:20:40 +0000
18+++ src/modules/Unity/Application/application.cpp 2015-12-07 10:51:38 +0000
19@@ -45,13 +45,14 @@
20 , m_sharedWakelock(sharedWakelock)
21 , m_desktopData(desktopFileReader)
22 , m_pid(0)
23- , m_stage((m_desktopData->stageHint() == "SideStage") ? Application::SideStage : Application::MainStage)
24+ , m_stage((desktopFileReader->stageHint() == "SideStage") ? Application::SideStage : Application::MainStage)
25 , m_state(InternalState::Starting)
26 , m_focused(false)
27 , m_arguments(arguments)
28 , m_session(nullptr)
29 , m_requestedState(RequestedRunning)
30 , m_processState(ProcessUnknown)
31+ , m_closeTimer(0)
32 {
33 qCDebug(QTMIR_APPLICATIONS) << "Application::Application - appId=" << desktopFileReader->appId();
34
35@@ -394,25 +395,22 @@
36 return m_pid;
37 }
38
39-void Application::setPid(pid_t pid)
40-{
41- m_pid = pid;
42-}
43-
44 void Application::close()
45 {
46 qCDebug(QTMIR_APPLICATIONS) << "Application::close - appId=" << appId();
47
48 switch (m_state) {
49 case InternalState::Starting:
50+ stop();
51+ break;
52 case InternalState::Running:
53- setInternalState(InternalState::Closing);
54+ doClose();
55 break;
56 case InternalState::SuspendingWaitSession:
57 case InternalState::SuspendingWaitProcess:
58 case InternalState::Suspended:
59 setRequestedState(RequestedRunning);
60- setInternalState(InternalState::Closing);
61+ doClose();
62 break;
63 case InternalState::Closing:
64 // already on the way
65@@ -422,6 +420,22 @@
66 // too late
67 break;
68 }
69+
70+}
71+
72+void Application::doClose()
73+{
74+ Q_ASSERT(m_closeTimer == 0);
75+ Q_ASSERT(m_session != nullptr);
76+
77+ m_session->close();
78+ m_closeTimer = startTimer(3000);
79+ setInternalState(InternalState::Closing);
80+}
81+
82+void Application::setPid(pid_t pid)
83+{
84+ m_pid = pid;
85 }
86
87 void Application::setArguments(const QStringList arguments)
88@@ -603,6 +617,8 @@
89
90 void Application::suspend()
91 {
92+ qCDebug(QTMIR_APPLICATIONS) << "Application::suspend - appId=" << appId();
93+
94 Q_ASSERT(m_state == InternalState::Running);
95 Q_ASSERT(m_session != nullptr);
96
97@@ -612,6 +628,8 @@
98
99 void Application::resume()
100 {
101+ qCDebug(QTMIR_APPLICATIONS) << "Application::resume - appId=" << appId();
102+
103 if (m_state == InternalState::Suspended) {
104 setInternalState(InternalState::Running);
105 Q_EMIT resumeProcessRequested();
106@@ -634,6 +652,21 @@
107 Q_EMIT startProcessRequested();
108 }
109
110+void Application::stop()
111+{
112+ qCDebug(QTMIR_APPLICATIONS) << "Application::stop - appId=" << appId();
113+
114+ Q_EMIT stopProcessRequested();
115+}
116+
117+void Application::timerEvent(QTimerEvent *event)
118+{
119+ if (event->timerId() == m_closeTimer) {
120+ m_closeTimer = 0;
121+ stop();
122+ }
123+}
124+
125 bool Application::isTouchApp() const
126 {
127 return m_desktopData->isTouchApp();
128@@ -704,7 +737,7 @@
129 * 3. application is managed by upstart and is in foreground (i.e. has
130 * Running state), if Mir reports the application disconnects, it
131 * either crashed or stopped itself.
132- * 4. We're expecting the application to stop after a close request
133+ * 4. We're expecting the application to stop after a close request
134 */
135 setInternalState(InternalState::Stopped);
136 } else {
137
138=== modified file 'src/modules/Unity/Application/application.h'
139--- src/modules/Unity/Application/application.h 2015-11-17 14:16:22 +0000
140+++ src/modules/Unity/Application/application.h 2015-12-07 10:51:38 +0000
141@@ -137,6 +137,7 @@
142 void sessionChanged(SessionInterface *session);
143
144 void startProcessRequested();
145+ void stopProcessRequested();
146 void suspendProcessRequested();
147 void resumeProcessRequested();
148 void stopped();
149@@ -146,6 +147,9 @@
150
151 void respawn();
152
153+protected:
154+ void timerEvent(QTimerEvent *event);
155+
156 private:
157
158 QString longAppId() const;
159@@ -158,11 +162,13 @@
160 void wipeQMLCache();
161 void suspend();
162 void resume();
163+ void stop();
164 QColor colorFromString(const QString &colorString, const char *colorName) const;
165 static const char* internalStateToStr(InternalState state);
166 void applyRequestedState();
167 void applyRequestedRunning();
168 void applyRequestedSuspended();
169+ void doClose();
170
171 QSharedPointer<SharedWakelock> m_sharedWakelock;
172 DesktopFileReader* m_desktopData;
173@@ -178,6 +184,7 @@
174 SessionInterface *m_session;
175 RequestedState m_requestedState;
176 ProcessState m_processState;
177+ int m_closeTimer;
178
179 friend class ApplicationManager;
180 friend class SessionManager;
181
182=== modified file 'src/modules/Unity/Application/application_manager.cpp'
183--- src/modules/Unity/Application/application_manager.cpp 2015-11-17 14:16:22 +0000
184+++ src/modules/Unity/Application/application_manager.cpp 2015-12-07 10:51:38 +0000
185@@ -353,6 +353,23 @@
186 return nullptr;
187 }
188
189+ if (m_queuedStartApplications.contains(inputAppId)) {
190+ qWarning() << "ApplicationManager::startApplication - application appId=" << appId << " is queued to start";
191+ return nullptr;
192+ } else {
193+ application = findClosingApplication(inputAppId);
194+ if (application) {
195+ m_queuedStartApplications.append(inputAppId);
196+ qWarning() << "ApplicationManager::startApplication - application appId=" << appId << " is closing. Queuing start";
197+ connect(application, &QObject::destroyed, this, [this, application, inputAppId, flags, arguments]() {
198+ m_queuedStartApplications.removeAll(inputAppId);
199+ // start the app.
200+ startApplication(inputAppId, flags, arguments);
201+ }, Qt::QueuedConnection); // Queued so that we finish the app removal before starting again.
202+ return nullptr;
203+ }
204+ }
205+
206 if (!m_taskController->start(appId, arguments)) {
207 qWarning() << "Upstart failed to start application with appId" << appId;
208 return nullptr;
209@@ -438,17 +455,12 @@
210 application->close();
211 remove(application);
212
213- bool result = m_taskController->stop(application->longAppId());
214-
215- if (!result && application->pid() > 0) {
216- qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
217- << "Sending SIGTERM to process:" << application->pid();
218- kill(application->pid(), SIGTERM);
219- result = true;
220- }
221-
222- delete application;
223- return result;
224+ connect(application, &QObject::destroyed, this, [this, application](QObject*) {
225+ m_closingApplications.removeAll(application);
226+ });
227+ m_closingApplications.append(application);
228+ application->close();
229+ return true;
230 }
231
232 void ApplicationManager::onProcessFailed(const QString &appId, const bool duringStartup)
233@@ -473,7 +485,11 @@
234 {
235 tracepoint(qtmir, onProcessStopped);
236 qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::onProcessStopped - appId=" << appId;
237+
238 Application *application = findApplication(appId);
239+ if (!application) {
240+ application = findClosingApplication(appId);
241+ }
242
243 if (!application) {
244 qDebug() << "ApplicationManager::onProcessStopped reports stop of appId=" << appId
245@@ -729,6 +745,15 @@
246 this, [=]() { m_taskController->start(appId, arguments); },
247 Qt::QueuedConnection);
248
249+ connect(application, &Application::stopProcessRequested, this, [=]() {
250+ if (!m_taskController->stop(application->longAppId()) && application->pid() > 0) {
251+ qWarning() << "FAILED to ask Upstart to stop application with appId" << appId
252+ << "Sending SIGTERM to process:" << appId;
253+ kill(application->pid(), SIGTERM);
254+ application->setProcessState(Application::ProcessStopped);
255+ }
256+ });
257+
258 connect(application, &Application::suspendProcessRequested, this, [=]() { m_taskController->suspend(longAppId); } );
259 connect(application, &Application::resumeProcessRequested, this, [=]() { m_taskController->resume(longAppId); } );
260
261@@ -753,7 +778,10 @@
262 Q_ASSERT(application != nullptr);
263 qCDebug(QTMIR_APPLICATIONS) << "ApplicationManager::remove - appId=" << application->appId();
264
265- application->disconnect(this);
266+ disconnect(application, &Application::fullscreenChanged, this, 0);
267+ disconnect(application, &Application::focusedChanged, this, 0);
268+ disconnect(application, &Application::stateChanged, this, 0);
269+ disconnect(application, &Application::stageChanged, this, 0);
270
271 int i = m_applications.indexOf(application);
272 if (i != -1) {
273@@ -813,4 +841,16 @@
274 return result;
275 }
276
277+Application *ApplicationManager::findClosingApplication(const QString &inputAppId) const
278+{
279+ const QString appId = toShortAppIdIfPossible(inputAppId);
280+
281+ for (Application *app : m_closingApplications) {
282+ if (app->appId() == appId) {
283+ return app;
284+ }
285+ }
286+ return nullptr;
287+}
288+
289 } // namespace qtmir
290
291=== modified file 'src/modules/Unity/Application/application_manager.h'
292--- src/modules/Unity/Application/application_manager.h 2015-11-17 14:16:22 +0000
293+++ src/modules/Unity/Application/application_manager.h 2015-12-07 10:51:38 +0000
294@@ -149,6 +149,7 @@
295 QString toString() const;
296
297 Application* findApplicationWithPromptSession(const mir::scene::PromptSession* promptSession);
298+ Application *findClosingApplication(const QString &inputAppId) const;
299
300 QSharedPointer<MirServer> m_mirServer;
301
302@@ -160,6 +161,8 @@
303 QSharedPointer<ProcInfo> m_procInfo;
304 QSharedPointer<SharedWakelock> m_sharedWakelock;
305 QSharedPointer<SettingsInterface> m_settings;
306+ QList<Application*> m_closingApplications;
307+ QList<QString> m_queuedStartApplications;
308 static ApplicationManager* the_application_manager;
309
310 friend class Application;
311
312=== modified file 'src/modules/Unity/Application/mirsurface.cpp'
313--- src/modules/Unity/Application/mirsurface.cpp 2015-11-25 15:38:39 +0000
314+++ src/modules/Unity/Application/mirsurface.cpp 2015-12-07 10:51:38 +0000
315@@ -420,6 +420,13 @@
316 }
317 }
318
319+void MirSurface::close()
320+{
321+ if (m_surface) {
322+ m_surface->request_client_surface_close();
323+ }
324+}
325+
326 void MirSurface::resize(int width, int height)
327 {
328 int mirWidth = m_surface->size().width.as_int();
329
330=== modified file 'src/modules/Unity/Application/mirsurface.h'
331--- src/modules/Unity/Application/mirsurface.h 2015-11-25 15:38:39 +0000
332+++ src/modules/Unity/Application/mirsurface.h 2015-12-07 10:51:38 +0000
333@@ -99,6 +99,8 @@
334
335 void setFocus(bool focus) override;
336
337+ void close() override;
338+
339 void mousePressEvent(QMouseEvent *event) override;
340 void mouseMoveEvent(QMouseEvent *event) override;
341 void mouseReleaseEvent(QMouseEvent *event) override;
342
343=== modified file 'src/modules/Unity/Application/mirsurfaceinterface.h'
344--- src/modules/Unity/Application/mirsurfaceinterface.h 2015-11-25 15:38:39 +0000
345+++ src/modules/Unity/Application/mirsurfaceinterface.h 2015-12-07 10:51:38 +0000
346@@ -64,6 +64,8 @@
347
348 virtual void setFocus(bool focus) = 0;
349
350+ virtual void close() = 0;
351+
352 virtual void mousePressEvent(QMouseEvent *event) = 0;
353 virtual void mouseMoveEvent(QMouseEvent *event) = 0;
354 virtual void mouseReleaseEvent(QMouseEvent *event) = 0;
355
356=== modified file 'src/modules/Unity/Application/session.cpp'
357--- src/modules/Unity/Application/session.cpp 2015-08-31 09:51:28 +0000
358+++ src/modules/Unity/Application/session.cpp 2015-12-07 10:51:38 +0000
359@@ -40,6 +40,28 @@
360 namespace qtmir
361 {
362
363+namespace {
364+
365+const char *sessionStateToString(SessionInterface::State state)
366+{
367+ switch (state) {
368+ case SessionInterface::Starting:
369+ return "starting";
370+ case SessionInterface::Running:
371+ return "running";
372+ case SessionInterface::Suspending:
373+ return "suspending";
374+ case SessionInterface::Suspended:
375+ return "suspended";
376+ case SessionInterface::Stopped:
377+ return "stopped";
378+ default:
379+ return "???";
380+ }
381+}
382+
383+}
384+
385 Session::Session(const std::shared_ptr<ms::Session>& session,
386 const std::shared_ptr<ms::PromptSessionManager>& promptSessionManager,
387 QObject *parent)
388@@ -142,6 +164,9 @@
389
390 void Session::setState(State state) {
391 if (state != m_state) {
392+ qCDebug(QTMIR_SESSIONS) << "Session::setState - session=" << name()
393+ << "state=" << sessionStateToString(state);
394+
395 m_state = state;
396 Q_EMIT stateChanged(m_state);
397 }
398@@ -232,7 +257,7 @@
399
400 void Session::suspend()
401 {
402- qCDebug(QTMIR_SESSIONS) << "Session::suspend - session=" << this << "state=" << applicationStateToStr(m_state);
403+ qCDebug(QTMIR_SESSIONS) << "Session::suspend - session=" << this << "state=" << sessionStateToString(m_state);
404 if (m_state == Running) {
405 session()->set_lifecycle_state(mir_lifecycle_state_will_suspend);
406 m_suspendTimer->start(1500);
407@@ -251,7 +276,7 @@
408
409 void Session::resume()
410 {
411- qCDebug(QTMIR_SESSIONS) << "Session::resume - session=" << this << "state=" << applicationStateToStr(m_state);
412+ qCDebug(QTMIR_SESSIONS) << "Session::resume - session=" << this << "state=" << sessionStateToString(m_state);
413
414 if (m_state == Suspending || m_state == Suspended) {
415 doResume();
416@@ -281,9 +306,20 @@
417 setState(Running);
418 }
419
420+void Session::close()
421+{
422+ qCDebug(QTMIR_SESSIONS) << "Session::close - " << name() << m_surface;
423+ if (m_surface) {
424+ m_surface->close();
425+ }
426+}
427+
428 void Session::stop()
429 {
430+ qCDebug(QTMIR_SESSIONS) << "Session::stop - " << name();
431+
432 if (m_state != Stopped) {
433+
434 stopPromptSessions();
435 if (m_suspendTimer->isActive())
436 m_suspendTimer->stop();
437@@ -304,6 +340,8 @@
438 void Session::setLive(const bool live)
439 {
440 if (m_live != live) {
441+ qCDebug(QTMIR_SESSIONS) << "Session::setLive - " << name() << "live=" << live;
442+
443 m_live = live;
444 Q_EMIT liveChanged(m_live);
445 if (!live) {
446
447=== modified file 'src/modules/Unity/Application/session.h'
448--- src/modules/Unity/Application/session.h 2015-08-31 09:51:28 +0000
449+++ src/modules/Unity/Application/session.h 2015-12-07 10:51:38 +0000
450@@ -63,6 +63,7 @@
451
452 void suspend() override;
453 void resume() override;
454+ void close() override;
455 void stop() override;
456
457 void addChildSession(SessionInterface* session) override;
458
459=== modified file 'src/modules/Unity/Application/session_interface.h'
460--- src/modules/Unity/Application/session_interface.h 2015-08-31 09:51:28 +0000
461+++ src/modules/Unity/Application/session_interface.h 2015-12-07 10:51:38 +0000
462@@ -80,6 +80,7 @@
463 virtual void setApplication(unity::shell::application::ApplicationInfoInterface* item) = 0;
464 virtual void suspend() = 0;
465 virtual void resume() = 0;
466+ virtual void close() = 0;
467 virtual void stop() = 0;
468
469 // For SessionManager use
470
471=== modified file 'tests/modules/Application/CMakeLists.txt'
472--- tests/modules/Application/CMakeLists.txt 2015-08-19 20:17:56 +0000
473+++ tests/modules/Application/CMakeLists.txt 2015-12-07 10:51:38 +0000
474@@ -11,6 +11,11 @@
475 ${CMAKE_SOURCE_DIR}/src/modules
476 ${CMAKE_SOURCE_DIR}/tests/modules/common
477 ${MIRSERVER_INCLUDE_DIRS}
478+
479+ ${Qt5Core_INCLUDE_DIRS}
480+ ${Qt5GUI_INCLUDE_DIRS}
481+ ${Qt5Quick_INCLUDE_DIRS}
482+ ${Qt5DBus_INCLUDE_DIRS}
483 )
484
485 add_executable(application_test ${APPLICATION_TEST_SOURCES})
486
487=== modified file 'tests/modules/ApplicationManager/application_manager_test.cpp'
488--- tests/modules/ApplicationManager/application_manager_test.cpp 2015-11-17 14:16:22 +0000
489+++ tests/modules/ApplicationManager/application_manager_test.cpp 2015-12-07 10:51:38 +0000
490@@ -1870,3 +1870,158 @@
491 EXPECT_EQ(0, applicationManager.count());
492 EXPECT_TRUE(dir.exists());
493 }
494+
495+/*
496+ * Test that there is an attempt at polite exiting of the app by requesting closure of the surface.
497+ */
498+TEST_F(ApplicationManagerTests,requestSurfaceCloseOnStop)
499+{
500+ using namespace ::testing;
501+
502+ const QString appId("testAppId");
503+ quint64 procId = 5551;
504+ Application* app = startApplication(procId, appId);
505+ std::shared_ptr<mir::scene::Session> session = app->session()->session();
506+
507+ FakeMirSurface *surface = new FakeMirSurface;
508+ onSessionCreatedSurface(session.get(), surface);
509+ surface->drawFirstFrame();
510+
511+ QSignalSpy spy(surface, SIGNAL(closeRequested()));
512+
513+ // Stop app
514+ applicationManager.stopApplication(appId);
515+
516+ EXPECT_EQ(1, spy.count());
517+}
518+
519+
520+// * Test that if there is no surface available to the app when it is stopped, that it is forced to close.
521+TEST_F(ApplicationManagerTests,forceAppDeleteWhenRemovedWithMissingSurface)
522+{
523+ using namespace ::testing;
524+
525+ int argc = 0;
526+ char* argv[0];
527+ QCoreApplication qtApp(argc, argv); // app for deleteLater event
528+
529+ const QString appId("testAppId");
530+ quint64 procId = 5551;
531+ Application* app = startApplication(procId, appId);
532+
533+ QSignalSpy spy(app, SIGNAL(destroyed(QObject*)));
534+ QObject::connect(app, &QObject::destroyed, app, [&qtApp](){ qtApp.quit(); });
535+
536+ // Stop app
537+ applicationManager.stopApplication(appId);
538+ qtApp.exec();
539+ EXPECT_EQ(1, spy.count());
540+}
541+
542+/*
543+ * Test that if an application is started while it is still attempting to close, it is queued to start again.
544+ */
545+TEST_F(ApplicationManagerTests,applicationStartQueuedOnStartStopStart)
546+{
547+ using namespace ::testing;
548+
549+ int argc = 0;
550+ char* argv[0];
551+ QCoreApplication qtApp(argc, argv); // app for deleteLater event
552+
553+ const QString appId("testAppId");
554+ quint64 procId = 5551;
555+ Application* app = startApplication(procId, appId);
556+ std::shared_ptr<mir::scene::Session> session = app->session()->session();
557+
558+ FakeMirSurface *surface = new FakeMirSurface;
559+ onSessionCreatedSurface(session.get(), surface);
560+ surface->drawFirstFrame();
561+
562+ EXPECT_EQ(Application::InternalState::Running, app->internalState());
563+
564+ // Stop app
565+ applicationManager.stopApplication(appId);
566+
567+ EXPECT_EQ(Application::InternalState::Closing, app->internalState());
568+
569+ // // Set up Mocks & signal watcher
570+ auto mockDesktopFileReader = new NiceMock<MockDesktopFileReader>(appId, QFileInfo());
571+ ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
572+ ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
573+ ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
574+
575+ EXPECT_EQ(nullptr, applicationManager.startApplication(appId, ApplicationManager::NoFlag));
576+
577+ QSignalSpy spy(&applicationManager, SIGNAL(applicationAdded(const QString&)));
578+ applicationManager.onProcessStopped(appId);
579+
580+ QObject::connect(&applicationManager, &ApplicationManager::applicationAdded,
581+ &applicationManager, [&qtApp, appId](const QString& startedApp) {
582+ if (startedApp == appId) {
583+ qtApp.quit();
584+ }
585+ });
586+
587+ qtApp.exec();
588+ EXPECT_EQ(1, spy.count());
589+}
590+
591+/*
592+ * Test that there is an attempt at polite exiting of the app by requesting closure of the surface.
593+ */
594+TEST_F(ApplicationManagerTests,suspendedApplicationResumesClosesAndDeletes)
595+{
596+ using namespace ::testing;
597+
598+ const QString appId("testAppId");
599+ quint64 procId = 5551;
600+ Application* app = startApplication(procId, appId);
601+ std::shared_ptr<mir::scene::Session> session = app->session()->session();
602+
603+ FakeMirSurface *surface = new FakeMirSurface;
604+ onSessionCreatedSurface(session.get(), surface);
605+ surface->drawFirstFrame();
606+ EXPECT_EQ(Application::InternalState::Running, app->internalState());
607+ EXPECT_EQ(SessionInterface::Running, app->session()->state());
608+
609+ // Suspend the application.
610+ suspend(app);
611+ EXPECT_EQ(Application::InternalState::Suspended, app->internalState());
612+
613+ // Stop app
614+ applicationManager.stopApplication(appId);
615+ EXPECT_EQ(Application::InternalState::Closing, app->internalState());
616+ EXPECT_EQ(SessionInterface::Running, app->session()->state());
617+}
618+
619+/*
620+ * Test that a application which fails to close will eventually be forceable closed.
621+ */
622+TEST_F(ApplicationManagerTests,failedApplicationCloseEventualyDeletesApplication)
623+{
624+ using namespace ::testing;
625+
626+ int argc = 0;
627+ char* argv[0];
628+ QCoreApplication qtApp(argc, argv); // app for deleteLater event
629+
630+ const QString appId("testAppId");
631+ quint64 procId = 5551;
632+ Application* app = startApplication(procId, appId);
633+ std::shared_ptr<mir::scene::Session> session = app->session()->session();
634+
635+ FakeMirSurface *surface = new FakeMirSurface;
636+ onSessionCreatedSurface(session.get(), surface);
637+ surface->drawFirstFrame();
638+
639+ EXPECT_EQ(Application::InternalState::Running, app->internalState());
640+
641+ QSignalSpy spy(app, SIGNAL(destroyed(QObject*)));
642+ QObject::connect(app, &QObject::destroyed, app, [&qtApp](){ qtApp.quit(); });
643+
644+ // Stop app
645+ applicationManager.stopApplication(appId);
646+ qtApp.exec();
647+ EXPECT_EQ(1, spy.count());
648+}
649
650=== modified file 'tests/modules/common/fake_mirsurface.h'
651--- tests/modules/common/fake_mirsurface.h 2015-11-25 15:38:39 +0000
652+++ tests/modules/common/fake_mirsurface.h 2015-12-07 10:51:38 +0000
653@@ -174,6 +174,13 @@
654
655 QCursor cursor() const override { return QCursor(); }
656
657+ void close() override {
658+ Q_EMIT closeRequested();
659+ }
660+
661+Q_SIGNALS:
662+ void closeRequested();
663+
664 public Q_SLOTS:
665 void onCompositorSwappedBuffers() override {}
666
667
668=== modified file 'tests/modules/common/fake_session.h'
669--- tests/modules/common/fake_session.h 2015-10-20 09:57:17 +0000
670+++ tests/modules/common/fake_session.h 2015-12-07 10:51:38 +0000
671@@ -15,6 +15,7 @@
672 */
673
674 #include <Unity/Application/session_interface.h>
675+#include <QDebug>
676
677 #ifndef QTMIR_FAKE_SESSION_H
678 #define QTMIR_FAKE_SESSION_H
679@@ -73,6 +74,9 @@
680 setState(Stopped);
681 }
682
683+ void close() override {
684+ }
685+
686 // For SessionManager use
687
688 void addChildSession(SessionInterface*) override {}
689
690=== modified file 'tests/modules/common/mock_session.h'
691--- tests/modules/common/mock_session.h 2015-08-31 09:51:28 +0000
692+++ tests/modules/common/mock_session.h 2015-12-07 10:51:38 +0000
693@@ -49,6 +49,7 @@
694
695 MOCK_METHOD0(suspend, void());
696 MOCK_METHOD0(resume, void());
697+ MOCK_METHOD0(close, void());
698 MOCK_METHOD0(stop, void());
699
700 MOCK_METHOD1(addChildSession, void(SessionInterface* session));
701
702=== modified file 'tests/modules/common/qtmir_test.h'
703--- tests/modules/common/qtmir_test.h 2015-11-19 12:56:02 +0000
704+++ tests/modules/common/qtmir_test.h 2015-12-07 10:51:38 +0000
705@@ -139,7 +139,9 @@
706 ON_CALL(*mockDesktopFileReader, loaded()).WillByDefault(Return(true));
707 ON_CALL(*mockDesktopFileReader, appId()).WillByDefault(Return(appId));
708
709- ON_CALL(desktopFileReaderFactory, createInstance(appId, _)).WillByDefault(Return(mockDesktopFileReader));
710+ EXPECT_CALL(desktopFileReaderFactory, createInstance(appId, _))
711+ .Times(1)
712+ .WillOnce(Return(mockDesktopFileReader));
713
714 EXPECT_CALL(appController, startApplicationWithAppIdAndArgs(appId, _))
715 .Times(1)
716@@ -153,7 +155,11 @@
717 EXPECT_EQ(authed, true);
718
719 auto appSession = std::make_shared<mir::scene::MockSession>(appId.toStdString(), procId);
720+ applicationManager.onSessionStarting(appSession);
721 sessionManager.onSessionStarting(appSession);
722+
723+ Mock::VerifyAndClearExpectations(&appController);
724+ Mock::VerifyAndClearExpectations(&desktopFileReaderFactory);
725 return application;
726 }
727

Subscribers

People subscribed via source and target branches