Merge lp:~robertcarr/mir/fix-focus-races into lp:~mir-team/mir/trunk

[Robert Carr]

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.

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.

[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

[lp:~robertcarr/mir/fix-focus-races]

1055. By Robert Carr on 2013-09-09

Default ApplicationSession constructor

[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.

[lp:~robertcarr/mir/fix-focus-races]

1056. By Robert Carr on 2013-09-19

Mutex

1057. By Robert Carr on 2013-09-19

Recursive mutex

[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)

[lp:~robertcarr/mir/fix-focus-races]

1058. By Robert Carr on 2013-09-19

Merge trunk

[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

Unmerged revisions

1058. By Robert Carr on 2013-09-19

Merge trunk

1057. By Robert Carr on 2013-09-19

Recursive mutex

1056. By Robert Carr on 2013-09-19

Mutex

1055. By Robert Carr on 2013-09-09

Default ApplicationSession constructor

1054. By Robert Carr on 2013-09-06

Do not destroy underlying surfaces until the msh::Surface d'tor

1053. By Eleni Maria Stea on 2013-09-05

graphics: Pull in Eleni's changes to get the DRM fd to init GBM from the host Mir instance.

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

1052. By PS Jenkins bot on 2013-09-04

Releasing 0.0.10+13.10.20130904-0ubuntu1 (revision 1051 from lp:mir).

Approved by PS Jenkins bot.

1051. By Kevin DuBois on 2013-09-04

fix lp:1220441 (a test for android display ID was not updated). Fixes: https://bugs.launchpad.net/bugs/1220441.

Approved by Daniel van Vugt, PS Jenkins bot, Robert Ancell.

1050. By PS Jenkins bot on 2013-09-03

Releasing 0.0.10+13.10.20130903-0ubuntu1 (revision 1049 from lp:mir).

Approved by PS Jenkins bot.

1049. By Daniel van Vugt on 2013-09-03

SwitchingBundle: Simplify and clarify guarantees that compositor_acquire
always has a buffer to return without blocking or throwing an exception.

The trade-off is that to enforce the guarantee we need to permanently
reserve one buffer for compositing. This reduces the flexibility of
SwitchingBundle a little, such that minimum nbuffers is now 2.

This was originally requested by Alexandros, as the potential throw concerned
him. Although, it was logically guaranteed to never happen for other reasons.

The second reason for doing this is to eliminate recycling logic, which
while safe and correct, was quite confusing. So this change further proves
that that logic (now removed) is not to blame for frame ordering bugs.
.

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

=== 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-19 16:20:57 +0000
@@ -46,7 +46,7 @@
         std::shared_ptr<SessionListener> const& session_listener,
         std::shared_ptr<frontend::EventSink> const& sink);
 
-    ~ApplicationSession();
+    ~ApplicationSession() = default;
 
     frontend::SurfaceId create_surface(shell::SurfaceCreationParameters const& params);
     void destroy_surface(frontend::SurfaceId surface);

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-08-28 03:41:48 +0000
+++ include/server/mir/shell/surface.h	2013-09-19 16:20:57 +0000
@@ -28,6 +28,7 @@
 #include "mir_toolkit/common.h"
 
 #include <string>
+#include <mutex>
 
 namespace mir
 {
@@ -104,6 +105,8 @@
 
     MirSurfaceType type_value;
     MirSurfaceState state_value;
+    
+    mutable std::recursive_mutex surface_lock;
 };
 }
 }

=== 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-19 16:20:57 +0000
@@ -50,15 +50,6 @@
     assert(surface_factory);
 }
 
-msh::ApplicationSession::~ApplicationSession()
-{
-    std::unique_lock<std::mutex> lock(surfaces_mutex);
-    for (auto const& pair_id_surface : surfaces)
-    {
-        pair_id_surface.second->destroy();
-    }
-}
-
 mf::SurfaceId msh::ApplicationSession::next_id()
 {
     return mf::SurfaceId(next_surface_id.fetch_add(1));
@@ -116,7 +107,6 @@
     
     session_listener->destroying_surface(*this, p->second);
 
-    p->second->destroy();
     surfaces.erase(p);
 }
 

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-08-28 03:41:48 +0000
+++ src/server/shell/surface.cpp	2013-09-19 16:20:57 +0000
@@ -58,14 +58,17 @@
 
 msh::Surface::~Surface() noexcept
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (surface.lock())
     {
-        destroy();
+        builder->destroy_surface(surface);
     }
 }
 
 void msh::Surface::hide()
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     if (auto const& s = surface.lock())
     {
         s->set_hidden(true);
@@ -78,6 +81,7 @@
 
 void msh::Surface::show()
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     if (auto const& s = surface.lock())
     {
         s->set_hidden(false);
@@ -90,11 +94,15 @@
 
 void msh::Surface::destroy()
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     builder->destroy_surface(surface);
 }
 
 void msh::Surface::force_requests_to_complete()
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         s->force_requests_to_complete();
@@ -103,6 +111,8 @@
 
 mir::geometry::Size msh::Surface::size() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->size();
@@ -115,6 +125,8 @@
 
 void msh::Surface::move_to(geometry::Point const& p)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         s->move_to(p);
@@ -127,6 +139,8 @@
 
 mir::geometry::Point msh::Surface::top_left() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->top_left();
@@ -139,6 +153,8 @@
 
 std::string msh::Surface::name() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->name();
@@ -151,6 +167,8 @@
 
 mir::geometry::PixelFormat msh::Surface::pixel_format() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->pixel_format();
@@ -163,6 +181,8 @@
 
 std::shared_ptr<mg::Buffer> msh::Surface::advance_client_buffer()
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->advance_client_buffer();
@@ -175,6 +195,8 @@
 
 void msh::Surface::allow_framedropping(bool allow)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         s->allow_framedropping(allow);
@@ -184,6 +206,8 @@
 void msh::Surface::with_most_recent_buffer_do(
     std::function<void(mg::Buffer&)> const& exec)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         auto buf = s->snapshot_buffer();
@@ -197,6 +221,8 @@
 
 bool msh::Surface::supports_input() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+    
     if (auto const& s = surface.lock())
     {
         return s->supports_input();
@@ -209,6 +235,8 @@
 
 int msh::Surface::client_input_fd() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     if (auto const& s = surface.lock())
     {
         return s->client_input_fd();
@@ -221,6 +249,8 @@
 
 int msh::Surface::configure(MirSurfaceAttrib attrib, int value)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     int result = 0;
     bool allow_dropping = false;
     /*
@@ -263,11 +293,14 @@
 
 MirSurfaceType msh::Surface::type() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
+
     return type_value;
 }
 
 bool msh::Surface::set_type(MirSurfaceType t)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     bool valid = false;
 
     if (t >= 0 && t < mir_surface_type_enum_max_)
@@ -281,11 +314,13 @@
 
 MirSurfaceState msh::Surface::state() const
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     return state_value;
 }
 
 bool msh::Surface::set_state(MirSurfaceState s)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     bool valid = false;
 
     if (s > mir_surface_state_unknown &&
@@ -302,6 +337,7 @@
 
 void msh::Surface::notify_change(MirSurfaceAttrib attrib, int value)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     MirEvent e;
 
     // This memset is not really required. However it does avoid some
@@ -319,6 +355,7 @@
 
 void msh::Surface::take_input_focus(std::shared_ptr<msh::InputTargeter> const& targeter)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     auto s = surface.lock();
     if (s)
         targeter->focus_changed(s->input_channel());
@@ -328,6 +365,7 @@
 
 void msh::Surface::set_input_region(std::vector<geom::Rectangle> const& region)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     if (auto const& s = surface.lock())
     {
         s->set_input_region(region);
@@ -340,6 +378,7 @@
 
 void msh::Surface::raise(std::shared_ptr<msh::SurfaceController> const& controller)
 {
+    std::unique_lock<std::recursive_mutex> lg(surface_lock);
     if (auto const& s = surface.lock())
     {
         controller->raise(s);

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-09-19 16:20:57 +0000
@@ -65,7 +65,6 @@
     ON_CALL(surface_factory, create_surface(_,_,_,_)).WillByDefault(Return(mock_surface));
 
     EXPECT_CALL(surface_factory, create_surface(_, _, _, _));
-    EXPECT_CALL(*mock_surface, destroy());
     
     mtd::MockSessionListener listener;
     EXPECT_CALL(listener, surface_created(_, _)).Times(1);
@@ -142,7 +141,6 @@
         InSequence seq;
         EXPECT_CALL(*mock_surface, hide()).Times(1);
         EXPECT_CALL(*mock_surface, show()).Times(1);
-        EXPECT_CALL(*mock_surface, destroy()).Times(1);
     }
 
     msh::SurfaceCreationParameters params;