Merge lp:~albaguirre/mir/fix-1537798 into lp:mir

[Alberto Aguirre]

Fix race in NestedServer.client_sees_set_scaling_factor (LP: #1537798)

Wait until the display configuration is applied before creating a client surface of the nested server;
otherwise the first surface output event will race against the update of the display configuration.

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

PASSED: Continuous integration, rev:3275
https://mir-jenkins.ubuntu.com/job/mir-ci/174/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/174/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3275
http://jenkins.qa.ubuntu.com/job/mir-ci/6145/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5716
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4623
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5672
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/349/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/469
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/469/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/469
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/469/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5669
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5669/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8099
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27107
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/345
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/345/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/201/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27108

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Pre-existing but...

+ /* Now, because we have absolutely no idea when the call to set_base_configuration will *actually*
+ * set the base configuration we get to poll configuration() until we see that it's actually changed.
+ */

...it would be better to use a custom DisplayConfigurationReport to detect the config change. Unfortunately, MediatingDisplayChanger::apply_config() notifies the change before applying it (which is probably wrong).

review: Approve

[Kevin DuBois (kdub)]

lgtm

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/2370/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5727
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4634/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5683
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/202
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-autolanding/202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/202
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-autolanding/202/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5680
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5680/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8107
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/27149

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^-- Looks unrelated. Captured in lp:1539321

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'tests/acceptance-tests/test_nested_mir.cpp'
--- tests/acceptance-tests/test_nested_mir.cpp	2016-01-23 01:38:43 +0000
+++ tests/acceptance-tests/test_nested_mir.cpp	2016-01-28 03:58:00 +0000
@@ -330,6 +330,46 @@
 
         return  std::make_shared<mtd::StubDisplayConfig>(new_displays);
     }
+
+    void change_display_configuration(NestedMirRunner& nested_mir, float scale, MirFormFactor form_factor)
+    {
+        auto const configurator = nested_mir.server.the_display_configuration_controller();
+        std::shared_ptr<mg::DisplayConfiguration> config = nested_mir.server.the_display()->configuration();
+
+        config->for_each_output([scale, form_factor] (mg::UserDisplayConfigurationOutput& output)
+        {
+            output.scale = scale;
+            output.form_factor = form_factor;
+        });
+
+        configurator->set_base_configuration(config);
+    }
+
+    bool wait_for_display_configuration_change(NestedMirRunner& nested_mir, float expected_scale, MirFormFactor expected_form_factor)
+    {
+        using namespace std::literals::chrono_literals;
+        /* Now, because we have absolutely no idea when the call to set_base_configuration will *actually*
+        * set the base configuration we get to poll configuration() until we see that it's actually changed.
+        */
+       auto const end_time = std::chrono::steady_clock::now() + 10s;
+       bool done{false};
+       while (!done && (std::chrono::steady_clock::now() < end_time))
+       {
+           auto const new_config = nested_mir.server.the_display()->configuration();
+
+           new_config->for_each_output([&done, expected_scale, expected_form_factor] (auto const& output)
+           {
+               if (output.scale == expected_scale &&
+                   output.form_factor == expected_form_factor)
+               {
+                   done = true;
+               }
+           });
+           if (!done)
+               std::this_thread::sleep_for(100ms);
+       }
+       return done;
+    }
 };
 
 struct Client
@@ -472,64 +512,27 @@
 
 TEST_F(NestedServer, shell_sees_set_scaling_factor)
 {
-    using namespace std::literals::chrono_literals;
     NestedMirRunner nested_mir{new_connection()};
 
     constexpr float expected_scale{2.3f};
     constexpr MirFormFactor expected_form_factor{mir_form_factor_tv};
 
-    auto const configurator = nested_mir.server.the_display_configuration_controller();
-    std::shared_ptr<mg::DisplayConfiguration> config = nested_mir.server.the_display()->configuration();
-
-    config->for_each_output([] (mg::UserDisplayConfigurationOutput& output)
-        {
-            output.scale = expected_scale;
-            output.form_factor = expected_form_factor;
-        });
-
-    configurator->set_base_configuration(config);
-
-    /* Now, because we have absolutely no idea when the call to set_base_configuration will *actually*
-     * set the base configuration we get to poll configuration() until we see that it's actually changed.
-     */
-    auto const end_time = std::chrono::steady_clock::now() + 10s;
-    bool done{false};
-    while (!done && (std::chrono::steady_clock::now() < end_time))
-    {
-        auto const new_config = nested_mir.server.the_display()->configuration();
-
-        new_config->for_each_output([&done, expected_scale, expected_form_factor] (auto const& output)
-                                    {
-                                        if (output.scale == expected_scale &&
-                                            output.form_factor == expected_form_factor)
-                                        {
-                                            done = true;
-                                        }
-                                    });
-    }
-
-    EXPECT_THAT(std::chrono::steady_clock::now(), Le(end_time)) << "Timed out waiting for display configuration to propagate";
+    change_display_configuration(nested_mir, expected_scale, expected_form_factor);
+    auto const config_applied = wait_for_display_configuration_change(nested_mir, expected_scale, expected_form_factor);
+
+    EXPECT_TRUE(config_applied);
 }
 
 TEST_F(NestedServer, client_sees_set_scaling_factor)
 {
-    using namespace std::literals::chrono_literals;
-
     NestedMirRunner nested_mir{new_connection()};
 
     constexpr float expected_scale{2.3f};
     constexpr MirFormFactor expected_form_factor{mir_form_factor_tv};
 
-    auto const configurator = nested_mir.server.the_display_configuration_controller();
-    std::shared_ptr<mg::DisplayConfiguration> config = nested_mir.server.the_display()->configuration();
-
-    config->for_each_output([] (mg::UserDisplayConfigurationOutput& output)
-                            {
-                                output.scale = expected_scale;
-                                output.form_factor = expected_form_factor;
-                            });
-
-    configurator->set_base_configuration(config);
+    change_display_configuration(nested_mir, expected_scale, expected_form_factor);
+    auto const config_applied = wait_for_display_configuration_change(nested_mir, expected_scale, expected_form_factor);
+    EXPECT_TRUE(config_applied);
 
     Client client{nested_mir};
 
@@ -554,12 +557,9 @@
     auto surface = mir_surface_create_sync(spec);
     mir_surface_spec_release(spec);
 
-    auto stream = mir_surface_get_buffer_stream(surface);
-    mir_buffer_stream_swap_buffers_sync(stream);
+    EXPECT_TRUE(surface_event_received.wait_for(30s));
 
     mir_surface_release_sync(surface);
-
-    EXPECT_TRUE(surface_event_received.wait_for(30s));
 }
 
 //////////////////////////////////////////////////////////////////