Merge lp:~robertcarr/mir/focus-tell-dont-ask into lp:~mir-team/mir/trunk

[Robert Carr]

== Old Description ==

Fix the races around focus. Essentially these stem from that holding std::shared_ptr<msh::Surface> doesn't guarantee that calling methods on that surface will be safe (the underlying surface could be destroyed via Surface::destroy and then your method call will throw).

So this branch changes ApplicationSession to not destroy the surface explicitly and instead rely on ~msh::Surface, which means that you really can hold a safe pointer. I think this as a reasonable choice, as the shell might want to do similar things, i.e. continue to animate a surface after a session has closed it.

==

New approach relies on ApplicationSession to hold the last focused ptr...WIP

[Robert Carr (robertcarr)]

I was able to run the stress test with continuous cursor input for the full ten minute run. There is also good reason to believe this fixes: https://bugs.launchpad.net/mir/+bug/1220845

The exception in ~SessionMediator can be triggered if session_manager->close_session throws (due to this focus race), causing SessionMediator::disconnect to fail to reset it's session member-variable pointer. This exception is caught in frontend. However now in ~SessionMediator the pointer has not been reset so it closes the session again throwing an Invalid Session exception. causing the session to be closed again in ~

[Robert Carr (robertcarr)]

The unsafely held surface pointer in this case is the previously focused app.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1054
http://jenkins.qa.ubuntu.com/job/mir-ci/1446/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1952
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1837
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/684
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/684/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1446/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

There is an inconsistency with this approach: when a session is closed its surface resources are destroyed immediately, but when a surface is explicitly destroyed the resources are not destroyed until there are no more users.

Would it (make sense|be possible) for msh::Surface to own (hold a strong pointer to) ms::Surface and rely on ~msh::Surface() in both the cases above, thus removing the msh::Surface -> ms::Surface weak_ptr dance?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> There is an inconsistency with this approach: when a session is closed its
> surface resources are destroyed immediately, but when a surface is explicitly
> destroyed the resources are not destroyed until there are no more users.
>
> Would it (make sense|be possible) for msh::Surface to own (hold a strong
> pointer to) ms::Surface and rely on ~msh::Surface() in both the cases above,
> thus removing the msh::Surface -> ms::Surface weak_ptr dance?

I've not yet looked at the MP, but I can contribute the origin of the "weak_ptr dance".

Once a surface has been "destroyed" it is no longer in the scenegraph (ok, SurfaceStack) and therefore not composited. If the ms::Surface continued to exist it could reach a state where it has client buffers meaning threads that require them will wait forever.

It may well be that subsequent changes (particularly the introduction of force_requests_to_complete()) permits a better solution than the "weak_ptr dance".

[Alan Griffiths (alan-griffiths)]

I don't think this is the right evolution of the code, but I don't think it makes things worse.

The right solution would be more complex and should probably come as part of fixing the data model.

review: Abstain

[Robert Carr (robertcarr)]

I think it makes sense to not destroy the surfaces explicitly from the session (as in the last committ). It may be possible to fix the weak_ptr dance (it almost surely is possible). It's not really clear what direction this code should go in right now though (except probably not further down this one), so I am trying to just fix the crashes non invasively

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1055
http://jenkins.qa.ubuntu.com/job/mir-ci/1454/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1963
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1848
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/692
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/692/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1454/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Bug 1216727 is still happening :(

terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<std::logic_error> >'
  what(): Requesting handle for an unregistered channel

Should we just unlink it from the branch?

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

P.S. Before declaring bug 1216727 fixed, please find a machine on which you can reproduce it (using trunk). To be sure the changes are actually having an effect.

[Robert Carr (robertcarr)]

Can someone review this again?

[Daniel van Vugt (vanvugt)]

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1057
http://jenkins.qa.ubuntu.com/job/mir-ci/1524/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2062
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1947
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/762/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/38
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/38/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1524/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Weirdly, merging this branch now causes an almost immediate hang:

Running tests...
Test project /home/dan/bzr/mir/tmp.focus/build
        Start 1: LGPL-required
  1/145 Test #1: LGPL-required ............................................... Passed 0.01 sec
        Start 2: GPL-required
  2/145 Test #2: GPL-required ................................................ Passed 0.01 sec
        Start 3: acceptance-tests.BespokeDisplayServerTestFixture.*

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1058
http://jenkins.qa.ubuntu.com/job/mir-ci/1528/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2068
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1953
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/766/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/42/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1528/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Another bug with this branch is that on Alt+Tab in demo_server_shell, the client rendering freezes. Permanently.

review: Needs Fixing

[lp:~robertcarr/mir/focus-tell-dont-ask]

1085. By Robert Carr on 2013-09-23

Typo

1086. By Robert Carr on 2013-09-26

Merge development-branch

1087. By Robert Carr on 2013-09-26

Merge socket-messenger-reporitng

1088. By Robert Carr on 2013-09-26

Fix error in relinquish focus

Unmerged revisions

1088. By Robert Carr on 2013-09-26

Fix error in relinquish focus

1087. By Robert Carr on 2013-09-26

Merge socket-messenger-reporitng

1086. By Robert Carr on 2013-09-26

Merge development-branch

1085. By Robert Carr on 2013-09-23

Typo

1084. By Robert Carr on 2013-09-23

application_session: Introduce default_surface_locked to dedupe some code

1083. By Robert Carr on 2013-09-23

Merge fix-1226139

1082. By Robert Carr on 2013-09-23

More cleanup

1081. By Robert Carr on 2013-09-23

Merge trunk

1080. By Robert Carr on 2013-09-23

Cleanup

1079. By Robert Carr on 2013-09-20

Mutex for surface

=== modified file 'include/server/mir/frontend/communicator_report.h'
--- include/server/mir/frontend/communicator_report.h	2013-06-19 16:14:40 +0000
+++ include/server/mir/frontend/communicator_report.h	2013-09-23 14:32:39 +0000
@@ -42,7 +42,6 @@
 class NullCommunicatorReport : public CommunicatorReport
 {
 public:
-
     void error(std::exception const& error);
 };
 }

=== modified file 'include/server/mir/shell/application_session.h'
--- include/server/mir/shell/application_session.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/application_session.h	2013-09-23 14:32:39 +0000
@@ -67,6 +67,9 @@
 
     void set_lifecycle_state(MirLifecycleState state);
 
+    void receive_focus(std::shared_ptr<InputTargeter> const& targeter, std::shared_ptr<SurfaceController> const& controller);
+    void relinquish_focus();
+
 protected:
     ApplicationSession(ApplicationSession const&) = delete;
     ApplicationSession& operator=(ApplicationSession const&) = delete;
@@ -79,6 +82,7 @@
     std::shared_ptr<frontend::EventSink> const event_sink;
 
     frontend::SurfaceId next_id();
+    std::shared_ptr<Surface> default_surface_locked(std::unique_lock<std::mutex> const& lock) const;
 
     std::atomic<int> next_surface_id;
 
@@ -86,6 +90,8 @@
     Surfaces::const_iterator checked_find(frontend::SurfaceId id) const;
     std::mutex mutable surfaces_mutex;
     Surfaces surfaces;
+    
+    std::weak_ptr<Surface> last_focused_surface;
 };
 
 }

=== modified file 'include/server/mir/shell/default_focus_mechanism.h'
--- include/server/mir/shell/default_focus_mechanism.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/default_focus_mechanism.h	2013-09-23 14:32:39 +0000
@@ -50,8 +50,8 @@
     std::shared_ptr<InputTargeter> const input_targeter;
     std::shared_ptr<SurfaceController> const surface_controller;
 
-    std::mutex surface_focus_lock;
-    std::weak_ptr<Surface> currently_focused_surface;
+    std::mutex focus_lock;
+    std::weak_ptr<Session> currently_focused_session;
 };
 
 }

=== modified file 'include/server/mir/shell/session.h'
--- include/server/mir/shell/session.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/session.h	2013-09-23 14:32:39 +0000
@@ -28,6 +28,8 @@
 namespace shell
 {
 class Surface;
+class InputTargeter;
+class SurfaceController;
 
 class Session : public frontend::Session
 {
@@ -37,6 +39,9 @@
 
     virtual void take_snapshot(SnapshotCallback const& snapshot_taken) = 0;
     virtual std::shared_ptr<Surface> default_surface() const = 0;
+    
+    virtual void receive_focus(std::shared_ptr<InputTargeter> const& targeter, std::shared_ptr<SurfaceController> const& controller) = 0;
+    virtual void relinquish_focus() = 0;
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_shell_session.h'
--- include/test/mir_test_doubles/mock_shell_session.h	2013-08-28 03:41:48 +0000
+++ include/test/mir_test_doubles/mock_shell_session.h	2013-09-23 14:32:39 +0000
@@ -49,6 +49,9 @@
     
     MOCK_METHOD1(send_display_config, void(graphics::DisplayConfiguration const&));
     MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int));
+    
+    MOCK_METHOD2(receive_focus, void(std::shared_ptr<shell::InputTargeter> const&, std::shared_ptr<shell::SurfaceController> const&));
+    MOCK_METHOD0(relinquish_focus, void());
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_shell_session.h'
--- include/test/mir_test_doubles/stub_shell_session.h	2013-08-13 22:20:37 +0000
+++ include/test/mir_test_doubles/stub_shell_session.h	2013-09-23 14:32:39 +0000
@@ -71,6 +71,14 @@
     {
         return std::shared_ptr<shell::Surface>();
     }
+
+    void receive_focus(std::shared_ptr<shell::InputTargeter> const&,
+                       std::shared_ptr<shell::SurfaceController> const&)
+    {
+    }
+    void relinquish_focus()
+    {
+    }
 };
 
 }

=== modified file 'src/server/frontend/protobuf_socket_communicator.cpp'
--- src/server/frontend/protobuf_socket_communicator.cpp	2013-08-28 03:41:48 +0000
+++ src/server/frontend/protobuf_socket_communicator.cpp	2013-09-23 14:32:39 +0000
@@ -135,7 +135,7 @@
 {
     if (!ec)
     {
-        auto messenger = std::make_shared<detail::SocketMessenger>(socket);
+        auto messenger = std::make_shared<detail::SocketMessenger>(socket, report);
         auto client_pid = messenger->client_pid(); 
         if (session_authorizer->connection_is_allowed(client_pid))
         {

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2013-08-28 03:41:48 +0000
+++ src/server/frontend/socket_messenger.cpp	2013-09-23 14:32:39 +0000
@@ -18,13 +18,16 @@
 
 #include "socket_messenger.h"
 #include "mir/frontend/client_constants.h"
+#include "mir/frontend/communicator_report.h"
 
 namespace mfd = mir::frontend::detail;
 namespace bs = boost::system;
 namespace ba = boost::asio;
 
-mfd::SocketMessenger::SocketMessenger(std::shared_ptr<ba::local::stream_protocol::socket> const& socket)
-    : socket(socket)
+mfd::SocketMessenger::SocketMessenger(std::shared_ptr<ba::local::stream_protocol::socket> const& socket,
+                                      std::shared_ptr<CommunicatorReport> const& report)
+    : socket(socket),
+      report(report)
 {
     whole_message.reserve(serialization_buffer_size);
 }
@@ -59,7 +62,14 @@
     // function has completed (if it would be executed asynchronously.
     // NOTE: we rely on this synchronous behavior as per the comment in
     // mf::SessionMediator::create_surface
-    ba::write(*socket, ba::buffer(whole_message));
+    try
+    {
+        ba::write(*socket, ba::buffer(whole_message));
+    }
+    catch (std::exception &ex)
+    {
+        report->error(ex);
+    }
 }
 
 void mfd::SocketMessenger::send_fds(std::vector<int32_t> const& fds)

=== modified file 'src/server/frontend/socket_messenger.h'
--- src/server/frontend/socket_messenger.h	2013-08-28 03:41:48 +0000
+++ src/server/frontend/socket_messenger.h	2013-09-23 14:32:39 +0000
@@ -26,13 +26,16 @@
 {
 namespace frontend
 {
+class CommunicatorReport;
+
 namespace detail
 {
 class SocketMessenger : public MessageSender,
                         public MessageReceiver
 {
 public:
-    SocketMessenger(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket);
+    SocketMessenger(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket,
+                    std::shared_ptr<frontend::CommunicatorReport> const& report);
 
     void send(std::string const& body);
     void send_fds(std::vector<int32_t> const& fds);
@@ -42,6 +45,8 @@
 
 private:
     std::shared_ptr<boost::asio::local::stream_protocol::socket> socket;
+    std::shared_ptr<CommunicatorReport> const& report;
+
     std::vector<char> whole_message;
 };
 }

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-08-28 03:41:48 +0000
+++ src/server/shell/application_session.cpp	2013-09-23 14:32:39 +0000
@@ -21,6 +21,7 @@
 #include "mir/shell/surface_factory.h"
 #include "mir/shell/snapshot_strategy.h"
 #include "mir/shell/session_listener.h"
+#include "mir/shell/input_targeter.h"
 #include "mir/frontend/event_sink.h"
 
 #include <boost/throw_exception.hpp>
@@ -99,16 +100,21 @@
     snapshot_strategy->take_snapshot_of(default_surface(), snapshot_taken);
 }
 
-std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface() const
+std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface_locked(std::unique_lock<std::mutex> const& /* lock */) const
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
-
     if (surfaces.size())
         return surfaces.begin()->second;
     else
         return std::shared_ptr<msh::Surface>();
 }
 
+std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface() const
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+
+    return default_surface_locked(lock);
+}
+
 void msh::ApplicationSession::destroy_surface(mf::SurfaceId id)
 {
     std::unique_lock<std::mutex> lock(surfaces_mutex);
@@ -171,3 +177,32 @@
 {
     event_sink->handle_lifecycle_event(state);
 }
+
+void msh::ApplicationSession::receive_focus(std::shared_ptr<msh::InputTargeter> const& targeter,
+                                            std::shared_ptr<SurfaceController> const& controller)
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    if (surfaces.size() == 0)
+    {
+        targeter->focus_cleared();
+        return;
+    }
+    // Use default_surface_locked idiom instead
+    auto focus_surface = surfaces.begin()->second;
+    
+    focus_surface->raise(controller);
+    focus_surface->take_input_focus(targeter);
+    focus_surface->configure(mir_surface_attrib_focus, mir_surface_focused);
+
+    last_focused_surface = focus_surface;
+}
+
+void msh::ApplicationSession::relinquish_focus()
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    auto surf = default_surface_locked(lock);
+    if (surf)
+    {
+        surf->configure(mir_surface_attrib_focus, mir_surface_unfocused);
+    }
+}

=== modified file 'src/server/shell/default_focus_mechanism.cpp'
--- src/server/shell/default_focus_mechanism.cpp	2013-08-28 03:41:48 +0000
+++ src/server/shell/default_focus_mechanism.cpp	2013-09-23 14:32:39 +0000
@@ -35,28 +35,20 @@
 
 void msh::DefaultFocusMechanism::set_focus_to(std::shared_ptr<Session> const& focus_session)
 {
+    std::lock_guard<std::mutex> lg(focus_lock);
+
+    auto last_session = currently_focused_session.lock();
+    if (last_session)
+    {
+        last_session->relinquish_focus();
+    }
+
     // TODO: This path should be encapsulated in a seperate clear_focus message
     if (!focus_session)
     {
         input_targeter->focus_cleared();
         return;
     }
-    
-    auto surface = focus_session->default_surface();
-    if (surface)
-    {
-        std::lock_guard<std::mutex> lg(surface_focus_lock);
-        auto current_focus = currently_focused_surface.lock();
-        if (current_focus)
-            current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
-        surface->configure(mir_surface_attrib_focus, mir_surface_focused);
-        currently_focused_surface = surface;
-        
-        surface->raise(surface_controller);
-        surface->take_input_focus(input_targeter);
-    }
-    else
-    {
-        input_targeter->focus_cleared();
-    }
+    focus_session->receive_focus(input_targeter, surface_controller);
+    currently_focused_session = focus_session;
 }

=== modified file 'tests/unit-tests/frontend/CMakeLists.txt'
--- tests/unit-tests/frontend/CMakeLists.txt	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/frontend/CMakeLists.txt	2013-09-23 14:32:39 +0000
@@ -8,6 +8,7 @@
   ${CMAKE_CURRENT_SOURCE_DIR}/test_resource_cache.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_session_mediator.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_session.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_messenger.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_event_sender.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_global_event_sender.cpp
 )

=== added file 'tests/unit-tests/frontend/test_socket_messenger.cpp'
--- tests/unit-tests/frontend/test_socket_messenger.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit-tests/frontend/test_socket_messenger.cpp	2013-09-23 14:32:39 +0000
@@ -0,0 +1,37 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "mir/frontend/communicator_report.h"
+#include "src/server/frontend/socket_messenger.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace mf = mir::frontend;
+
+using namespace mf::detail;
+using namespace boost::asio;
+
+TEST(SocketMessengerTest, write_failures_never_throw)
+{
+    io_service svc;
+    auto sock = std::make_shared<local::stream_protocol::socket>(svc);
+    SocketMessenger mess(sock, std::make_shared<mf::NullCommunicatorReport>());
+
+    EXPECT_NO_THROW( mess.send("foo") );
+}

=== modified file 'tests/unit-tests/shell/test_default_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_default_focus_mechanism.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/shell/test_default_focus_mechanism.cpp	2013-09-23 14:32:39 +0000
@@ -47,80 +47,34 @@
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
-TEST(DefaultFocusMechanism, raises_default_surface)
-{
-    using namespace ::testing;
-    
-    NiceMock<mtd::MockShellSession> app1;
-    mtd::MockSurface mock_surface(&app1, std::make_shared<mtd::StubSurfaceBuilder>());
-    {
-        InSequence seq;
-        EXPECT_CALL(app1, default_surface()).Times(1)
-            .WillOnce(Return(mt::fake_shared(mock_surface)));
-    }
-
-    auto controller = std::make_shared<mtd::StubSurfaceController>();
-    EXPECT_CALL(mock_surface, raise(Eq(controller))).Times(1);
-    mtd::StubInputTargeter targeter;
-    msh::DefaultFocusMechanism focus_mechanism(mt::fake_shared(targeter), controller);
-    
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-}
-
-TEST(DefaultFocusMechanism, mechanism_notifies_default_surface_of_focus_changes)
-{
-    using namespace ::testing;
-
-    NiceMock<mtd::MockShellSession> app1, app2;
-    mtd::MockSurface mock_surface1(&app1, std::make_shared<mtd::StubSurfaceBuilder>());
-    mtd::MockSurface mock_surface2(&app2, std::make_shared<mtd::StubSurfaceBuilder>());
-    
-    ON_CALL(app1, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface1)));
-    ON_CALL(app2, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface2)));
-
-    
-    {
-        InSequence seq;
-        EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
-        EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1);
-        EXPECT_CALL(mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
-    }
-
-    msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(),
-                                                        std::make_shared<mtd::StubSurfaceController>());
-
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(mt::fake_shared(app2));
-}
-
-TEST(DefaultFocusMechanism, sets_input_focus)
-{
-    using namespace ::testing;
-    
-    NiceMock<mtd::MockShellSession> app1;
-    mtd::MockSurface mock_surface(&app1, std::make_shared<mtd::StubSurfaceBuilder>());
-    {
-        InSequence seq;
-        EXPECT_CALL(app1, default_surface()).Times(1)
-            .WillOnce(Return(mt::fake_shared(mock_surface)));
-        EXPECT_CALL(app1, default_surface()).Times(1)
-            .WillOnce(Return(std::shared_ptr<msh::Surface>()));
-    }
-
-    mtd::MockInputTargeter targeter;
-    
-    msh::DefaultFocusMechanism focus_mechanism(mt::fake_shared(targeter), std::make_shared<mtd::StubSurfaceController>());
-    
-    {
-        InSequence seq;
-        EXPECT_CALL(mock_surface, take_input_focus(_)).Times(1);
-        // When we have no default surface.
-        EXPECT_CALL(targeter, focus_cleared()).Times(1);
-        // When we have no session.
-        EXPECT_CALL(targeter, focus_cleared()).Times(1);
-    }
-    
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(std::shared_ptr<msh::Session>());
+TEST(DefaultFocusMechanism, hands_focus_to_session)
+{
+    using namespace ::testing;
+    
+    mtd::MockShellSession app1;
+    EXPECT_CALL(app1, receive_focus(_, _)).Times(1);
+    
+    msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(), std::make_shared<mtd::StubSurfaceController>());
+
+    focus_mechanism.set_focus_to(mt::fake_shared(app1));
+}
+
+TEST(DefaultFocusMechanism, relinquishes_focus_from_last_session)
+{
+    using namespace ::testing;
+    
+    auto app1 = std::make_shared<mtd::MockShellSession>();
+    auto app2 = std::make_shared<mtd::MockShellSession>();
+    
+    {
+        InSequence seq;
+        EXPECT_CALL(*app1, receive_focus(_, _)).Times(1);
+        EXPECT_CALL(*app1, relinquish_focus()).Times(1);
+        EXPECT_CALL(*app2, receive_focus(_, _)).Times(1);
+    }
+    
+    msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(), std::make_shared<mtd::StubSurfaceController>());
+
+    focus_mechanism.set_focus_to(app1);
+    focus_mechanism.set_focus_to(app2);
 }