7028320 · unity8 · git.iko.soy

git.iko.soy/unity8

TopLevelWindowModel: Don't refocus closed Window

Prior to this, clearing rootFocus, closing the previously focused Window, then setting rootFocus would cause a use-after-delete, resulting in a segfault at best or undefined behavior at worst. Additionally, running 'm_previousWindow->activate()' can cause setRootFocus to be called a second time, when rootFocus is being used properly. To prevent this from causing trouble, ignore a call if the surfaceManager has notified us that it is making changes. Fixes https://github.com/ubports/unity8/issues/234

commit70283200ead9f3ac4de3d2a8557fe27ef2407781 authorDalton Durst <dalton@ubports.com> dateWed 11 Dec 2019, 21:23 (7 years ago) parent5e3b45c

3 files changed+50−2

compare with xenial

unifiedsplit

plugins/WindowManager/TopLevelWindowModel.cpp+19−2 plugins/WindowManager/TopLevelWindowModel.h+4 tests/qmltests/Stage/tst_PhoneStage.qml+27

plugins/WindowManager/TopLevelWindowModel.cpp+19−2

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

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

3939

4040TopLevelWindowModel::TopLevelWindowModel()

41− : m_nullWindow(createWindow(nullptr))

41+ : m_nullWindow(createWindow(nullptr)),

42+ m_surfaceManagerBusy(false)

4243{

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

4445 Q_EMIT rootFocusChanged();

@@ -439,6 +440,10 @@ void TopLevelWindowModel::removeAt(int index)

439440 setFocusedWindow(nullptr);

440441 }

441442

443+ if (m_previousWindow == window) {

444+ m_previousWindow = nullptr;

445+ }

446+

442447 if (m_closingAllApps) {

443448 if (m_windowModel.isEmpty()) {

444449 Q_EMIT closedAllWindows();

@@ -709,6 +714,7 @@ void TopLevelWindowModel::move(int from, int to)

709714}

710715void TopLevelWindowModel::onModificationsStarted()

711716{

717+ m_surfaceManagerBusy = true;

712718}

713719

714720void TopLevelWindowModel::onModificationsEnded()

@@ -719,6 +725,7 @@ void TopLevelWindowModel::onModificationsEnded()

719725 // reset

720726 m_focusedWindowChanged = false;

721727 m_newlyFocusedWindow = nullptr;

728+ m_surfaceManagerBusy = false;

722729}

723730

724731void TopLevelWindowModel::activateTopMostWindowWithoutId(int forbiddenId)

@@ -754,7 +761,14 @@ bool TopLevelWindowModel::rootFocus()

754761

755762void TopLevelWindowModel::setRootFocus(bool focus)

756763{

757− INFO_MSG << "(" << focus << ")";

764+ INFO_MSG << "(" << focus << "), surfaceManagerBusy is " << m_surfaceManagerBusy;

765+

766+ if (m_surfaceManagerBusy) {

767+ // Something else is probably being focused already, let's not add to

768+ // the noise.

769+ return;

770+ }

771+

758772 if (focus) {

759773 // Give focus back to previous focused window, only if null window is focused.

760774 // If null window is not focused, a different app had taken the focus and we

@@ -762,6 +776,9 @@ void TopLevelWindowModel::setRootFocus(bool focus)

762776 if (m_previousWindow && !m_previousWindow->focused() && !m_pendingActivation &&

763777 m_nullWindow == m_focusedWindow && m_previousWindow != m_nullWindow) {

764778 m_previousWindow->activate();

779+ } else if (!m_pendingActivation) {

780+ // The previous window does not exist any more, focus top window.

781+ activateTopMostWindowWithoutId(-1);

765782 }

766783 } else {

767784 if (!m_nullWindow->focused()) {

plugins/WindowManager/TopLevelWindowModel.h+4

@@ -103,6 +103,9 @@ class WINDOWMANAGERQML_EXPORT TopLevelWindowModel : public QAbstractListModel

103103 * Setting rootFocus attempts to focus the Window which was focused last -

104104 * unless another app is attempting to gain focus (as determined by

105105 * pendingActivation) and that's why we got rootFocus.

106+ *

107+ * If the previously-focused Window was closed before rootFocus was set,

108+ * the next available window will be focused.

106109 */

107110 Q_PROPERTY(bool rootFocus READ rootFocus WRITE setRootFocus NOTIFY rootFocusChanged)

108111

@@ -286,6 +289,7 @@ private:

286289

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

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

292+ bool m_surfaceManagerBusy;

289293

290294 enum ModelState {

291295 IdleState,

tests/qmltests/Stage/tst_PhoneStage.qml+27

@@ -592,5 +592,32 @@ Item {

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

593593 compare(webbrowserApp.focused, true);

594594 }

595+

596+ /*

597+ Ensure that closing a surface while rootFocus is off focuses the

598+ next available surface when rootFocus is given back.

599+

600+ Regression test for https://github.com/ubports/unity8/issues/234

601+

602+ This cannot be tested in tst_TopLevelWindowModel.cpp, the mocks for

603+ it are not advanced enough.

604+ */

605+ function test_closeFocusedAppWithSpreadOpen()

606+ {

607+ var dashApp = ApplicationManager.findApplication("unity8-dash");

608+ var webbrowserSurfaceId = topLevelSurfaceList.nextId;

609+ var webbrowserApp = ApplicationManager.startApplication("morph-browser");

610+ waitUntilAppSurfaceShowsUp(webbrowserSurfaceId);

611+

612+ topLevelSurfaceList.rootFocus = false;

613+

614+ performEdgeSwipeToShowAppSpread();

615+

616+ swipeSurfaceUpwards(webbrowserSurfaceId);

617+ tryCompareFunction(function() { return topLevelSurfaceList.indexForId(webbrowserSurfaceId); }, -1);

618+

619+ topLevelSurfaceList.rootFocus = true;

620+ compare(dashApp.focused, true);

621+ }

595622 }

596623}