Mir

Merge lp:~afrantzis/mir/fix-1628828-nested-server-tests into lp:mir

Proposed by Alexandros Frantzis
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 3739
Proposed branch: lp:~afrantzis/mir/fix-1628828-nested-server-tests
Merge into: lp:mir
Diff against target: 56 lines (+9/-1)
2 files modified
src/server/graphics/nested/mir_client_host_connection.cpp (+6/-1)
src/server/graphics/nested/mir_client_host_connection.h (+3/-0)
To merge this branch: bzr merge lp:~afrantzis/mir/fix-1628828-nested-server-tests
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Chris Halse Rogers Approve
Mir CI Bot continuous-integration Approve
Kevin DuBois (community) Approve
Review via email: mp+307549@code.launchpad.net

Commit message

nested: Set the MirClientHostConnection callbacks (thread-)safely
(LP: #1628828)

Description of the change

nested: Set the MirClientHostConnection callbacks (thread-)safely

Without this protection we may change the callback while it is being called, leading to all sorts of amusing consequences (aka crashes).

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3737
https://mir-jenkins.ubuntu.com/job/mir-ci/1881/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2385/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2448
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2440
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2440
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2440
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2414/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2414
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2414/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2414
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2414/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2414
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2414/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2414
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2414/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2414
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2414/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1881/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

CI failure is the unrelated bug: https://bugs.launchpad.net/mir/+bug/1616312

Revision history for this message
Kevin DuBois (kdub) wrote :

lgtm

review: Approve
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3737
https://mir-jenkins.ubuntu.com/job/mir-ci/1882/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2386/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2449
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2441
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2441
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2441
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2415/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2415
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2415/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2415
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2415/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2415
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2415/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2415
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2415/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2415
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2415/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1882/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

> CI failure is the unrelated bug: https://bugs.launchpad.net/mir/+bug/1616312

... again.

Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3737
https://mir-jenkins.ubuntu.com/job/mir-ci/1887/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2391
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2454
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2446
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2446
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2446
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2420/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2420/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2420/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2420/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2420/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2420
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2420/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1887/rebuild

review: Approve (continuous-integration)
Revision history for this message
Chris Halse Rogers (raof) wrote :

Might be nice to use PosixRWMutex and std::lock_guard<>/std::shared_lock<> once https://code.launchpad.net/~raof/mir/shared-mutex/+merge/307499 lands, but this looks correct.

review: Approve
Revision history for this message
Chris Halse Rogers (raof) wrote :

Hm.

PosixRWMutex would also be more correct, because this code is still susceptible to crashes if the input_config_callback *itself* calls set_input_device_change_callback? With PosixRWMutex it'd deadlock, which is more noticable?

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

Cool.

I wonder if this has any implications for bug 1523621 too...

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/graphics/nested/mir_client_host_connection.cpp'
2--- src/server/graphics/nested/mir_client_host_connection.cpp 2016-09-22 16:27:13 +0000
3+++ src/server/graphics/nested/mir_client_host_connection.cpp 2016-10-04 11:59:56 +0000
4@@ -291,6 +291,7 @@
5 [](MirConnection* connection, void* context)
6 {
7 auto obj = static_cast<MirClientHostConnection*>(context);
8+ RecursiveReadLock lock{obj->input_config_callback_mutex};
9 if (obj->input_config_callback)
10 obj->input_config_callback(make_input_config(connection));
11 },
12@@ -453,17 +454,21 @@
13
14 void mgn::MirClientHostConnection::set_input_device_change_callback(std::function<void(UniqueInputConfig)> const& cb)
15 {
16+ RecursiveWriteLock lock{input_config_callback_mutex};
17 input_config_callback = cb;
18 }
19
20 void mgn::MirClientHostConnection::set_input_event_callback(std::function<void(MirEvent const&, mir::geometry::Rectangle const&)> const& cb)
21 {
22+ RecursiveWriteLock lock{event_callback_mutex};
23 event_callback = cb;
24 }
25
26 void mgn::MirClientHostConnection::emit_input_event(MirEvent const& event, mir::geometry::Rectangle const& source_frame)
27 {
28- event_callback(event, source_frame);
29+ RecursiveReadLock lock{event_callback_mutex};
30+ if (event_callback)
31+ event_callback(event, source_frame);
32 }
33
34 std::unique_ptr<mgn::HostStream> mgn::MirClientHostConnection::create_stream(
35
36=== modified file 'src/server/graphics/nested/mir_client_host_connection.h'
37--- src/server/graphics/nested/mir_client_host_connection.h 2016-09-22 16:27:13 +0000
38+++ src/server/graphics/nested/mir_client_host_connection.h 2016-10-04 11:59:56 +0000
39@@ -25,6 +25,7 @@
40 #include "mir/geometry/size.h"
41 #include "mir/geometry/displacement.h"
42 #include "mir/graphics/cursor_image.h"
43+#include "mir/recursive_read_write_mutex.h"
44
45 #include <string>
46 #include <vector>
47@@ -96,7 +97,9 @@
48
49 std::vector<HostSurface*> surfaces;
50
51+ RecursiveReadWriteMutex input_config_callback_mutex;
52 std::function<void(UniqueInputConfig)> input_config_callback;
53+ RecursiveReadWriteMutex event_callback_mutex;
54 std::function<void(MirEvent const&, mir::geometry::Rectangle const&)> event_callback;
55
56 struct NestedCursorImage : graphics::CursorImage

Subscribers

People subscribed via source and target branches