Merge lp:~andreas-pokorny/mir/wait-for-input-config-before-emitting-input-state into lp:mir

[Andreas Pokorny]

Wait for the arrival of the input config message before attempting to propagate input device state events

The former informs the nested server about the available devices, the latter is supposed to synchronize the host server input state with the focused client (here the nested server).

When launching the nested server there is a potential race between the initial input device state event and the input config. This problem is most visible when vt switching. Since the per-surface input fds have been removed, and all events are sent to the connection this race is harder to notice. Still the two messages may be sent by different threads in the server. This should be the last cause for the two prominent CI failures mentioned.

This race was found while working on the issue with stuck alt keys.

There are two messages involved. One indicates which input devices are available in the system. The one is treated as an Event the InputDeviceStateEvent, which provides the current input status like cursor position, pointer buttons, modifier state and pressed keys.

In the past a reordering of that was easy to trigger because it involved separate fds, now it requires a specific scheduling on the server, which happens frequently when trying to simulate pause/resume behavior.

Maybe the attached bugs have already been fixed or are just more rare due to other changes:
* removal of the per-surface fds
* updated surface input dispatcher logic in mir::input::SurfaceInputDispatcher::for_each)

Nonetheless the change here is related to the two linked problems.

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

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

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

The root cause of this issue is that the server-client information exchange does not enforce the required order. It would be preferable if we fixed the out of order issue in the server, rather than implementing what is essentially a workaround in the client. Is this feasible?

Also, is there the possibility of this race occurring after we already have some input devices (i.e. a devices is added after startup and the state event reaches the client before the input-config event)? If so, the workaround is not enough, since it is only applied when devices.empty() == true.

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

> The root cause of this issue is that the server-client information exchange
> does not enforce the required order. It would be preferable if we fixed the
> out of order issue in the server, rather than implementing what is essentially
> a workaround in the client. Is this feasible?

The MP here seemed like the easy fix to get the follow up parts working.

> Also, is there the possibility of this race occurring after we already have
> some input devices (i.e. a devices is added after startup and the state event
> reaches the client before the input-config event)? If so, the workaround is
> not enough, since it is only applied when devices.empty() == true.

Sounds right..

I will try to change the way the input device changes are handled in the server... currently through an observer in the main thread .. vs the input thread..

[Daniel van Vugt (vanvugt)]

So should this be work in progress now?

=== modified file 'src/server/graphics/nested/input_platform.cpp'
--- src/server/graphics/nested/input_platform.cpp	2017-03-03 07:08:58 +0000
+++ src/server/graphics/nested/input_platform.cpp	2017-03-08 15:05:26 +0000
@@ -338,34 +338,9 @@
             {
                 std::lock_guard<std::mutex> lock(devices_guard);
                 if (!devices.empty())
-                {
-                    auto const* device_state = mir_event_get_input_device_state_event(&event);
-                    for (size_t index = 0, end_index = mir_input_device_state_event_device_count(device_state);
-                         index != end_index; ++index)
-                    {
-                        auto it = devices.find(mir_input_device_state_event_device_id(device_state, index));
-                        if (it != end(devices) && it->second->destination)
-                        {
-                            auto dest = it->second->destination;
-                            auto key_count = mir_input_device_state_event_device_pressed_keys_count(device_state, index);
-                            std::vector<uint32_t> scan_codes;
-                            for (uint32_t i = 0; i < key_count; i++)
-                            {
-                                scan_codes.push_back(mir_input_device_state_event_device_pressed_keys_for_index(device_state, index, i));
-                            }
-
-                            dest->key_state(scan_codes);
-                            dest->pointer_state(
-                                mir_input_device_state_event_device_pointer_buttons(device_state, index));
-                        }
-                    }
-
-                    auto& front = begin(devices)->second;
-                    auto device_state_event = front->builder->device_state_event(
-                        mir_input_device_state_event_pointer_axis(device_state, mir_pointer_axis_x),
-                        mir_input_device_state_event_pointer_axis(device_state, mir_pointer_axis_y));
-                    front->destination->handle_input(*device_state_event);
-                }
+                    handle_device_state(event);
+                else
+                    early_device_states.push_back(mir::events::clone_event(event));
             }
         });
 }
@@ -421,7 +396,13 @@
     for (auto new_dev : new_devs)
     {
         input_device_registry->add_device(new_dev.first);
-
+    }
+    for (auto const& event : early_device_states)
+    {
+        handle_device_state(*event);
+    }
+    for (auto new_dev : new_devs)
+    {
         auto early_event_queue = unknown_device_events.find(new_dev.second);
         if (early_event_queue != end(unknown_device_events))
         {
@@ -433,6 +414,7 @@
         }
     }
     unknown_device_events.clear();
+    early_device_states.clear();
 }
 
 void mgn::InputPlatform::config_changed()
@@ -461,3 +443,33 @@
     }
     state = started;
 }
+
+void mgn::InputPlatform::handle_device_state(MirEvent const& event)
+{
+    auto const* device_state = mir_event_get_input_device_state_event(&event);
+    for (size_t index = 0, end_index = mir_input_device_state_event_device_count(device_state);
+         index != end_index; ++index)
+    {
+        auto it = devices.find(mir_input_device_state_event_device_id(device_state, index));
+        if (it != end(devices) && it->second->destination)
+        {
+            auto dest = it->second->destination;
+            auto key_count = mir_input_device_state_event_device_pressed_keys_count(device_state, index);
+            std::vector<uint32_t> scan_codes;
+            for (uint32_t i = 0; i < key_count; i++)
+            {
+                scan_codes.push_back(mir_input_device_state_event_device_pressed_keys_for_index(device_state, index, i));
+            }
+
+            dest->key_state(scan_codes);
+            dest->pointer_state(
+                mir_input_device_state_event_device_pointer_buttons(device_state, index));
+        }
+    }
+
+    auto& front = begin(devices)->second;
+    auto device_state_event = front->builder->device_state_event(
+        mir_input_device_state_event_pointer_axis(device_state, mir_pointer_axis_x),
+        mir_input_device_state_event_pointer_axis(device_state, mir_pointer_axis_y));
+    front->destination->handle_input(*device_state_event);
+}

=== modified file 'src/server/graphics/nested/input_platform.h'
--- src/server/graphics/nested/input_platform.h	2017-02-15 07:38:33 +0000
+++ src/server/graphics/nested/input_platform.h	2017-03-08 15:05:26 +0000
@@ -58,6 +58,7 @@
     void config_changed();
     void update_devices();
     void update_devices_locked();
+    void handle_device_state(MirEvent const& event);
     struct InputDevice;
     std::shared_ptr<HostConnection> const connection;
     std::shared_ptr<input::InputDeviceRegistry> const input_device_registry;
@@ -70,6 +71,7 @@
     std::unordered_map<MirInputDeviceId, std::shared_ptr<InputDevice>> devices;
     std::unordered_map<MirInputDeviceId, std::vector<std::pair<EventUPtr, mir::geometry::Rectangle>>>
         unknown_device_events;
+    std::vector<EventUPtr> early_device_states;
     enum State
     {
         started, stopped, paused