Merge lp:~albaguirre/mir/fix-1427976-take2 into lp:mir

[Alberto Aguirre]

Avoid unlocking a mutex in a thread that does not own it.

SessionMediator unlocks its session_mutex before invoking rpc callbacks.
This becomes cumbersome when SessionMediator needs to call Surface::swap_buffers,
as the completion function may be executed in an entirely different thread.

The session_mutex should only be unlocked if the completion function is invoked during Surface::swap_buffers.

Avoid unlocking a mutex in a thread that does not own it.

SessionMediator unlocks its session_mutex before invoking rpc callbacks.
This becomes cumbersome when SessionMediator needs to call Surface::swap_buffers,
as the completion function may be executed in an entirely different thread.

The session_mutex should only be unlocked if the completion function is invoked during Surface::swap_buffers.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2370
http://jenkins.qa.ubuntu.com/job/mir-ci/3146/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1534
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1533
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1488
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1143
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1143/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1488
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1488/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4507
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18603

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Ugly, but makes sense in the context of the way the code is currently.

So let's land this, but keep the technical debt around this area on the radar.

review: Approve

[lp:~albaguirre/mir/fix-1427976-take2]

2371. By Alberto Aguirre on 2015-03-06

Make SurfaceTracker thread safe to avoid locking session_mutex.

Locking the session_mutex during the completion callback when executing on a separate
thread can cause lock ordering issues.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2371
http://jenkins.qa.ubuntu.com/job/mir-ci/3155/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1548
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1547
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1502
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1152
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1152/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1502
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1502/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4520
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18632

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

+1 for also fixing a potential race in mf::SessionMediator::exchange_buffer() (which previously called SurfaceTracker unlocked).

review: Approve

[Kevin DuBois (kdub)]

> Ugly, but makes sense in the context of the way the code is currently.
>
> So let's land this, but keep the technical debt around this area on the radar.

maybe put it on the backlog?

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2015-02-18 05:27:28 +0000
+++ src/server/frontend/session_mediator.cpp	2015-03-06 15:31:16 +0000
@@ -60,6 +60,7 @@
 #include <boost/throw_exception.hpp>
 
 #include <mutex>
+#include <thread>
 #include <functional>
 #include <cstring>
 
@@ -151,17 +152,27 @@
 void mf::SessionMediator::advance_buffer(
     SurfaceId surf_id,
     Surface& surface,
+    graphics::Buffer* old_buffer,
+    std::unique_lock<std::mutex>& lock,
     std::function<void(graphics::Buffer*, graphics::BufferIpcMsgType)> complete)
 {
-    auto client_buffer = surface_tracker.last_buffer(surf_id);
+    auto const tid = std::this_thread::get_id();
+
     surface.swap_buffers(
-        client_buffer, 
-        [this, surf_id, complete](mg::Buffer* new_buffer)
+        old_buffer,
+        // Note: We assume that the lambda will be executed within swap_buffers
+        // (in which case the lock reference is valid) or in a different thread
+        // altogether (in which case the dangling reference is not accessed)
+        [this, tid, &lock, surf_id, complete](mg::Buffer* new_buffer)
         {
+            if (tid == std::this_thread::get_id())
+                lock.unlock();
+
             if (surface_tracker.track_buffer(surf_id, new_buffer))
                 complete(new_buffer, mg::BufferIpcMsgType::update_msg);
             else
                 complete(new_buffer, mg::BufferIpcMsgType::full_msg);
+
         });
 }
 
@@ -172,7 +183,7 @@
     google::protobuf::Closure* done)
 {
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    std::unique_lock<std::mutex> lock{session_mutex};
 
     auto const session = weak_session.lock();
 
@@ -242,12 +253,10 @@
         setting->set_ivalue(shell->get_surface_attribute(session, surf_id, static_cast<MirSurfaceAttrib>(i)));
     }
 
-    advance_buffer(surf_id, *surface,
-        [lock, this, &surf_id, response, done, session]
+    advance_buffer(surf_id, *surface, surface_tracker.last_buffer(surf_id), lock,
+        [this, surf_id, response, done, session]
         (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
         {
-            lock->unlock();
-
             response->mutable_buffer_stream()->mutable_id()->set_value(
                surf_id.as_value());
             pack_protobuf_buffer(*response->mutable_buffer_stream()->mutable_buffer(),
@@ -271,7 +280,7 @@
 {
     SurfaceId const surf_id{request->value()};
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    std::unique_lock<std::mutex> lock{session_mutex};
 
     auto const session = weak_session.lock();
 
@@ -281,15 +290,11 @@
     report->session_next_buffer_called(session->name());
 
     auto surface = session->get_surface(surf_id);
-
-    advance_buffer(surf_id, *surface,
-        [lock, this, response, done, session]
+    advance_buffer(surf_id, *surface, surface_tracker.last_buffer(surf_id), lock,
+        [this, response, done]
         (graphics::Buffer* client_buffer, graphics::BufferIpcMsgType msg_type)
         {
-            lock->unlock();
-
             pack_protobuf_buffer(*response, client_buffer, msg_type);
-
             done->Run();
         });
 }
@@ -306,7 +311,7 @@
     mfd::ProtobufBufferPacker request_msg{const_cast<mir::protobuf::Buffer*>(&request->buffer())};
     ipc_operations->unpack_buffer(request_msg, *surface_tracker.last_buffer(surface_id));
 
-    auto const lock = std::make_shared<std::unique_lock<std::mutex>>(session_mutex);
+    std::unique_lock<std::mutex> lock{session_mutex};
     auto const session = weak_session.lock();
     if (!session)
         BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
@@ -314,17 +319,11 @@
     report->session_exchange_buffer_called(session->name());
 
     auto const& surface = session->get_surface(surface_id);
-    surface->swap_buffers(
-        surface_tracker.buffer_from(buffer_id),
-        [this, surface_id, lock, response, done](mg::Buffer* new_buffer)
+    advance_buffer(surface_id, *surface, surface_tracker.buffer_from(buffer_id), lock,
+        [this, response, done]
+        (graphics::Buffer* new_buffer, graphics::BufferIpcMsgType msg_type)
         {
-            lock->unlock();
-
-            if (surface_tracker.track_buffer(surface_id, new_buffer))
-                pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::update_msg);
-            else
-                pack_protobuf_buffer(*response, new_buffer, mg::BufferIpcMsgType::full_msg);
-
+            pack_protobuf_buffer(*response, new_buffer, msg_type);
             done->Run();
         });
 }

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2015-01-21 07:34:50 +0000
+++ src/server/frontend/session_mediator.h	2015-03-06 15:31:16 +0000
@@ -192,6 +192,8 @@
     void advance_buffer(
         SurfaceId surf_id,
         Surface& surface,
+        graphics::Buffer* old_buffer,
+        std::unique_lock<std::mutex>& lock,
         std::function<void(graphics::Buffer*, graphics::BufferIpcMsgType)> complete);
 
     virtual std::function<void(std::shared_ptr<Session> const&)> prompt_session_connect_handler() const;

=== modified file 'src/server/frontend/surface_tracker.cpp'
--- src/server/frontend/surface_tracker.cpp	2015-01-21 07:34:50 +0000
+++ src/server/frontend/surface_tracker.cpp	2015-03-06 15:31:16 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2015 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
@@ -17,8 +17,11 @@
  */
 
 #include "surface_tracker.h"
+#include "client_buffer_tracker.h"
+
 #include "mir/graphics/buffer.h"
 #include "mir/graphics/buffer_id.h"
+
 #include <boost/throw_exception.hpp>
 #include <stdexcept>
 
@@ -32,6 +35,7 @@
 
 bool mf::SurfaceTracker::track_buffer(SurfaceId surface_id, mg::Buffer* buffer)
 {
+    std::lock_guard<decltype(mutex)> lock{mutex};
     auto& tracker = client_buffer_tracker[surface_id];
     if (!tracker)
         tracker = std::make_shared<ClientBufferTracker>(client_cache_size);
@@ -53,6 +57,7 @@
 
 void mf::SurfaceTracker::remove_surface(SurfaceId surface_id)
 {
+    std::lock_guard<decltype(mutex)> lock{mutex};
     auto it = client_buffer_tracker.find(surface_id);
     if (it != client_buffer_tracker.end())
         client_buffer_tracker.erase(it);
@@ -64,6 +69,7 @@
 
 mg::Buffer* mf::SurfaceTracker::last_buffer(SurfaceId surface_id) const
 {
+    std::lock_guard<decltype(mutex)> lock{mutex};
     auto it = client_buffer_resource.find(surface_id);
     if (it != client_buffer_resource.end())
         return it->second;
@@ -74,6 +80,7 @@
 
 mg::Buffer* mf::SurfaceTracker::buffer_from(mg::BufferID buffer_id) const
 {
+    std::lock_guard<decltype(mutex)> lock{mutex};
     for (auto const& tracker : client_buffer_tracker)
     {
         auto buffer = tracker.second->buffer_from(buffer_id);

=== modified file 'src/server/frontend/surface_tracker.h'
--- src/server/frontend/surface_tracker.h	2015-03-04 11:06:52 +0000
+++ src/server/frontend/surface_tracker.h	2015-03-06 15:31:16 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2013 Canonical Ltd.
+ * Copyright © 2013-2015 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
@@ -20,9 +20,11 @@
 #define MIR_FRONTEND_SURFACE_TRACKER_H_
 
 #include "mir/frontend/surface_id.h"
-#include "client_buffer_tracker.h"
+#include "mir/graphics/buffer_id.h"
+
 #include <unordered_map>
 #include <memory>
+#include <mutex>
 
 namespace mir
 {
@@ -32,7 +34,7 @@
 }
 namespace frontend
 {
-
+class ClientBufferTracker;
 class SurfaceTracker
 {
 public:
@@ -64,6 +66,7 @@
     /* accesses the last buffer given to track_buffer() for the given SurfaceId */
     graphics::Buffer* last_buffer(SurfaceId) const;
 private:
+    mutable std::mutex mutex;
     std::unordered_map<SurfaceId, graphics::Buffer*> client_buffer_resource;
 };