on xenial help sign in
Make it less likely to issue duplicate Window IDs

Some tests will issue the null window and the first window at almost the same time, causing their window IDs to be the same prior to this change. That was fun to debug. I'm not going to pretend that this is absolutely foolproof, it's possible we still end up with duplicate IDs, but the risk should be quite small. I had trouble testing this with the rest of the PhoneStage tests where it is now customary to put the TLWM tests, there is too much troublesome behavior when changing maxWindowId with an in-use model. Instead, the new tst_QMLTopLevelWindowModel gives a case similar to PhoneStage but with less touch input and more raw window actions.

commitf85133bdf0a1269def8e3b17dce10b7108b4bd39 authorDalton Durst <dalton@ubports.com> dateTue 24 Dec 2019, 10:43 (7 years ago) parent7fb1979
6 files changed+270−55
plugins/WindowManager/TopLevelWindowModel.cpp+43−22 plugins/WindowManager/TopLevelWindowModel.h+22−6 tests/plugins/WindowManager/tst_TopLevelWindowModel.cpp+2 tests/qmltests/CMakeLists.txt+1 tests/qmltests/Stage/tst_PhoneStage.qml+1−27 tests/qmltests/Stage/tst_QMLTopLevelWindowModel.qml+201
plugins/WindowManager/TopLevelWindowModel.cpp+43−22
@@ -1,5 +1,6 @@
11/*
22 * Copyright (C) 2016-2017 Canonical, Ltd.
3+ * Copyright 2019 UBports Foundation
34 *
45 * This program is free software: you can redistribute it and/or modify it under
56 * the terms of the GNU Lesser General Public License version 3, as published by
@@ -38,7 +39,8 @@ Q_LOGGING_CATEGORY(TOPLEVELWINDOWMODEL, "toplevelwindowmodel", QtInfoMsg)
3839namespace unityapi = unity::shell::application;
3940
4041TopLevelWindowModel::TopLevelWindowModel()
41− : m_nullWindow(createWindow(nullptr)),
42+ : m_nullWindow(createNullWindow()),
43+ m_maxWindowId(1000000),
4244 m_surfaceManagerBusy(false)
4345{
4446 connect(m_nullWindow, &Window::focusedChanged, this, [this] {
@@ -350,6 +352,16 @@ void TopLevelWindowModel::onSurfaceDestroyed(unityapi::MirSurfaceInterface *surf
350352Window *TopLevelWindowModel::createWindow(unityapi::MirSurfaceInterface *surface)
351353{
352354 int id = generateId();
355+ return createWindowWithId(surface, id);
356+}
357+
358+Window *TopLevelWindowModel::createNullWindow()
359+{
360+ return createWindowWithId(nullptr, 0);
361+}
362+
363+Window *TopLevelWindowModel::createWindowWithId(unityapi::MirSurfaceInterface *surface, int id)
364+{
353365 Window *qmlWindow = new Window(id, this);
354366 connectWindow(qmlWindow);
355367 if (surface) {
@@ -514,34 +526,29 @@ int TopLevelWindowModel::findIndexOf(const unityapi::MirSurfaceInterface *surfac
514526 return -1;
515527}
516528
529+// Finds a unique integer identifier not greater than maxWindowId.
517530int TopLevelWindowModel::generateId()
518531{
519− int id = m_nextId;
520− m_nextId = nextFreeId(nextId(id), id);
521− return id;
522−}
532+ int candidateId = m_nextId.fetchAndAddAcquire(1);
523533
524−int TopLevelWindowModel::nextId(int id) const
525−{
526− if (id == m_maxId) {
527− return id = 1;
528− } else {
529− return id + 1;
530− }
531−}
534+ // If the user opens two windows at the exact same time after opening
535+ // maxWindowId windows, it is potentially possible that we issue the same
536+ // ID for both. This is a risk we can accept.
537+ if (candidateId > getMaxWindowId()) {
538+ int originalCandidateId = candidateId;
539+ candidateId = 1;
532540
533−int TopLevelWindowModel::nextFreeId(int candidateId, const int latestId)
534−{
535− int firstCandidateId = candidateId;
541+ while (indexForId(candidateId) != -1) {
542+ candidateId += 1;
536543
537− while (indexForId(candidateId) != -1 || candidateId == latestId) {
538− candidateId = nextId(candidateId);
539−
540− if (candidateId == firstCandidateId) {
541− qFatal("TopLevelWindowModel: run out of window ids.");
544+ if (candidateId == originalCandidateId) {
545+ // The user has maxWindowId windows open somehow.
546+ qFatal("TopLevelWindowModel: Ran out of window IDs!");
547+ }
542548 }
543− }
544549
550+ m_nextId.fetchAndStoreAcquire(candidateId + 1);
551+ }
545552 return candidateId;
546553}
547554
@@ -788,6 +795,20 @@ void TopLevelWindowModel::setRootFocus(bool focus)
788795 }
789796}
790797
798+void TopLevelWindowModel::setMaxWindowId(int newMaxWindowId)
799+{
800+ DEBUG_MSG << "(" << newMaxWindowId << ")";
801+ if (m_maxWindowId != newMaxWindowId) {
802+ m_maxWindowId = newMaxWindowId;
803+ Q_EMIT maxWindowIdChanged();
804+ }
805+}
806+
807+int TopLevelWindowModel::getMaxWindowId()
808+{
809+ return m_maxWindowId;
810+}
811+
791812// Pending Activation will block refocus of previous focused window
792813// this is needed since surface activation with miral is async,
793814// and activation of placeholder is sync. This causes a race condition
plugins/WindowManager/TopLevelWindowModel.h+22−6
@@ -1,5 +1,6 @@
11/*
22 * Copyright (C) 2016-2017 Canonical, Ltd.
3+ * Copyright 2019 UBports Foundation
34 *
45 * This program is free software: you can redistribute it and/or modify it under
56 * the terms of the GNU Lesser General Public License version 3, as published by
@@ -18,6 +19,7 @@
1819#define TOPLEVELWINDOWMODEL_H
1920
2021#include <QAbstractListModel>
22+#include <QAtomicInteger>
2123#include <QLoggingCategory>
2224
2325#include "WindowManagerGlobal.h"
@@ -86,7 +88,15 @@ class WINDOWMANAGERQML_EXPORT TopLevelWindowModel : public QAbstractListModel
8688 The id to be used on the next entry created
8789 Useful for tests
8890 */
89− Q_PROPERTY(int nextId READ nextId NOTIFY nextIdChanged)
91+ Q_PROPERTY(int nextId READ nextId)
92+
93+ /**
94+ The maximum window ID that may be issued by the model.
95+ Will only leave the model in a consistent state if changed before
96+ any windows are added.
97+ Don't change this unless you are the tests or want to break everything.
98+ */
99+ Q_PROPERTY(int maxWindowId READ getMaxWindowId WRITE setMaxWindowId)
90100
91101
92102 /**
@@ -143,7 +153,7 @@ public:
143153 unity::shell::application::SurfaceManagerInterface *surfaceManager() const { return m_surfaceManager; }
144154 void setSurfaceManager(unity::shell::application::SurfaceManagerInterface*);
145155
146− int nextId() const { return m_nextId; }
156+ int nextId() const { return m_nextId.load(); }
147157
148158public:
149159 /**
@@ -198,6 +208,9 @@ public:
198208 void setRootFocus(bool focus);
199209 bool rootFocus();
200210
211+ void setMaxWindowId(int newMaxWindowId);
212+ int getMaxWindowId();
213+
201214Q_SIGNALS:
202215 void countChanged();
203216 void inputMethodSurfaceChanged(unity::shell::application::MirSurfaceInterface* inputMethodSurface);
@@ -212,12 +225,12 @@ Q_SIGNALS:
212225 */
213226 void listChanged();
214227
215− void nextIdChanged();
216−
217228 void closedAllWindows();
218229
219230 void rootFocusChanged();
220231
232+ void maxWindowIdChanged();
233+
221234private Q_SLOTS:
222235 void onSurfaceCreated(unity::shell::application::MirSurfaceInterface *surface);
223236 void onSurfacesRaised(const QVector<unity::shell::application::MirSurfaceInterface*> &surfaces);
@@ -263,6 +276,8 @@ private:
263276 void activateTopMostWindowWithoutId(int forbiddenId);
264277
265278 Window *createWindow(unity::shell::application::MirSurfaceInterface *surface);
279+ Window *createWindowWithId(unity::shell::application::MirSurfaceInterface *surface, int id);
280+ Window *createNullWindow();
266281
267282 struct ModelEntry {
268283 ModelEntry() {}
@@ -281,11 +296,12 @@ private:
281296 Window* m_previousWindow{nullptr};
282297 bool m_pendingActivation;
283298
284− int m_nextId{1};
299+ QAtomicInteger<int> m_nextId{1};
300+
285301 // Just something big enough that we don't risk running out of unused id numbers.
286302 // Not sure if QML int type supports something close to std::numeric_limits<int>::max() and
287303 // there's no reason to try out its limits.
288− static const int m_maxId{1000000};
304+ int m_maxWindowId;
289305
290306 unity::shell::application::ApplicationManagerInterface* m_applicationManager{nullptr};
291307 unity::shell::application::SurfaceManagerInterface *m_surfaceManager{nullptr};
tests/plugins/WindowManager/tst_TopLevelWindowModel.cpp+2
@@ -14,6 +14,8 @@
1414 * along with this program. If not, see <http://www.gnu.org/licenses/>.
1515 */
1616
17+// There is a companion set of QMLTests for TLWM in tst_QMLTopLevelWindowModel.qml.
18+
1719#include <QtTest/QtTest>
1820#include <QSignalSpy>
1921
tests/qmltests/CMakeLists.txt+1
@@ -54,6 +54,7 @@ add_unity8_qmltest(Panel/Indicators MessageMenuItemFactory)
5454add_unity8_qmltest(Stage ApplicationWindow)
5555add_unity8_qmltest(Stage DesktopStage)
5656add_unity8_qmltest(Stage PhoneStage)
57+add_unity8_qmltest(Stage QMLTopLevelWindowModel)
5758add_unity8_qmltest(Stage SurfaceContainer)
5859add_unity8_qmltest(Stage TabletStage)
5960add_unity8_qmltest(Stage WindowResizeArea)
tests/qmltests/Stage/tst_PhoneStage.qml+1−27
@@ -1,5 +1,6 @@
11/*
22 * Copyright 2014-2016 Canonical Ltd.
3+ * Copyright 2019 UBports Foundation
34 *
45 * This program is free software; you can redistribute it and/or modify
56 * it under the terms of the GNU General Public License as published by
@@ -582,32 +583,5 @@ Item {
582583 compare(topLevelSurfaceList.idAt(0), webbrowserSurfaceId);
583584 compare(webbrowserApp.focused, true);
584585 }
585−
586− /*
587− Ensure that closing a surface while rootFocus is off focuses the
588− next available surface when rootFocus is given back.
589−
590− Regression test for https://github.com/ubports/unity8/issues/234
591−
592− This cannot be tested in tst_TopLevelWindowModel.cpp, the mocks for
593− it are not advanced enough.
594− */
595− function test_closeFocusedAppWithSpreadOpen()
596− {
597− var dashApp = ApplicationManager.findApplication("unity8-dash");
598− var webbrowserSurfaceId = topLevelSurfaceList.nextId;
599− var webbrowserApp = ApplicationManager.startApplication("morph-browser");
600− waitUntilAppSurfaceShowsUp(webbrowserSurfaceId);
601−
602− topLevelSurfaceList.rootFocus = false;
603−
604− performEdgeSwipeToShowAppSpread();
605−
606− swipeSurfaceUpwards(webbrowserSurfaceId);
607− tryCompareFunction(function() { return topLevelSurfaceList.indexForId(webbrowserSurfaceId); }, -1);
608−
609− topLevelSurfaceList.rootFocus = true;
610− compare(dashApp.focused, true);
611− }
612586 }
613587}
tests/qmltests/Stage/tst_QMLTopLevelWindowModel.qmlnew file+201
@@ -0,0 +1,201 @@
1+/*
2+ * Copyright 2014-2016 Canonical Ltd.
3+ * Copyright 2019 UBports Foundation
4+ *
5+ * This program is free software; you can redistribute it and/or modify
6+ * it under the terms of the GNU General Public License as published by
7+ * the Free Software Foundation; version 3.
8+ *
9+ * This program is distributed in the hope that it will be useful,
10+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
11+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+ * GNU General Public License for more details.
13+ *
14+ * You should have received a copy of the GNU General Public License
15+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
16+ */
17+
18+import QtQuick 2.4
19+import QtTest 1.0
20+import Unity.Test 0.1 as UT
21+import ".."
22+import "../../../qml/Components"
23+import "../../../qml/Stage"
24+import Ubuntu.Components 1.3
25+import Unity.Application 0.1
26+import WindowManager 1.0
27+
28+Item {
29+ id: root
30+ width: units.gu(70)
31+ height: units.gu(70)
32+
33+ property var greeter: { fullyShown: true }
34+
35+ SurfaceManager { id: sMgr }
36+ ApplicationMenuDataLoader {
37+ id: appMenuData
38+ surfaceManager: sMgr
39+ }
40+
41+ Loader {
42+ id: loader
43+ active: true;
44+
45+ sourceComponent: Stage {
46+ id: stage
47+ anchors { fill: parent; rightMargin: units.gu(30) }
48+ focus: true
49+ dragAreaWidth: units.gu(2)
50+ allowInteractivity: true
51+ shellOrientation: Qt.PortraitOrientation
52+ orientations: Orientations {}
53+ applicationManager: ApplicationManager
54+ mode: "staged"
55+ topLevelSurfaceList: TopLevelWindowModel {
56+ id: topLevelWindowModel
57+ applicationManager: ApplicationManager
58+ surfaceManager: sMgr
59+ }
60+ availableDesktopArea: availableDesktopAreaItem
61+ Item {
62+ id: availableDesktopAreaItem
63+ anchors.fill: parent
64+ }
65+ }
66+ }
67+
68+ Flickable {
69+ contentHeight: controlRect.height
70+
71+ anchors.top: root.top
72+ anchors.bottom: root.bottom
73+ anchors.right: root.right
74+ width: units.gu(30)
75+ Rectangle {
76+ id: controlRect
77+ anchors { left: parent.left; right: parent.right }
78+ height: childrenRect.height + units.gu(2)
79+ color: "darkGrey"
80+ Column {
81+ anchors { left: parent.left; right: parent.right; top: parent.top; margins: units.gu(1) }
82+ spacing: units.gu(1)
83+ Repeater {
84+ model: ApplicationManager.availableApplications
85+ ApplicationCheckBox { appId: modelData }
86+ }
87+ }
88+ }
89+ }
90+
91+ property alias stage: loader.item
92+ property var tlwm: loader.item.topLevelSurfaceList
93+
94+ UT.UnityTestCase {
95+ id: testCase
96+ name: "PhoneStage"
97+ when: windowShown
98+
99+ function init() {
100+ loader.active = true;
101+ tryCompare(loader, "status", Loader.Ready);
102+ verify(stage);
103+ verify(tlwm);
104+ waitForRendering(stage);
105+ }
106+
107+ function cleanup() {
108+ killApps();
109+ loader.active = false;
110+ }
111+
112+ function findAppWindowForSurfaceId(surfaceId) {
113+ var delegateObjectName = "appDelegate_" + surfaceId;
114+ var spreadDelegate = findChild(stage, delegateObjectName);
115+ if (!spreadDelegate) {
116+ console.warn("Failed to find " + delegateObjectName + " in stage");
117+ return null;
118+ }
119+ var appWindow = findChild(spreadDelegate, "appWindow");
120+ return appWindow;
121+ }
122+
123+ // Waits until ApplicationWindow has moved from showing a splash screen to displaying
124+ // the application surface.
125+ function waitUntilAppSurfaceShowsUp(surfaceId) {
126+ var appWindow = findAppWindowForSurfaceId(surfaceId);
127+ verify(appWindow);
128+ var appWindowStates = findInvisibleChild(appWindow, "applicationWindowStateGroup");
129+ verify(appWindowStates);
130+ tryCompare(appWindowStates, "state", "surface");
131+ }
132+
133+ function addApps(count) {
134+ if (count == undefined) count = 1;
135+ for (var i = 0; i < count; i++) {
136+ var startingAppId = ApplicationManager.availableApplications[ApplicationManager.count];
137+ var appSurfaceId = tlwm.nextId;
138+ var app = ApplicationManager.startApplication(startingAppId)
139+ tryCompare(app, "state", ApplicationInfoInterface.Running)
140+ waitUntilAppSurfaceShowsUp(appSurfaceId);
141+ waitForRendering(stage)
142+ tryCompare(ApplicationManager, "focusedApplicationId", startingAppId)
143+ }
144+ }
145+
146+ /*
147+ Ensure that closing a surface while rootFocus is off focuses the
148+ next available surface when rootFocus is given back.
149+
150+ Regression test for https://github.com/ubports/unity8/issues/234
151+
152+ This cannot be tested in tst_TopLevelWindowModel.cpp, the mocks for
153+ it are not advanced enough.
154+ */
155+ function test_closeLastFocusedApp()
156+ {
157+ var dialerApp = ApplicationManager.startApplication("dialer-app");
158+ var webbrowserSurfaceId = tlwm.nextId;
159+ var webbrowserApp = ApplicationManager.startApplication("morph-browser");
160+ waitUntilAppSurfaceShowsUp(webbrowserSurfaceId);
161+
162+ tlwm.rootFocus = false;
163+
164+ ApplicationManager.stopApplication(webbrowserApp.appId);
165+ // It should be gone soon
166+ tryCompareFunction(function() { return tlwm.indexForId(webbrowserSurfaceId); }, -1);
167+
168+ tlwm.rootFocus = true;
169+ compare(dialerApp.focused, true);
170+ }
171+
172+ /*
173+ Ensure that we can handle issuing windows up to maxWindowId,
174+ including reusing IDs once we're exhausted.
175+ */
176+ function test_issueAllTheIds()
177+ {
178+ tlwm.maxWindowId = 5;
179+
180+ // Start an app ourselves so we can close it
181+ var nextAppId = ApplicationManager.availableApplications[5];
182+ var app = ApplicationManager.startApplication(nextAppId);
183+ var appWindowId = tlwm.idAt(0);
184+
185+ // Add four more apps to bring us to five
186+ addApps(4);
187+
188+ ApplicationManager.stopApplication(app.appId);
189+ // It should be gone soon
190+ tryCompareFunction(function() { return tlwm.indexForId(appWindowId); }, -1);
191+
192+ // And now we can start a new one
193+ var appAgain = ApplicationManager.startApplication(nextAppId);
194+ verify(appAgain);
195+
196+ // The new app should have taken the ID of the closed app since
197+ // we're under contention.
198+ tryCompareFunction(function() { return tlwm.indexForId(appWindowId); }, 0);
199+ }
200+ }
201+}