Merge lp:~afrantzis/mir/fix-surface-visibility-event-test into lp:mir

[Alexandros Frantzis]

tests: Fix race conditions affecting MirSurfaceVisibilityEvent tests

tests: Fix race conditions affecting MirSurfaceVisibilityEvent tests

Three problems/races were identified and fixed:

1. WindowManagerPolicy function calls are normally serialized using locks in BasicWindowManager. However, msh::CanonicalWindowManagerPolicy::select_active_surface() could be called unlocked through SurfaceReadyObserver, leading to various races and subsequent failures. This was fixed by serializing select_active_surface() with an internal (recursive) lock. There are other solutions, which involve changing the API, but I didn't want to interfere with the API without Alan's feedback. This solution is good enough for now.

2. For reasons unknown, googlemock doesn't fully clear the expectations with VerifyAndClearExpectations(), causing intermittent failures for visibility events received during surface teardown. This problem was worked around by unsetting the client surface event handler before surface destruction.

3. Waiting for a surface to become visible is not a sufficient condition for testing that the surface has reached a steady state as needed by the tests. We also need to wait for the surface to get the focus. This MP fixes the problem by waiting for both visibility and focus state when creating a surface. We use a spin_wait instead of monitoring surface events to workaround the same googlemock problem mentioned in (2).

[Mir CI Bot (mir-ci-bot)]

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

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

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

LGTM

review: Approve

[Andreas Pokorny (andreas-pokorny)]

lgtm

review: Approve

=== modified file 'src/include/server/mir/shell/canonical_window_manager.h'
--- src/include/server/mir/shell/canonical_window_manager.h	2016-05-03 06:55:25 +0000
+++ src/include/server/mir/shell/canonical_window_manager.h	2016-06-22 15:26:47 +0000
@@ -106,7 +106,6 @@
 
     geometry::Rectangle display_area;
     geometry::Point old_cursor{};
-    std::weak_ptr<scene::Surface> active_surface_;
     using FullscreenSurfaces = std::set<std::weak_ptr<scene::Surface>, std::owner_less<std::weak_ptr<scene::Surface>>>;
 
     FullscreenSurfaces fullscreen_surfaces;
@@ -114,6 +113,9 @@
     bool resizing = false;
     bool left_resize = false;
     bool top_resize = false;
+
+    std::recursive_mutex active_surface_mutex;
+    std::weak_ptr<scene::Surface> active_surface_;
 };
 
 using CanonicalWindowManager = WindowManagerConstructor<CanonicalWindowManagerPolicy>;

=== modified file 'src/server/shell/canonical_window_manager.cpp'
--- src/server/shell/canonical_window_manager.cpp	2016-06-07 19:23:52 +0000
+++ src/server/shell/canonical_window_manager.cpp	2016-06-22 15:26:47 +0000
@@ -757,6 +757,8 @@
 
 void msh::CanonicalWindowManagerPolicy::select_active_surface(std::shared_ptr<ms::Surface> const& surface)
 {
+    std::lock_guard<std::recursive_mutex> lock{active_surface_mutex};
+
     if (!surface)
     {
         if (active_surface_.lock())

=== modified file 'tests/acceptance-tests/test_client_surface_visibility.cpp'
--- tests/acceptance-tests/test_client_surface_visibility.cpp	2016-05-03 06:55:25 +0000
+++ tests/acceptance-tests/test_client_surface_visibility.cpp	2016-06-22 15:26:47 +0000
@@ -25,6 +25,7 @@
 
 #include "mir_test_framework/connected_client_with_a_surface.h"
 #include "mir/test/signal.h"
+#include "mir/test/spin_wait.h"
 
 #include <mutex>
 #include <condition_variable>
@@ -81,6 +82,10 @@
     MOCK_METHOD2(handle, void(MirSurface*,MirSurfaceVisibility));
 };
 
+void null_event_callback(MirSurface*, MirEvent const*, void*)
+{
+}
+
 void event_callback(MirSurface* surface, MirEvent const* event, void* ctx)
 {
     if (mir_event_get_type(event) != mir_event_type_surface)
@@ -96,12 +101,12 @@
         static_cast<MirSurfaceVisibility>(mir_surface_event_get_attribute_value(sev)));
 }
 
-MirSurface* create_surface(MirConnection* connection, geom::Size size,
+MirSurface* create_surface(MirConnection* connection, const char* name, geom::Size size,
     testing::NiceMock<MockVisibilityCallback>& mock_callback)
 {
     auto const spec = mir_connection_create_spec_for_normal_surface(
         connection, size.width.as_int(), size.height.as_int(), mir_pixel_format_bgr_888);
-    mir_surface_spec_set_name(spec, "ConnectedClientWithASurfaceFixtureSurface");
+    mir_surface_spec_set_name(spec, name);
     mir_surface_spec_set_buffer_usage(spec, mir_buffer_usage_hardware);
     mir_surface_spec_set_event_handler(spec, &event_callback, &mock_callback);
     auto surface = mir_surface_create_sync(spec);
@@ -111,14 +116,15 @@
 
 struct Surface
 {
-    Surface(MirConnection* connection, geom::Size size) :
-        surface(create_surface(connection, size, callback))
+    Surface(MirConnection* connection, char const* name, geom::Size size) :
+        surface(create_surface(connection, name, size, callback))
     {
-        wait_for_visible();
+        wait_for_visible_and_focused();
     }
 
     ~Surface()
     {
+        mir_surface_set_event_handler(surface, null_event_callback, nullptr);
         mir_surface_release_sync(surface);
     }
 
@@ -146,7 +152,7 @@
     }
 
 private:
-    void wait_for_visible()
+    void wait_for_visible_and_focused()
     {
         expect_surface_visibility_event_after(
             mir_surface_visibility_exposed,
@@ -154,6 +160,12 @@
             {
                 mir_buffer_stream_swap_buffers_sync(mir_surface_get_buffer_stream(surface));
             });
+
+        // GMock is behaving strangely, checking expectations after they
+        // have been cleared, so we use spin_wait() instead.
+        mt::spin_wait_for_condition_or_timeout(
+            [this] { return mir_surface_get_focus(surface) == mir_surface_focused; },
+            std::chrono::seconds{2});
     }
 
     testing::NiceMock<MockVisibilityCallback> callback;
@@ -172,7 +184,7 @@
             });
 
         mtf::ConnectedClientHeadlessServer::SetUp();
-        surface = std::make_unique<Surface>(connection, small_size);
+        surface = std::make_unique<Surface>(connection, "small", small_size);
     }
 
     void TearDown() override
@@ -184,7 +196,7 @@
 
     void create_larger_surface_on_top()
     {
-        second_surface = std::make_unique<Surface>(connection, large_size);
+        second_surface = std::make_unique<Surface>(connection, "large", large_size);
         shell.lock()->raise(1);
     }