Mir

Merge lp:~afrantzis/mir/fix-1612012 into lp:mir

  1. fix-1612012
  2. Merge into development-branch
Proposed by Alexandros Frantzis
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 3656
Proposed branch: lp:~afrantzis/mir/fix-1612012
Merge into: lp:mir
Diff against target: 133 lines (+107/-0)
3 files modified
src/server/graphics/nested/display_buffer.cpp (+1/-0)
tests/unit-tests/platforms/nested/CMakeLists.txt (+1/-0)
tests/unit-tests/platforms/nested/test_nested_display_buffer.cpp (+105/-0)
To merge this branch: bzr merge lp:~afrantzis/mir/fix-1612012
Reviewer Review Type Date Requested Status
Cemil Azizoglu (community) Approve
Mir CI Bot continuous-integration Approve
Alan Griffiths Approve
Kevin DuBois (community) Approve
Review via email: mp+303242@code.launchpad.net

Commit message

nested: Fix race between event dispatch and DisplayBuffer destruction

Description of the change

nested: Fix race between event dispatch and DisplayBuffer destruction

Events could be dispatched from the HostSurface to the DisplayBuffer, while the latter was being destroyed, causing valgrind errors in the best case, crashes in the worst case.

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

FAILED: Continuous integration, rev:3655
https://mir-jenkins.ubuntu.com/job/mir-ci/1481/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1834/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1888
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1879
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1858
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1858/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/1858
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1858/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1858
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1858/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1858/console
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1858/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/1858
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1858/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/1858
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1858/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/1481/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/1560909

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:3655
https://mir-jenkins.ubuntu.com/job/mir-ci/1482/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/1835/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1889
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1880
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1880
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1880
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1859
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1859/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/1859
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1859/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1859/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1859
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1859/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/1859
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1859/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/1859
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1859/artifact/output/*zip*/output.zip

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

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

This time CI failure is https://bugs.launchpad.net/mir/+bug/1523621

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

LGTM

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

PASSED: Continuous integration, rev:3655
https://mir-jenkins.ubuntu.com/job/mir-ci/1484/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/1837
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/1891
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/1882
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/1882
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/1882
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/1861/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/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/1861/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/1861/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/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/1861/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/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/1861/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/1861
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/1861/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Cemil Azizoglu (cemil-azizoglu) wrote :

ok

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/display_buffer.cpp'
2--- src/server/graphics/nested/display_buffer.cpp 2016-06-10 13:50:29 +0000
3+++ src/server/graphics/nested/display_buffer.cpp 2016-08-18 11:17:25 +0000
4@@ -93,6 +93,7 @@
5
6 mgn::detail::DisplayBuffer::~DisplayBuffer() noexcept
7 {
8+ host_surface->set_event_handler(nullptr, nullptr);
9 }
10
11 void mgn::detail::DisplayBuffer::event_thunk(
12
13=== modified file 'tests/unit-tests/platforms/nested/CMakeLists.txt'
14--- tests/unit-tests/platforms/nested/CMakeLists.txt 2016-08-05 15:54:30 +0000
15+++ tests/unit-tests/platforms/nested/CMakeLists.txt 2016-08-18 11:17:25 +0000
16@@ -3,6 +3,7 @@
17 ${CMAKE_CURRENT_SOURCE_DIR}/test_nested_display.cpp
18 ${CMAKE_CURRENT_SOURCE_DIR}/mir_display_configuration_builder.cpp
19 ${CMAKE_CURRENT_SOURCE_DIR}/test_nested_cursor.cpp
20+ ${CMAKE_CURRENT_SOURCE_DIR}/test_nested_display_buffer.cpp
21 $<TARGET_OBJECTS:mir-test-doubles-udev>
22 ${MIR_PLATFORM_OBJECTS}
23 ${MIR_SERVER_OBJECTS}
24
25=== added file 'tests/unit-tests/platforms/nested/test_nested_display_buffer.cpp'
26--- tests/unit-tests/platforms/nested/test_nested_display_buffer.cpp 1970-01-01 00:00:00 +0000
27+++ tests/unit-tests/platforms/nested/test_nested_display_buffer.cpp 2016-08-18 11:17:25 +0000
28@@ -0,0 +1,105 @@
29+/*
30+ * Copyright © 2016 Canonical Ltd.
31+ *
32+ * This program is free software: you can redistribute it and/or modify
33+ * it under the terms of the GNU General Public License version 3 as
34+ * published by the Free Software Foundation.
35+ *
36+ * This program is distributed in the hope that it will be useful,
37+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
38+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
39+ * GNU General Public License for more details.
40+ *
41+ * You should have received a copy of the GNU General Public License
42+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
43+ *
44+ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
45+ */
46+
47+#include "src/server/graphics/nested/display_buffer.h"
48+
49+#include "mir/events/event_builders.h"
50+
51+#include "mir/test/doubles/mock_egl.h"
52+#include "mir/test/doubles/stub_gl_config.h"
53+#include "mir/test/doubles/stub_host_connection.h"
54+#include "mir/test/fake_shared.h"
55+
56+#include <gtest/gtest.h>
57+#include <gmock/gmock.h>
58+
59+namespace mgn = mir::graphics::nested;
60+namespace mgnd = mgn::detail;
61+namespace mt = mir::test;
62+namespace mtd = mir::test::doubles;
63+
64+namespace
65+{
66+
67+class EventHostSurface : public mgn::HostSurface
68+{
69+public:
70+ EGLNativeWindowType egl_native_window() override { return {}; }
71+
72+ void set_event_handler(mir_surface_event_callback cb, void* ctx) override
73+ {
74+ std::lock_guard<std::mutex> lock{event_mutex};
75+ event_handler = cb;
76+ event_context = ctx;
77+ }
78+
79+ void emit_input_event()
80+ {
81+ auto const ev = mir::events::make_event(
82+ MirInputDeviceId(), {}, std::vector<uint8_t>{},
83+ MirKeyboardAction(), 0, 0, MirInputEventModifiers());
84+
85+ std::lock_guard<std::mutex> lock{event_mutex};
86+ // Normally we shouldn't call external code under lock, but, for our
87+ // test use case, doing so is safe and simplifies the test code.
88+ if (event_handler)
89+ event_handler(nullptr, ev.get(), event_context);
90+ }
91+
92+private:
93+ std::mutex event_mutex;
94+ mir_surface_event_callback event_handler;
95+ void* event_context;
96+};
97+
98+struct NestedDisplayBuffer : testing::Test
99+{
100+ auto create_display_buffer()
101+ {
102+ return std::make_shared<mgnd::DisplayBuffer>(
103+ egl_display,
104+ mt::fake_shared(host_surface),
105+ mir::geometry::Rectangle{},
106+ MirPixelFormat{},
107+ std::make_shared<mtd::StubHostConnection>()
108+ );
109+ }
110+
111+ testing::NiceMock<mtd::MockEGL> mock_egl;
112+ mgnd::EGLDisplayHandle egl_display{nullptr, std::make_shared<mtd::StubGLConfig>()};
113+ EventHostSurface host_surface;
114+};
115+
116+}
117+
118+// Regression test for LP: #1612012
119+// This test is trying to reproduce a race, so it's not deterministic, but
120+// in practice the reproduction rate is very close to 100%.
121+TEST_F(NestedDisplayBuffer, event_dispatch_does_not_race_with_destruction)
122+{
123+ auto display_buffer = create_display_buffer();
124+ std::thread t{
125+ [&]
126+ {
127+ for (int i = 0; i < 100; ++i)
128+ host_surface.emit_input_event();
129+ }};
130+
131+ display_buffer.reset();
132+ t.join();
133+}

Subscribers

People subscribed via source and target branches