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

[Robert Carr]

Handle tracking the last focused surface inside the ApplicationSession

Fixed error in ApplicationSession::relinquish_focus :)

Targeting to development-branch

===

Solve some of the races around focus by applying 'tell-dont-ask' in relation to internal Session state and the Focus Mechanism. Now the focus mechanism doesn't have to hold an (unsafe) shared_ptr to the last focused surface. It's ok for the Session to hold this weak_ptr, as it only attempts to lock it when holding surfaces_mutex.

This branch requires: https://code.launchpad.net/~vanvugt/mir/fix-1226139/+merge/186809

As it will cause unfocus messages to be sent in the "connection dropped without disconnect case"(i.e. many of the protobuf tool tests which fail to call disconnect) very likely to lead to a broken pipe error.

== 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

[Robert Carr (robertcarr)]

The diff noise is because I merged https://code.launchpad.net/~robertcarr/mir/socket-messenger-reporting/+merge/187023 in but only set the branch dependency on https://code.launchpad.net/~vanvugt/mir/fix-1226139/+merge/186809

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1084
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/~robertcarr/mir/focus-tell-dont-ask/+merge/187037/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1558/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2090
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1975
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/796/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/58
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/58/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[kevin gunn (kgunn72)]

wouldn't this require a server api bump? i think so...

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1085
http://jenkins.qa.ubuntu.com/job/mir-ci/1562/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2098
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1983
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/800/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/62
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-armhf-ci/62/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

On both g++ and clang 64bit builds I see:

[ FAILED ] 2 tests, listed below:
[ FAILED ] BespokeDisplayServerTestFixture.a_surface_is_notified_of_receiving_focus
[ FAILED ] BespokeDisplayServerTestFixture.two_surfaces_are_notified_of_gaining_and_losing_focus

(and so does jenkins on amd64)

review: Needs Fixing

[Robert Ancell (robert-ancell)]

This merge should be moved to lp:~mir-team/mir/development-branch

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1085
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/~robertcarr/mir/focus-tell-dont-ask/+merge/187648/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/31/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2142/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2027/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/31/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/31/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1088
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/~robertcarr/mir/focus-tell-dont-ask/+merge/187886/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/44/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2157
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2042
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/44
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/44/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/44/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1088
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/45/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2159
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2044
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/45
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/45/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-team-mir-development-branch-ci/45/rebuild

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

I think the idea is a step in the right direction, given the problem :) (makes more sense than the shared_ptr in the focus controller)

receive_focus/relinquish focus seems a funny name for what's going on, given that there's no exchange of resources.
There's some gaps with who really has the-focus though still, like with,
148 + if (surfaces.size() == 0)
149 + {
150 + targeter->focus_cleared();
151 + return;
152 + }
when this happens, the focus controller is unaware that it could reassign the focus... perhaps the session should accept or reject the focus, and let the focus mechanism know. I Maybe we're twisting around the funny way ms::Surface is.

[Alan Griffiths (alan-griffiths)]

It looked weird before, it looks weird after

review: Abstain

[Daniel van Vugt (vanvugt)]

The prereq branch needs fixing or even rejecting. And therefore its dependents will need updating too.

review: Needs Fixing

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/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-26 22:16:32 +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-26 22:16:32 +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-26 22:16:32 +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-26 22:16:32 +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-26 22:16:32 +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/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-26 22:16:32 +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,33 @@
 {
     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 = last_focused_surface.lock();
+    if (surf)
+    {
+        surf->configure(mir_surface_attrib_focus, mir_surface_unfocused);
+    }
+    last_focused_surface.reset();
+}

=== 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-26 22:16:32 +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/acceptance-tests/test_client_focus_notification.cpp'
--- tests/acceptance-tests/test_client_focus_notification.cpp	2013-09-17 14:31:42 +0000
+++ tests/acceptance-tests/test_client_focus_notification.cpp	2013-09-26 22:16:32 +0000
@@ -159,7 +159,9 @@
         void expect_events(mt::WaitCondition* all_events_received) override
         {
             EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))).Times(1)
-                .WillOnce(mt::WakeUp(all_events_received));
+                .WillOnce(mt::WakeUp(all_events_received)); // Now we close the surface generating an unfocused event
+            // libmirclient guarantees we will see this before mir_surface_release finishes, so we don't need further
+            // synchronization
             EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_unfocused)))).Times(1);
         }
     } client_config;

=== 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-26 22:16:32 +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);
 }