Merge lp:~afrantzis/mir/fix-1477467-callback-before-wait-handle into lp:mir

[Alexandros Frantzis]

client: Ensure the callback for mir_buffer_stream_swap_buffers is called before the corresponding MirWaitHandle is signaled

client: Ensure the callback for mir_buffer_stream_swap_buffers is called before the corresponding MirWaitHandle is signaled

This MP also adds acceptance tests to check this behavior in a few other callbacks too. Since the acceptance tests don't have access to mirclient internal code, they depend on a small artificial delay to trigger the desired effect/race. Although not ideal, this is good enough in practice.

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Kevin DuBois (kdub)]

eh, still +1, but the bug seems to be covered well enough from the acceptance test, and the unit tests and the addition to WaitHandle seem less valuable

[Alexandros Frantzis (afrantzis)]

> eh, still +1, but the bug seems to be covered well enough from the acceptance test,
> and the unit tests and the addition to WaitHandle seem less valuable

I added the unit tests because it was not easy to test the fix for the buffer_unavailable() path in the acceptance tests. Since I added the buffer_unavailable unit test I also added the buffer_available() test for completeness.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2772
http://jenkins.qa.ubuntu.com/job/mir-ci/4403/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3281
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/134
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/806
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3229
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/552
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/552/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3229
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3229/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6028
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22085

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm not convinced by the acceptance tests, but the unit tests seem adequate.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1692/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3292
    FAILURE: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/145/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/817
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3240/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/245
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/245/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3240
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3240/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6039/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22106

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/client/buffer_stream.cpp'
--- src/client/buffer_stream.cpp	2015-07-16 07:03:19 +0000
+++ src/client/buffer_stream.cpp	2015-07-23 11:00:27 +0000
@@ -424,9 +424,9 @@
     if (on_incoming_buffer)
     {
         process_buffer(buffer, lock);
+        on_incoming_buffer();
+        on_incoming_buffer = std::function<void()>{};
         next_buffer_wait_handle.result_received();
-        on_incoming_buffer();
-        on_incoming_buffer = std::function<void()>{};
     }
     else
     {
@@ -438,10 +438,10 @@
 {
     std::unique_lock<decltype(mutex)> lock(mutex);
     server_connection_lost = true;
-    next_buffer_wait_handle.result_received();
     if (on_incoming_buffer)
     {
         on_incoming_buffer();
         on_incoming_buffer = std::function<void()>{};
     }
+    next_buffer_wait_handle.result_received();
 }

=== modified file 'src/client/mir_wait_handle.cpp'
--- src/client/mir_wait_handle.cpp	2015-02-22 07:46:25 +0000
+++ src/client/mir_wait_handle.cpp	2015-07-23 11:00:27 +0000
@@ -74,3 +74,9 @@
     --expecting;
 }
 
+bool MirWaitHandle::has_result()
+{
+    std::lock_guard<std::mutex> lock(guard);
+
+    return received > 0;
+}

=== modified file 'src/client/mir_wait_handle.h'
--- src/client/mir_wait_handle.h	2013-10-03 03:44:08 +0000
+++ src/client/mir_wait_handle.h	2015-07-23 11:00:27 +0000
@@ -40,6 +40,8 @@
     void wait_for_one();
     void wait_for_pending(std::chrono::milliseconds limit);
 
+    bool has_result();
+
 private:
     std::mutex guard;
     std::condition_variable wait_condition;

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2015-07-23 02:39:20 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2015-07-23 11:00:27 +0000
@@ -13,6 +13,7 @@
   test_client_header_version.cpp
   test_client_input.cpp
   test_client_library.cpp
+  test_client_library_callbacks.cpp
   test_client_library_errors.cpp
   test_client_library_old.cpp
   test_client_surface_events.cpp

=== added file 'tests/acceptance-tests/test_client_library_callbacks.cpp'
--- tests/acceptance-tests/test_client_library_callbacks.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_client_library_callbacks.cpp	2015-07-23 11:00:27 +0000
@@ -0,0 +1,180 @@
+/*
+ * Copyright © 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
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alexandros Frantzis <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include "mir_test_framework/headless_in_process_server.h"
+#include "mir_test_framework/using_stub_client_platform.h"
+#include "mir_test_framework/any_surface.h"
+#include "mir/test/spin_wait.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <atomic>
+#include <chrono>
+#include <thread>
+
+namespace mt = mir::test;
+namespace mtf = mir_test_framework;
+
+using namespace std::chrono_literals;
+
+namespace
+{
+
+struct ClientLibraryCallbacks : mtf::HeadlessInProcessServer
+{
+    std::atomic<MirConnection*> connection{nullptr};
+    std::atomic<MirSurface*> surface{nullptr};
+    std::atomic<int> buffers{0};
+
+    void TearDown() override
+    {
+        if (surface)
+            mir_surface_release_sync(surface);
+        if (connection)
+            mir_connection_release(connection);
+
+        mtf::HeadlessInProcessServer::TearDown();
+    }
+
+    static void connection_callback(MirConnection* connection, void* context)
+    {
+        auto const config = reinterpret_cast<ClientLibraryCallbacks*>(context);
+        config->connected(connection);
+    }
+
+    static void create_surface_callback(MirSurface* surface, void* context)
+    {
+        auto const config = reinterpret_cast<ClientLibraryCallbacks*>(context);
+        config->surface_created(surface);
+    }
+
+    static void next_buffer_callback(MirBufferStream* bs, void* context)
+    {
+        auto const config = reinterpret_cast<ClientLibraryCallbacks*>(context);
+        config->next_buffer(bs);
+    }
+
+    static void release_surface_callback(MirSurface* surface, void* context)
+    {
+        auto const config = reinterpret_cast<ClientLibraryCallbacks*>(context);
+        config->surface_released(surface);
+    }
+
+    virtual void connected(MirConnection* conn)
+    {
+        std::this_thread::sleep_for(10ms);
+        connection = conn;
+    }
+
+    virtual void surface_created(MirSurface* new_surface)
+    {
+        std::this_thread::sleep_for(10ms);
+        surface = new_surface;
+    }
+
+    virtual void next_buffer(MirBufferStream*)
+    {
+        std::this_thread::sleep_for(10ms);
+        ++buffers;
+    }
+
+    void surface_released(MirSurface*)
+    {
+        std::this_thread::sleep_for(10ms);
+        surface = nullptr;
+    }
+
+    mtf::UsingStubClientPlatform using_stub_client_platform;
+};
+
+}
+
+using namespace testing;
+
+TEST_F(ClientLibraryCallbacks, connect_callback_is_called_before_wait_handler_has_result)
+{
+    auto const wh = mir_connect(
+        new_connection().c_str(), __PRETTY_FUNCTION__,
+        connection_callback, this);
+    mir_wait_for(wh);
+
+    EXPECT_THAT(connection.load(), NotNull());
+
+    // Even if the test fails, wait for object to become ready so we can
+    // tear down properly
+    mt::spin_wait_for_condition_or_timeout(
+        [this] { return connection != nullptr; },
+        3s);
+}
+
+TEST_F(ClientLibraryCallbacks, create_surface_callback_is_called_before_wait_handler_has_result)
+{
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+
+    auto const spec = mir_connection_create_spec_for_normal_surface(
+        connection, 100, 100, mir_pixel_format_argb_8888);
+    auto const wh = mir_surface_create(spec, create_surface_callback, this);
+    mir_surface_spec_release(spec);
+    mir_wait_for(wh);
+
+    EXPECT_THAT(surface.load(), NotNull());
+
+    // Even if the test fails, wait for object to become ready so we can
+    // tear down properly
+    mt::spin_wait_for_condition_or_timeout(
+        [this] { return surface != nullptr; },
+        3s);
+}
+
+TEST_F(ClientLibraryCallbacks, swap_buffers_callback_is_called_before_wait_handler_has_result)
+{
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+    surface = mtf::make_any_surface(connection);
+
+    auto const wh = mir_buffer_stream_swap_buffers(
+        mir_surface_get_buffer_stream(surface), next_buffer_callback, this);
+    mir_wait_for(wh);
+
+    EXPECT_THAT(buffers, Eq(1));
+
+    // Even if the test fails, wait for object to become ready so we can
+    // tear down properly
+    mt::spin_wait_for_condition_or_timeout(
+        [this] { return buffers == 1; },
+        3s);
+}
+
+TEST_F(ClientLibraryCallbacks, release_surface_callback_is_called_before_wait_handler_has_result)
+{
+    connection = mir_connect_sync(new_connection().c_str(), __PRETTY_FUNCTION__);
+    surface = mtf::make_any_surface(connection);
+
+    auto const wh = mir_surface_release(surface, release_surface_callback, this);
+    mir_wait_for(wh);
+
+    EXPECT_THAT(surface.load(), IsNull());
+
+    // Even if the test fails, wait for object to become ready so we can
+    // tear down properly
+    mt::spin_wait_for_condition_or_timeout(
+        [this] { return surface == nullptr; },
+        3s);
+}

=== modified file 'tests/unit-tests/client/test_client_buffer_stream.cpp'
--- tests/unit-tests/client/test_client_buffer_stream.cpp	2015-07-16 08:13:28 +0000
+++ tests/unit-tests/client/test_client_buffer_stream.cpp	2015-07-23 11:00:27 +0000
@@ -530,3 +530,53 @@
         bs->request_and_wait_for_next_buffer();
     }, std::runtime_error);
 }
+
+TEST_F(ClientBufferStreamTest, invokes_callback_on_buffer_available_before_wait_handle_has_result)
+{
+    using namespace ::testing;
+
+    MirWaitHandle* wh{nullptr};
+    bool wait_handle_has_result_in_callback = false;
+
+    EXPECT_CALL(mock_protobuf_server, submit_buffer(_,_,_,_))
+        .WillOnce(RunProtobufClosure());
+
+    auto const protobuf_bs = a_protobuf_buffer_stream(
+        default_pixel_format, default_buffer_usage, a_buffer_package());
+    auto const bs = make_buffer_stream(protobuf_bs);
+
+    wh = bs->next_buffer(
+        [&]
+        {
+            wait_handle_has_result_in_callback = wh->has_result();
+        });
+
+    bs->buffer_available(mp::Buffer{});
+
+    EXPECT_FALSE(wait_handle_has_result_in_callback);
+}
+
+TEST_F(ClientBufferStreamTest, invokes_callback_on_buffer_unavailable_before_wait_handle_has_result)
+{
+    using namespace ::testing;
+
+    MirWaitHandle* wh{nullptr};
+    bool wait_handle_has_result_in_callback = false;
+
+    EXPECT_CALL(mock_protobuf_server, submit_buffer(_,_,_,_))
+        .WillOnce(RunProtobufClosure());
+
+    auto const protobuf_bs = a_protobuf_buffer_stream(
+        default_pixel_format, default_buffer_usage, a_buffer_package());
+    auto const bs = make_buffer_stream(protobuf_bs);
+
+    wh = bs->next_buffer(
+        [&]
+        {
+            wait_handle_has_result_in_callback = wh->has_result();
+        });
+
+    bs->buffer_unavailable();
+
+    EXPECT_FALSE(wait_handle_has_result_in_callback);
+}