f85133b · unity8 · git.iko.soy

git.iko.soy/unity8

on xenial help sign in

Make it less likely to issue duplicate Window IDs

f85133bDalton Durst7 years ago

plugins/WindowManager/TopLevelWindowModel.cpp+43−22

compare with xenial

unifiedsplit

@@ -1,5 +1,6 @@

1/*

3 *

4 *

4 * This program is free software: you can redistribute it and/or modify it under

5 * This program is free software: you can redistribute it and/or modify it under

5 * the terms of the GNU Lesser General Public License version 3, as published by

6 * the terms of the GNU Lesser General Public License version 3, as published by

@@ -38,7 +39,8 @@ Q_LOGGING_CATEGORY(TOPLEVELWINDOWMODEL, "toplevelwindowmodel", QtInfoMsg)

38namespace unityapi = unity::shell::application;

39namespace unityapi = unity::shell::application;

40TopLevelWindowModel::TopLevelWindowModel()

41TopLevelWindowModel::TopLevelWindowModel()

41− : m_nullWindow(createWindow(nullptr)),

42+ : m_nullWindow(createNullWindow()),

43+ m_maxWindowId(1000000),

42 m_surfaceManagerBusy(false)

44 m_surfaceManagerBusy(false)

43{

45{

44 connect(m_nullWindow, &Window::focusedChanged, this, [this] {

46 connect(m_nullWindow, &Window::focusedChanged, this, [this] {

@@ -350,6 +352,16 @@ void TopLevelWindowModel::onSurfaceDestroyed(unityapi::MirSurfaceInterface *surf

350Window *TopLevelWindowModel::createWindow(unityapi::MirSurfaceInterface *surface)

352Window *TopLevelWindowModel::createWindow(unityapi::MirSurfaceInterface *surface)

351{

353{

352 int id = generateId();

354 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+{

353 Window *qmlWindow = new Window(id, this);

365 Window *qmlWindow = new Window(id, this);

354 connectWindow(qmlWindow);

366 connectWindow(qmlWindow);

355 if (surface) {

367 if (surface) {

@@ -514,34 +526,29 @@ int TopLevelWindowModel::findIndexOf(const unityapi::MirSurfaceInterface *surfac

514 return -1;

526 return -1;

515}

527}

516

528

529+// Finds a unique integer identifier not greater than maxWindowId.

517int TopLevelWindowModel::generateId()

530int TopLevelWindowModel::generateId()

518{

531{

519− int id = m_nextId;

532+ int candidateId = m_nextId.fetchAndAddAcquire(1);

520− m_nextId = nextFreeId(nextId(id), id);

521− return id;

522−}

523

533

524−int TopLevelWindowModel::nextId(int id) const

534+ // If the user opens two windows at the exact same time after opening

525−{

535+ // maxWindowId windows, it is potentially possible that we issue the same

526− if (id == m_maxId) {

536+ // ID for both. This is a risk we can accept.

527− return id = 1;

537+ if (candidateId > getMaxWindowId()) {

528− } else {

538+ int originalCandidateId = candidateId;

529− return id + 1;

539+ candidateId = 1;

530− }

531−}

532

540

533−int TopLevelWindowModel::nextFreeId(int candidateId, const int latestId)

541+ while (indexForId(candidateId) != -1) {

534−{

542+ candidateId += 1;

535− int firstCandidateId = candidateId;

536

543

537− while (indexForId(candidateId) != -1 || candidateId == latestId) {

538− candidateId = nextId(candidateId);

539−

540− if (candidateId == firstCandidateId) {

544+ if (candidateId == originalCandidateId) {

545+ // The user has maxWindowId windows open somehow.

541− qFatal("TopLevelWindowModel: run out of window ids.");

546+ qFatal("TopLevelWindowModel: Ran out of window IDs!");

547+ }

542 }

548 }

543− }

544

549

550+ m_nextId.fetchAndStoreAcquire(candidateId + 1);

551+ }

545 return candidateId;

552 return candidateId;

546}

553}

547

554

@@ -788,6 +795,20 @@ void TopLevelWindowModel::setRootFocus(bool focus)

788 }

795 }

789}

796}

790

797

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+

791// Pending Activation will block refocus of previous focused window

812// Pending Activation will block refocus of previous focused window

792// this is needed since surface activation with miral is async,

813// this is needed since surface activation with miral is async,

793// and activation of placeholder is sync. This causes a race condition

814// and activation of placeholder is sync. This causes a race condition

plugins/WindowManager/TopLevelWindowModel.h+22−6

@@ -1,5 +1,6 @@

1/*

3 *

4 *

4 * This program is free software: you can redistribute it and/or modify it under

5 * This program is free software: you can redistribute it and/or modify it under

5 * the terms of the GNU Lesser General Public License version 3, as published by

6 * the terms of the GNU Lesser General Public License version 3, as published by

@@ -18,6 +19,7 @@

18#define TOPLEVELWINDOWMODEL_H

19#define TOPLEVELWINDOWMODEL_H

20#include <QAbstractListModel>

21#include <QAbstractListModel>

22+#include <QAtomicInteger>

21#include <QLoggingCategory>

23#include <QLoggingCategory>

23#include "WindowManagerGlobal.h"

25#include "WindowManagerGlobal.h"

@@ -86,7 +88,15 @@ class WINDOWMANAGERQML_EXPORT TopLevelWindowModel : public QAbstractListModel

86 The id to be used on the next entry created

88 The id to be used on the next entry created

87 Useful for tests

89 Useful for tests

88 */

90 */

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)

100

101

92 /**

102 /**

@@ -143,7 +153,7 @@ public:

143 unity::shell::application::SurfaceManagerInterface *surfaceManager() const { return m_surfaceManager; }

153 unity::shell::application::SurfaceManagerInterface *surfaceManager() const { return m_surfaceManager; }

144 void setSurfaceManager(unity::shell::application::SurfaceManagerInterface*);

154 void setSurfaceManager(unity::shell::application::SurfaceManagerInterface*);

145

155

146− int nextId() const { return m_nextId; }

156+ int nextId() const { return m_nextId.load(); }

147

157

148public:

158public:

149 /**

159 /**

@@ -198,6 +208,9 @@ public:

198 void setRootFocus(bool focus);

208 void setRootFocus(bool focus);

199 bool rootFocus();

209 bool rootFocus();

200

210

211+ void setMaxWindowId(int newMaxWindowId);

212+ int getMaxWindowId();

213+

201Q_SIGNALS:

214Q_SIGNALS:

202 void countChanged();

215 void countChanged();

203 void inputMethodSurfaceChanged(unity::shell::application::MirSurfaceInterface* inputMethodSurface);

216 void inputMethodSurfaceChanged(unity::shell::application::MirSurfaceInterface* inputMethodSurface);

@@ -212,12 +225,12 @@ Q_SIGNALS:

212 */

225 */

213 void listChanged();

226 void listChanged();

214

227

215− void nextIdChanged();

216−

217 void closedAllWindows();

228 void closedAllWindows();

218

229

219 void rootFocusChanged();

230 void rootFocusChanged();

220

231

232+ void maxWindowIdChanged();

233+

221private Q_SLOTS:

234private Q_SLOTS:

222 void onSurfaceCreated(unity::shell::application::MirSurfaceInterface *surface);

235 void onSurfaceCreated(unity::shell::application::MirSurfaceInterface *surface);

223 void onSurfacesRaised(const QVector<unity::shell::application::MirSurfaceInterface*> &surfaces);

236 void onSurfacesRaised(const QVector<unity::shell::application::MirSurfaceInterface*> &surfaces);

@@ -263,6 +276,8 @@ private:

263 void activateTopMostWindowWithoutId(int forbiddenId);

276 void activateTopMostWindowWithoutId(int forbiddenId);

264

277

265 Window *createWindow(unity::shell::application::MirSurfaceInterface *surface);

278 Window *createWindow(unity::shell::application::MirSurfaceInterface *surface);

279+ Window *createWindowWithId(unity::shell::application::MirSurfaceInterface *surface, int id);

280+ Window *createNullWindow();

266

281

267 struct ModelEntry {

282 struct ModelEntry {

268 ModelEntry() {}

283 ModelEntry() {}

@@ -281,11 +296,12 @@ private:

281 Window* m_previousWindow{nullptr};

296 Window* m_previousWindow{nullptr};

282 bool m_pendingActivation;

297 bool m_pendingActivation;

283

298

284− int m_nextId{1};

299+ QAtomicInteger<int> m_nextId{1};

300+

285 // Just something big enough that we don't risk running out of unused id numbers.

301 // Just something big enough that we don't risk running out of unused id numbers.

286 // Not sure if QML int type supports something close to std::numeric_limits<int>::max() and

302 // Not sure if QML int type supports something close to std::numeric_limits<int>::max() and

287 // there's no reason to try out its limits.

303 // there's no reason to try out its limits.

288− static const int m_maxId{1000000};

304+ int m_maxWindowId;

289

305

290 unity::shell::application::ApplicationManagerInterface* m_applicationManager{nullptr};

306 unity::shell::application::ApplicationManagerInterface* m_applicationManager{nullptr};

291 unity::shell::application::SurfaceManagerInterface *m_surfaceManager{nullptr};

307 unity::shell::application::SurfaceManagerInterface *m_surfaceManager{nullptr};

tests/plugins/WindowManager/tst_TopLevelWindowModel.cpp+2

@@ -14,6 +14,8 @@

14 * along with this program. If not, see <http://www.gnu.org/licenses/>.

15 */

17+// There is a companion set of QMLTests for TLWM in tst_QMLTopLevelWindowModel.qml.

18+

17#include <QtTest/QtTest>

19#include <QtTest/QtTest>

18#include <QSignalSpy>

20#include <QSignalSpy>

tests/qmltests/CMakeLists.txt+1

@@ -54,6 +54,7 @@ add_unity8_qmltest(Panel/Indicators MessageMenuItemFactory)

54add_unity8_qmltest(Stage ApplicationWindow)

55add_unity8_qmltest(Stage DesktopStage)

56add_unity8_qmltest(Stage PhoneStage)

57+add_unity8_qmltest(Stage QMLTopLevelWindowModel)

57add_unity8_qmltest(Stage SurfaceContainer)

58add_unity8_qmltest(Stage SurfaceContainer)

58add_unity8_qmltest(Stage TabletStage)

59add_unity8_qmltest(Stage TabletStage)

59add_unity8_qmltest(Stage WindowResizeArea)

60add_unity8_qmltest(Stage WindowResizeArea)

tests/qmltests/Stage/tst_PhoneStage.qml+1−27

@@ -1,5 +1,6 @@

1/*

3 *

4 *

4 * This program is free software; you can redistribute it and/or modify

5 * This program is free software; you can redistribute it and/or modify

5 * it under the terms of the GNU General Public License as published by

6 * it under the terms of the GNU General Public License as published by

@@ -582,32 +583,5 @@ Item {

582 compare(topLevelSurfaceList.idAt(0), webbrowserSurfaceId);

583 compare(topLevelSurfaceList.idAt(0), webbrowserSurfaceId);

583 compare(webbrowserApp.focused, true);

584 compare(webbrowserApp.focused, true);

584 }

585 }

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− }

612 }

586 }

613}

587}

tests/qmltests/Stage/tst_QMLTopLevelWindowModel.qmlnew file+201

@@ -0,0 +1,201 @@

1+/*

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+}