Merge lp:~robertcarr/mir/session-transactions into lp:~mir-team/mir/trunk

[Robert Carr]

Perform focus setting transactionally, enabling the stress test suite to run.

This branch is a stab at fixing:

https://bugs.launchpad.net/mir/+bug/1195089

First the back story:

The way the crash from mir_stress presented itself from me was a throw in SessionManager::close_session "Invalid Session" when called from ~SessionMediator. That is to say, somehow we still have the msh::Session shared_ptr around but it has already been removed from the session manager. A bit of digging revealed the problem presents from SessionMediator::disconnect. SessionManager::close_session as called from SessionMediator::disconnect can throw in a few different ways, and when it does so the SessionMediator will be destroyed, but fail to reset the session pointer (which it does immediately following the call to SessionManager::close_session).

I tried logging this exception, and found that all the exceptions which could be thrown originated from setting the focus when closing a surface. I found the following two:

1. SingleVisibilityFocusMechanism requests the default_surface of a session, getting std::shared_ptr<msh::Session>. Now say, we are prempted by a frontend thread which closes the surface we have just grabbed. So now our msh::Session pointer is still valid, but the call to lock the internal ms::Surface will fail and throw an exception "Invalid surface" (when we call ->set_input_focus).
2. Things start the same way, but the surface is destroyed later, say after we enter AndroidInputTargeter::focus_changed(std::shared_ptr<mi::InputChannel>) we are preempted by a frontend thread destroying the surface. Now we try and find the input window handle for InputChannel (back in the focus_changed thread), but it has already been unregistered, and so we throw "Window handle requested for unregistered surface."

In order to solve this, I've introduced an API, msh::Session::transaction(std::function). The semantics of this function are easy to describe:

TEST(Session, create_and_destroy_surface_from_other_threads_blocks_during_transaction)

Unfortunately this test is quite difficult to write deterministic-ally so the function is essentially untested. I came up with one solution using real-time scheduling attributes (SCHED_FIFO), but this requires running the tests as root or using root to set a system capability on the test binary. If we want to do this/can think of a good way I can write the test.

With this branch I was able to run the stress test for several minutes and drag the cursor around. Eventually I got bored.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:857
http://jenkins.qa.ubuntu.com/job/mir-ci/1056/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1373
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1258
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/294
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/294/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/680

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

+ // from a second thread before input focus is set.xs
Use emacs much?

[Robert Ancell (robert-ancell)]

I'd prefer a second opinion on using the transaction function over just using a lock for consistency with the other code, but otherwise it seems to make sense.

review: Approve

[lp:~robertcarr/mir/session-transactions]

858. By Robert Carr on 2013-07-19

Remove secret emacs signet ring

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:858
http://jenkins.qa.ubuntu.com/job/mir-ci/1063/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/301
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/301/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I see the need, but don't like the solution. Maybe my next article should be "recursive_mutex considered evil"?

What we are lacking is a clear synchronization strategy in our data model. (C.f. "The case for a "SceneGraph"" on mir-devel.)

I'm tempted to hold my nose and "Approve" but will try to think of a cleaner solution first.

review: Abstain

[Alan Griffiths (alan-griffiths)]

...having thought about it: doesn't the problem stem from not holding SessionManager::mutex when these actions occur?

void msh::SessionManager::close_session(std::shared_ptr<mf::Session> const& session)
{
    auto shell_session = std::dynamic_pointer_cast<Session>(session);

    session_listener->stopping(shell_session);

    app_container->remove_session(shell_session);

    std::unique_lock<std::mutex> lock(mutex);
    set_focus_to_locked(lock, focus_sequence->default_focus());
}

Note that the SessionManager::mutex isn't seized until after the remove_session() call.

inline void msh::SessionManager::set_focus_to_locked(std::unique_lock<std::mutex> const&, std::shared_ptr<Session> const& shell_session)
{
    auto old_focus = focus_application.lock();

    focus_application = shell_session;

    focus_setter->set_focus_to(shell_session);
    if (shell_session)
    {
        session_listener->focused(shell_session);
    }
    else
    {
        session_listener->unfocused();
    }
}

Note that the SessionManager::mutex isn't seized.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> inline void
> msh::SessionManager::set_focus_to_locked(std::unique_lock<std::mutex> const&,
> std::shared_ptr<Session> const& shell_session)
> {
> auto old_focus = focus_application.lock();
>
> focus_application = shell_session;
>
> focus_setter->set_focus_to(shell_session);
> if (shell_session)
> {
> session_listener->focused(shell_session);
> }
> else
> {
> session_listener->unfocused();
> }
> }
>
> Note that the SessionManager::mutex isn't seized.

Sorry, that last bit is a thinko.

[Alan Griffiths (alan-griffiths)]

Also, is there any need for DefaultSessionContainer::remove_session() to throw? It can (trivially) meet its post condition.

[Robert Carr (robertcarr)]

Holding the SessionManager mutex isn't enough. Not that close_surface isn't routed through the SessionManager which is where this race is triggered. The existing locking around close_session is asctually enough to prevent it.

DefaultSessionContainer::remove_session perhaps need not throw (though I think this usage is indicative of programmer error). Still though, that would leave the race around WindowHandleRepository.

More than those two, there is a race in every portion of the shell which has code like:

Step 1: Get surface from session
Step 2: Query properties from surface.

Step 2 can always throw as it stands and there is no reasonable way to get around it.

[Robert Carr (robertcarr)]

Does anyone have opinions?

[Alan Griffiths (alan-griffiths)]

OK, I'm beginning to see that the code is broken worse than I thought.

In any case, the problems arise from a poorly synchronized model - and part of that is that some locks are in the wrong place (i.e. in the controller).

Putting recursive locks into the controller seems like the wrong answer.

Sadly, fixing the model ("scenegraph") is a larger conversation.

review: Needs Fixing

Unmerged revisions

858. By Robert Carr on 2013-07-19

Remove secret emacs signet ring

857. By Robert Carr on 2013-07-18

Set focus transactionally

856. By Robert Carr on 2013-07-17

Hack

855. By Daniel van Vugt on 2013-07-16

buffer_swapper_spin.h: Remove dead code: initialize_queues.

Approved by PS Jenkins bot, Alan Griffiths.

854. By Stephen M. Webb on 2013-07-15

disable running the integration test suite on arm architecture during the packaging builds (lp: #1195265). Fixes: https://bugs.launchpad.net/bugs/1195260, https://bugs.launchpad.net/bugs/1195265.

Approved by PS Jenkins bot, Kevin DuBois.

853. By Alan Griffiths on 2013-07-15

doc: use house font and colors for coding guidelines.

Approved by PS Jenkins bot, Kevin DuBois, Alexandros Frantzis.

852. By Robert Carr on 2013-07-15

Implement a connection authorization mechanism.

Approved by Gerry Boland, Alan Griffiths, Robert Ancell, PS Jenkins bot.

851. By Robert Ancell on 2013-07-14

Releasing 0.0.7

850. By Robert Carr on 2013-07-12

Extract DefaultServerConfiguration::the_cursor_listener from DefaultServerConfiguration::the_input_configuration. Fixes: https://bugs.launchpad.net/bugs/1192916.

Approved by PS Jenkins bot, Alan Griffiths, Alexandros Frantzis, Robert Ancell.

849. By Alan Griffiths on 2013-07-12

geometry: make geometry compound types easier to construct. Fixes: https://bugs.launchpad.net/bugs/1199756.

Approved by PS Jenkins bot, Alexandros Frantzis.

=== modified file 'include/server/mir/shell/application_session.h'
--- include/server/mir/shell/application_session.h	2013-06-25 13:19:15 +0000
+++ include/server/mir/shell/application_session.h	2013-07-19 01:29:27 +0000
@@ -66,6 +66,8 @@
     void show();
 
     int configure_surface(frontend::SurfaceId id, MirSurfaceAttrib attrib, int value);
+    
+    void transaction(std::function<void()> const& body);
 
 protected:
     ApplicationSession(ApplicationSession const&) = delete;
@@ -83,7 +85,7 @@
 
     typedef std::map<frontend::SurfaceId, std::shared_ptr<Surface>> Surfaces;
     Surfaces::const_iterator checked_find(frontend::SurfaceId id) const;
-    std::mutex mutable surfaces_mutex;
+    std::recursive_mutex mutable surfaces_mutex;
     Surfaces surfaces;
 };
 
 
=== modified file 'include/server/mir/shell/session.h'
--- include/server/mir/shell/session.h	2013-06-25 13:19:15 +0000
+++ include/server/mir/shell/session.h	2013-07-19 01:29:27 +0000
@@ -22,6 +22,8 @@
 #include "mir/frontend/session.h"
 #include "mir/shell/snapshot.h"
 
+#include <functional>
+
 namespace mir
 {
 
@@ -37,6 +39,11 @@
 
     virtual void take_snapshot(SnapshotCallback const& snapshot_taken) = 0;
     virtual std::shared_ptr<Surface> default_surface() const = 0;
+    
+    /// Executes the body callback as a transaction. That is to say
+    /// other requests to this session will block during the execution
+    //  of body.
+    virtual void transaction(std::function<void()> const& body) = 0;
 };
 
 }
 
=== modified file 'include/test/mir_test_doubles/mock_session.h'
--- include/test/mir_test_doubles/mock_session.h	2013-05-21 17:16:43 +0000
+++ include/test/mir_test_doubles/mock_session.h	2013-07-19 01:29:27 +0000
@@ -42,7 +42,7 @@
     MOCK_METHOD0(hide, void());
     MOCK_METHOD0(show, void());
 
-    MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int));
+    MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int));    
 };
 
 }
 
=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-06-25 13:19:15 +0000
+++ src/server/shell/application_session.cpp	2013-07-19 01:29:27 +0000
@@ -56,7 +56,7 @@
 
 msh::ApplicationSession::~ApplicationSession()
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     for (auto const& pair_id_surface : surfaces)
     {
         pair_id_surface.second->destroy();
@@ -73,7 +73,7 @@
     auto const id = next_id();
     auto surf = surface_factory->create_surface(params, id, event_sink);
 
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     surfaces[id] = surf;
     return id;
 }
@@ -90,7 +90,7 @@
 
 std::shared_ptr<mf::Surface> msh::ApplicationSession::get_surface(mf::SurfaceId id) const
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
 
     return checked_find(id)->second;
 }
@@ -102,7 +102,7 @@
 
 std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface() const
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
 
     if (surfaces.size())
         return surfaces.begin()->second;
@@ -112,7 +112,7 @@
 
 void msh::ApplicationSession::destroy_surface(mf::SurfaceId id)
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     auto p = checked_find(id);
 
     p->second->destroy();
@@ -126,7 +126,7 @@
 
 void msh::ApplicationSession::force_requests_to_complete()
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     for (auto& id_s : surfaces)
     {
         id_s.second->force_requests_to_complete();
@@ -135,7 +135,7 @@
 
 void msh::ApplicationSession::hide()
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     for (auto& id_s : surfaces)
     {
         id_s.second->hide();
@@ -144,7 +144,7 @@
 
 void msh::ApplicationSession::show()
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     for (auto& id_s : surfaces)
     {
         id_s.second->show();
@@ -155,8 +155,14 @@
                                                MirSurfaceAttrib attrib,
                                                int value)
 {
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::unique_lock<std::recursive_mutex> lock(surfaces_mutex);
     std::shared_ptr<mf::Surface> surf(checked_find(id)->second);
 
     return surf->configure(attrib, value);
 }
+
+void msh::ApplicationSession::transaction(std::function<void()> const& body)
+{
+    std::unique_lock<std::recursive_mutex> lg(surfaces_mutex);
+    body();
+}
 
=== modified file 'src/server/shell/single_visibility_focus_mechanism.cpp'
--- src/server/shell/single_visibility_focus_mechanism.cpp	2013-05-28 15:44:18 +0000
+++ src/server/shell/single_visibility_focus_mechanism.cpp	2013-07-19 01:29:27 +0000
@@ -40,13 +40,18 @@
         [&](std::shared_ptr<mf::Session> const& session) {
         if (session == focus_session)
         {
-            auto surface = focus_session->default_surface();
-            if (surface)
+            // We use a transaction to guarantee the surface will not be destroyed
+            // from a second thread before input focus is set.
+            focus_session->transaction([&]()
             {
-                surface->take_input_focus(input_targeter);
-                set_input_focus = true;
-            }
-            session->show();
+                auto surface = focus_session->default_surface();
+                if (surface)
+                    {
+                        surface->take_input_focus(input_targeter);
+                        set_input_focus = true;
+                    }
+                session->show();
+            });
         }
         else
         {
 
=== modified file 'tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-06-25 13:19:15 +0000
+++ tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-07-19 01:29:27 +0000
@@ -60,6 +60,15 @@
     MOCK_METHOD0(show, void());
 
     MOCK_METHOD3(configure_surface, int(mf::SurfaceId, MirSurfaceAttrib, int));
+
+    MOCK_METHOD0(mock_transaction_started, void());
+    MOCK_METHOD0(mock_transaction_finished, void());
+    void transaction(std::function<void()> const& body)
+    {
+        mock_transaction_started();
+        body();
+        mock_transaction_finished();
+    }
 };
 
 TEST(SingleVisibilityFocusMechanism, mechanism_sets_visibility)
@@ -114,8 +123,13 @@
     
     {
         InSequence seq;
+        EXPECT_CALL(app1, mock_transaction_started()).Times(1);
         EXPECT_CALL(mock_surface, take_input_focus(_)).Times(1);
+        EXPECT_CALL(app1, mock_transaction_finished()).Times(1);
         // When we have no default surface.
+        EXPECT_CALL(app1, mock_transaction_started()).Times(1);
+        EXPECT_CALL(app1, mock_transaction_finished()).Times(1);
+
         EXPECT_CALL(targeter, focus_cleared()).Times(1);
         // When we have no session.
         EXPECT_CALL(targeter, focus_cleared()).Times(1);