Fix test error in adbd-client-test

Approved by: unity-api-1-bot

14 files changed, 458 insertions, 154 deletions

diff --git a/src/adbd-client.cpp b/src/adbd-client.cpp
index 400c7c9..47914cb 100644
--- a/src/adbd-client.cpp
+++ b/src/adbd-client.cpp
@@ -23,6 +23,7 @@
 #include <gio/gunixsocketaddress.h>
 
 #include <algorithm>
+#include <atomic>
 #include <cctype>
 #include <cstring>
 #include <chrono>
@@ -48,7 +49,9 @@ public:
         g_cancellable_cancel(m_cancellable);
         m_pkresponse_cv.notify_one();
         m_sleep_cv.notify_one();
-        m_worker_thread.join();
+        if (m_worker_thread.joinable()) {
+            m_worker_thread.join();
+        }
         g_clear_object(&m_cancellable);
     }
 
@@ -66,7 +69,7 @@ private:
         GCancellable* cancellable = nullptr;
         const std::string public_key;
 
-        PKIdleData(Impl* self_, GCancellable* cancellable_, std::string public_key_):
+        PKIdleData(Impl* self_, GCancellable* cancellable_, const std::string& public_key_):
             self(self_),
             cancellable(G_CANCELLABLE(g_object_ref(cancellable_))),
             public_key(public_key_) {}
@@ -104,8 +107,9 @@ private:
 
     void on_public_key_response(PKResponse response)
     {
+        g_debug("%s thread %p got response %d", G_STRLOC, g_thread_self(), int(response));
+
         // set m_pkresponse and wake up the waiting worker thread
-        std::unique_lock<std::mutex> lk(m_pkresponse_mutex);
         m_pkresponse = response;
         m_pkresponse_ready = true;
         m_pkresponse_cv.notify_one();
@@ -121,11 +125,11 @@ private:
 
         while (!g_cancellable_is_cancelled(m_cancellable))
         {
-            g_debug("%s creating a client socket to '%s'", G_STRLOC, socket_path.c_str());
+            g_debug("%s thread %p creating a client socket to '%s'", G_STRLOC, g_thread_self(), socket_path.c_str());
             auto socket = create_client_socket(socket_path);
             bool got_valid_req = false;
 
-            g_debug("%s calling read_request", G_STRLOC);
+            g_debug("%s thread %p calling read_request", g_thread_self(), G_STRLOC);
             std::string reqstr;
             if (socket != nullptr)
                 reqstr = read_request(socket);
@@ -135,22 +139,25 @@ private:
             if (reqstr.substr(0,2) == "PK") {
                 PKResponse response = PKResponse::DENY;
                 const auto public_key = reqstr.substr(2);
-                g_debug("%s got pk [%s]", G_STRLOC, public_key.c_str());
+                g_debug("%s thread %p got pk [%s]", G_STRLOC, g_thread_self(), public_key.c_str());
                 if (!public_key.empty()) {
                     got_valid_req = true;
                     std::unique_lock<std::mutex> lk(m_pkresponse_mutex);
                     m_pkresponse_ready = false;
+                    m_pkresponse = AdbdClient::PKResponse::DENY;
                     pass_public_key_to_main_thread(public_key);
                     m_pkresponse_cv.wait(lk, [this](){
                         return m_pkresponse_ready || g_cancellable_is_cancelled(m_cancellable);
                     });
                     response = m_pkresponse;
-                    g_debug("%s got response '%d', is-cancelled %d", G_STRLOC,
+                    g_debug("%s thread %p got response '%d', is-cancelled %d", G_STRLOC,
+                            g_thread_self(),
                             int(response),
                             int(g_cancellable_is_cancelled(m_cancellable)));
                 }
-                if (!g_cancellable_is_cancelled(m_cancellable))
+                if (!g_cancellable_is_cancelled(m_cancellable)) {
                     send_pk_response(socket, response);
+                }
             } else if (!reqstr.empty()) {
                 g_warning("Invalid ADB request: [%s]", reqstr.c_str());
             }
@@ -270,7 +277,7 @@ private:
 
     std::mutex m_pkresponse_mutex;
     std::condition_variable m_pkresponse_cv;
-    bool m_pkresponse_ready = false;
+    std::atomic<bool> m_pkresponse_ready {false};
     PKResponse m_pkresponse = PKResponse::DENY;
 };
 
diff --git a/src/greeter.cpp b/src/greeter.cpp
index f9cd965..3d0f347 100644
--- a/src/greeter.cpp
+++ b/src/greeter.cpp
@@ -26,117 +26,167 @@ class UnityGreeter::Impl
 {
 public:
 
-    Impl():
-        m_cancellable{g_cancellable_new()}
+    Impl()
     {
-        g_bus_get(G_BUS_TYPE_SESSION, m_cancellable, on_bus_ready_static, this);
+        m_cancellable.reset(
+            g_cancellable_new(),
+            [](GCancellable* o){
+                g_cancellable_cancel(o);
+                g_clear_object(&o);
+            }
+        );
+
+        g_bus_get(G_BUS_TYPE_SESSION, m_cancellable.get(), on_bus_ready, this);
     }
 
-    ~Impl()
-    {
-        g_cancellable_cancel(m_cancellable);
-        g_clear_object(&m_cancellable);
-
-        if (m_subscription_id != 0)
-            g_dbus_connection_signal_unsubscribe (m_bus, m_subscription_id);
+    ~Impl() =default;
 
-        g_clear_object(&m_bus);
-    }
-
-    core::Property<bool>& is_active()
+    core::Property<State>& state()
     {
-        return m_is_active;
+        return m_state;
     }
 
 private:
 
-    static void on_bus_ready_static(GObject* /*source*/, GAsyncResult* res, gpointer gself)
+    void set_state(const State& state)
+    {
+        m_state.set(state);
+    }
+
+    static void on_bus_ready(
+        GObject* /*source*/,
+        GAsyncResult* res,
+        gpointer gself)
     {
         GError* error {};
-        auto bus = g_bus_get_finish (res, &error);
-        if (error != nullptr) {
+        auto bus = g_bus_get_finish(res, &error);
+        if (error != nullptr)
+        {
             if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-                g_warning("UsbSnap: Error getting session bus: %s", error->message);
+                g_warning("Greeter: Error getting bus: %s", error->message);
             g_clear_error(&error);
-        } else {
-            static_cast<Impl*>(gself)->on_bus_ready(bus);
         }
-        g_clear_object(&bus);
+        else
+        {
+            auto self = static_cast<Impl*>(gself);
+
+            const auto watcher_id = g_bus_watch_name_on_connection(
+                bus,
+                DBusNames::UnityGreeter::NAME,
+                G_BUS_NAME_WATCHER_FLAGS_AUTO_START,
+                on_greeter_appeared,
+                on_greeter_vanished,
+                gself,
+                nullptr);
+
+            const auto subscription_id = g_dbus_connection_signal_subscribe(
+                bus,
+                DBusNames::UnityGreeter::NAME,
+                DBusNames::Properties::INTERFACE,
+                DBusNames::Properties::PropertiesChanged::NAME,
+                DBusNames::UnityGreeter::PATH,
+                DBusNames::UnityGreeter::INTERFACE,
+                G_DBUS_SIGNAL_FLAGS_NONE,
+                on_properties_changed_signal,
+                gself,
+                nullptr);
+
+            self->m_bus.reset(
+                bus,
+                [watcher_id, subscription_id](GDBusConnection* o){
+                    g_bus_unwatch_name(watcher_id);
+                    g_dbus_connection_signal_unsubscribe(o, subscription_id);
+                    g_clear_object(&o);
+                }
+            );
+        }
+    }
+
+    static void on_greeter_appeared(
+        GDBusConnection* bus,
+        const char* /*name*/,
+        const char* name_owner,
+        gpointer gself)
+    {
+        auto self = static_cast<Impl*>(gself);
+
+        self->m_owner = name_owner;
+
+        g_dbus_connection_call(
+            bus,
+            DBusNames::UnityGreeter::NAME,
+            DBusNames::UnityGreeter::PATH,
+            DBusNames::Properties::INTERFACE,
+            "Get",
+            g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"),
+            G_VARIANT_TYPE("(v)"),
+            G_DBUS_CALL_FLAGS_NONE,
+            -1,
+            self->m_cancellable.get(),
+            on_get_is_active_ready,
+            gself);
     }
 
-    void on_bus_ready(GDBusConnection* bus)
+    static void on_greeter_vanished(
+        GDBusConnection* /*bus*/,
+        const char* /*name*/,
+        gpointer gself)
     {
-        m_bus = G_DBUS_CONNECTION(g_object_ref(G_OBJECT(bus)));
-
-        g_dbus_connection_call(m_bus,
-                               DBusNames::UnityGreeter::NAME,
-                               DBusNames::UnityGreeter::PATH,
-                               DBusNames::Properties::INTERFACE,
-                               "Get",
-                               g_variant_new("(ss)", DBusNames::UnityGreeter::INTERFACE, "IsActive"),
-                               G_VARIANT_TYPE("(v)"),
-                               G_DBUS_CALL_FLAGS_NONE,
-                               -1,
-                               m_cancellable,
-                               on_get_is_active_ready,
-                               this);
-
-        m_subscription_id = g_dbus_connection_signal_subscribe(m_bus,
-                                                               DBusNames::UnityGreeter::NAME,
-                                                               DBusNames::Properties::INTERFACE,
-                                                               DBusNames::Properties::PropertiesChanged::NAME,
-                                                               DBusNames::UnityGreeter::PATH,
-                                                               DBusNames::UnityGreeter::INTERFACE,
-                                                               G_DBUS_SIGNAL_FLAGS_NONE,
-                                                               on_properties_changed_signal,
-                                                               this,
-                                                               nullptr);
+        auto self = static_cast<Impl*>(gself);
+
+        self->m_owner.clear();
+        self->set_state(State::UNAVAILABLE);
     }
 
-    static void on_get_is_active_ready(GObject* source, GAsyncResult* res, gpointer gself)
+    static void on_get_is_active_ready(
+        GObject* source,
+        GAsyncResult* res, 
+        gpointer gself)
     {
         GError* error {};
         auto v = g_dbus_connection_call_finish(G_DBUS_CONNECTION(source), res, &error);
         if (error != nullptr) {
-            if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
-                g_warning("UsbSnap: Error getting session bus: %s", error->message);
+            if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) {
+                g_warning("Greeter: Error getting IsActive property: %s", error->message);
+            }
             g_clear_error(&error);
         } else {
             GVariant* is_active {};
             g_variant_get_child(v, 0, "v", &is_active);
-            static_cast<Impl*>(gself)->m_is_active.set(g_variant_get_boolean(is_active));
+            static_cast<Impl*>(gself)->set_state(g_variant_get_boolean(is_active) ? State::ACTIVE : State::INACTIVE);
             g_clear_pointer(&is_active, g_variant_unref);
         }
         g_clear_pointer(&v, g_variant_unref);
     }
 
-    static void on_properties_changed_signal(GDBusConnection* /*connection*/,
-                                             const gchar* /*sender_name*/,
-                                             const gchar* object_path,
-                                             const gchar* interface_name,
-                                             const gchar* signal_name,
-                                             GVariant* parameters,
-                                             gpointer gself)
+    static void on_properties_changed_signal(
+        GDBusConnection* /*bus*/,
+        const gchar* sender_name,
+        const gchar* object_path,
+        const gchar* interface_name,
+        const gchar* signal_name,
+        GVariant* parameters,
+        gpointer gself)
     {
+        auto self = static_cast<Impl*>(gself);
+
+        g_return_if_fail(!g_strcmp0(sender_name, self->m_owner.c_str()));
         g_return_if_fail(!g_strcmp0(object_path, DBusNames::UnityGreeter::PATH));
         g_return_if_fail(!g_strcmp0(interface_name, DBusNames::Properties::INTERFACE));
         g_return_if_fail(!g_strcmp0(signal_name, DBusNames::Properties::PropertiesChanged::NAME));
         g_return_if_fail(g_variant_is_of_type(parameters, G_VARIANT_TYPE(DBusNames::Properties::PropertiesChanged::ARGS_VARIANT_TYPE)));
 
-        auto v = g_variant_get_child_value (parameters, 1);
+        auto v = g_variant_get_child_value(parameters, 1);
         gboolean is_active {};
         if (g_variant_lookup(v, "IsActive", "b", &is_active))
-        {
-            g_debug("%s is_active changed to %d", G_STRLOC, int(is_active));
-            static_cast<Impl*>(gself)->m_is_active.set(is_active);
-        }
+            self->set_state(is_active ? State::ACTIVE : State::INACTIVE);
         g_clear_pointer(&v, g_variant_unref);
     }
 
-    core::Property<bool> m_is_active;
-    GCancellable* m_cancellable {};
-    GDBusConnection* m_bus {};
-    unsigned int m_subscription_id {};
+    core::Property<State> m_state {State::UNAVAILABLE};
+    std::shared_ptr<GCancellable> m_cancellable;
+    std::shared_ptr<GDBusConnection> m_bus;
+    std::string m_owner;
 };
 
 /***
@@ -154,8 +204,8 @@ UnityGreeter::UnityGreeter():
 
 UnityGreeter::~UnityGreeter() =default;
 
-core::Property<bool>&
-UnityGreeter::is_active()
+core::Property<Greeter::State>&
+UnityGreeter::state()
 {
-    return impl->is_active();
+    return impl->state();
 }
diff --git a/src/greeter.h b/src/greeter.h
index e084d25..f3012f6 100644
--- a/src/greeter.h
+++ b/src/greeter.h
@@ -29,7 +29,13 @@ class Greeter
 public:
     Greeter();
     virtual ~Greeter();
-    virtual core::Property<bool>& is_active() =0;
+
+    enum class State { UNAVAILABLE, INACTIVE, ACTIVE };
+static inline const char* state_str(const State& state) {
+    static constexpr char const * state_str[] = { "Unavailable", "Inactive", "Active" };
+    return state_str[int(state)];
+}
+    virtual core::Property<State>& state() =0;
 };
 
 
@@ -38,7 +44,7 @@ class UnityGreeter: public Greeter
 public:
     UnityGreeter();
     virtual ~UnityGreeter();
-    core::Property<bool>& is_active() override;
+    core::Property<State>& state() override;
 
 protected:
     class Impl;
diff --git a/src/usb-manager.cpp b/src/usb-manager.cpp
index 4d750c0..f83b5f1 100644
--- a/src/usb-manager.cpp
+++ b/src/usb-manager.cpp
@@ -46,72 +46,89 @@ public:
         m_greeter{greeter}
     {
         m_usb_monitor->on_usb_disconnected().connect([this](const std::string& /*usb_name*/) {
-            restart();
+            m_req.reset();
         });
 
-        m_greeter->is_active().changed().connect([this](bool /*is_active*/) {
-            maybe_snap();
+        m_greeter->state().changed().connect([this](const Greeter::State& state) {
+            if (state == Greeter::State::INACTIVE) {
+                maybe_snap();
+            } else {
+                stop_snap();
+            }
         });
 
-        restart();
+        // create a new adbd client
+        m_adbd_client.reset(new GAdbdClient{m_socket_path});
+        m_adbd_client->on_pk_request().connect(
+            [this](const AdbdClient::PKRequest& req) {
+                g_debug("%s got pk request: %s, calling maybe_snap()", G_STRLOC, req.fingerprint.c_str());
+
+                m_response = AdbdClient::PKResponse::DENY; // set the fallback response
+                m_req.reset(
+                    new AdbdClient::PKRequest(req),
+                    [this](AdbdClient::PKRequest* r) {
+                        stop_snap();
+                        r->respond(m_response);
+                        delete r;
+                    }
+                );
+                maybe_snap();
+            }
+        );
     }
 
     ~Impl()
     {
-        if (m_restart_idle_tag)
-            g_source_remove(m_restart_idle_tag);
-
-        clear();
+        if (m_request_complete_idle_tag) {
+            g_source_remove(m_request_complete_idle_tag);
+        }
     }
 
 private:
 
-    void clear()
+    void stop_snap()
     {
-        // clear out old state
         m_snap_connections.clear();
         m_snap.reset();
-        m_req = decltype(m_req)();
-        m_adbd_client.reset();
     }
 
-    void restart()
+    void maybe_snap()
     {
-        clear();
+        // only prompt if there's something to prompt about
+        if (!m_req) {
+            return;
+        }
 
-        // set a new client
-        m_adbd_client.reset(new GAdbdClient{m_socket_path});
-        m_adbd_client->on_pk_request().connect(
-            [this](const AdbdClient::PKRequest& req) {
-                g_debug("%s got pk request: %s", G_STRLOC, req.fingerprint.c_str());
-                m_req = req;
-                maybe_snap();
-            }
-        );
-    }
+        // only prompt in an unlocked session
+        if (m_greeter->state().get() != Greeter::State::INACTIVE) {
+            return;
+        }
 
-    void maybe_snap()
-    {
-        // don't prompt in the greeter!
-        if (!m_req.public_key.empty() && !m_greeter->is_active().get())
-            snap();
+        snap();
     }
 
     void snap()
     {
-        m_snap = std::make_shared<UsbSnap>(m_req.fingerprint);
+        m_snap = std::make_shared<UsbSnap>(m_req->fingerprint);
         m_snap_connections.insert((*m_snap).on_user_response().connect(
             [this](AdbdClient::PKResponse response, bool remember_choice){
-                g_debug("%s user responded! response %d, remember %d", G_STRLOC, int(response), int(remember_choice));
-                m_req.respond(response);
-                if (remember_choice && (response == AdbdClient::PKResponse::ALLOW))
-                    write_public_key(m_req.public_key);
-                m_restart_idle_tag = g_idle_add([](gpointer gself){
-                    auto self = static_cast<Impl*>(gself);
-                    self->m_restart_idle_tag = 0;
-                    self->restart();
-                    return G_SOURCE_REMOVE;
-                }, this);
+
+                if (remember_choice && (response == AdbdClient::PKResponse::ALLOW)) {
+                    write_public_key(m_req->public_key);
+                }
+
+                m_response = response;
+
+                // defer finishing the request into an idle func because
+                // ScopedConnections can't be destroyed inside their callbacks
+                if (m_request_complete_idle_tag == 0) {
+                    m_request_complete_idle_tag = g_idle_add([](gpointer gself){
+                        auto self = static_cast<Impl*>(gself);
+                        self->m_request_complete_idle_tag = 0;
+                        self->m_req.reset();
+                        return G_SOURCE_REMOVE;
+                    }, this);
+                }
             }
         ));
     }
@@ -152,12 +169,13 @@ private:
     const std::shared_ptr<UsbMonitor> m_usb_monitor;
     const std::shared_ptr<Greeter> m_greeter;
  
-    unsigned int m_restart_idle_tag {};
+    unsigned int m_request_complete_idle_tag {};
 
     std::shared_ptr<GAdbdClient> m_adbd_client;
-    AdbdClient::PKRequest m_req;
     std::shared_ptr<UsbSnap> m_snap;
     std::set<core::ScopedConnection> m_snap_connections;
+    AdbdClient::PKResponse m_response {AdbdClient::PKResponse::DENY};
+    std::shared_ptr<AdbdClient::PKRequest> m_req;
 };
 
 /***
@@ -177,4 +195,3 @@ UsbManager::UsbManager(
 UsbManager::~UsbManager()
 {
 }
-
diff --git a/src/usb-snap.cpp b/src/usb-snap.cpp
index ba964fb..53db6c4 100644
--- a/src/usb-snap.cpp
+++ b/src/usb-snap.cpp
@@ -208,6 +208,7 @@ private:
         const bool remember_this_choice = response == AdbdClient::PKResponse::ALLOW;
 
         m_on_user_response(response, remember_this_choice);
+        m_notification_id = 0;
     }
 
     void on_notification_closed(uint32_t close_reason)
diff --git a/tests/integration/usb-manager-test.cpp b/tests/integration/usb-manager-test.cpp
index d62756f..805ba6e 100644
--- a/tests/integration/usb-manager-test.cpp
+++ b/tests/integration/usb-manager-test.cpp
@@ -143,7 +143,7 @@ TEST_F(UsbManagerFixture, Allow)
     );
 
     // confirm that the AdbdServer got the right response
-    wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000);
+    wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000);
     ASSERT_EQ(1, adbd_server->m_responses.size());
     EXPECT_EQ("OK", adbd_server->m_responses.front());
 
@@ -163,13 +163,16 @@ TEST_F(UsbManagerFixture, USBDisconnectedDuringPrompt)
     const std::shared_ptr<std::string> public_keys_path {new std::string{*m_tmpdir+"/adb_keys"}, file_deleter};
 
     // start a mock AdbdServer ready to submit a request
+    const size_t N_TESTS {3};
     const std::string public_key {"public_key"};
-    auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key});
+    const std::vector<std::string> requests(N_TESTS, "PK"+public_key);
+    const std::vector<std::string> expected_responses(N_TESTS, "NO");
+    auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, requests);
 
     // set up a UsbManager to process the request
     auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter);
 
-    for (int i=0; i<3; i++)
+    for (std::remove_const<decltype(N_TESTS)>::type i=0; i<N_TESTS; ++i)
     {
         // add a signal spy to listen to the notification daemon
         QSignalSpy notificationsSpy(
@@ -194,6 +197,9 @@ TEST_F(UsbManagerFixture, USBDisconnectedDuringPrompt)
         EXPECT_EQ("CloseNotification", notificationsSpy.at(0).at(0));
         notificationsSpy.clear();
     }
+
+    EXPECT_TRUE(wait_for([adbd_server, N_TESTS](){return adbd_server->m_responses.size() == N_TESTS;}, 5000));
+    EXPECT_EQ(expected_responses, adbd_server->m_responses);
 }
 
 TEST_F(UsbManagerFixture, Greeter)
@@ -206,7 +212,7 @@ TEST_F(UsbManagerFixture, Greeter)
     auto adbd_server = std::make_shared<GAdbdServer>(*socket_path, std::vector<std::string>{"PK"+public_key});
 
     // set up a UsbManager to process the request
-    m_greeter->m_is_active.set(true);
+    m_greeter->m_state.set(Greeter::State::ACTIVE);
     auto usb_manager = std::make_shared<UsbManager>(*socket_path, *public_keys_path, m_usb_monitor, m_greeter);
 
     // add a signal spy to listen to the notification daemon
@@ -219,7 +225,7 @@ TEST_F(UsbManagerFixture, Greeter)
     EXPECT_FALSE(notificationsSpy.wait(2000));
 
     // disable the greeter, the notification should appear
-    m_greeter->m_is_active.set(false);
+    m_greeter->m_state.set(Greeter::State::INACTIVE);
     wait_for_signals(notificationsSpy, 1);
     EXPECT_EQ("Notify", notificationsSpy.at(0).at(0));
     notificationsSpy.clear();
diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt
index fe70461..9d8cad2 100644
--- a/tests/unit/CMakeLists.txt
+++ b/tests/unit/CMakeLists.txt
@@ -14,6 +14,10 @@ set(TEST_LINK_LIBRARIES
     ${GMOCK_LIBRARIES}
 )
 
+add_definitions(
+  -DGREETER_TEMPLATE="${CMAKE_SOURCE_DIR}/tests/utils/mock-unity-greeter.py"
+)
+
 function(add_test_by_name name)
   set(TEST_NAME ${name})
   add_executable (${TEST_NAME} ${TEST_NAME}.cpp)
@@ -31,4 +35,5 @@ function(add_qt_test_by_name name)
   set_property(TEST ${TEST_NAME} APPEND PROPERTY ENVIRONMENT ${CTEST_ENVIRONMENT})
   target_link_libraries(${TEST_NAME} ${SERVICE_LINK_LIBRARIES} ${QT_LINK_LIBRARIES} ${TEST_LINK_LIBRARIES} ${THREAD_LINK_LIBRARIES})
 endfunction()
+add_qt_test_by_name(greeter-test)
 add_qt_test_by_name(usb-snap-test)
diff --git a/tests/unit/adbd-client-test.cpp b/tests/unit/adbd-client-test.cpp
index 754f76c..d1ce740 100644
--- a/tests/unit/adbd-client-test.cpp
+++ b/tests/unit/adbd-client-test.cpp
@@ -73,7 +73,7 @@ TEST_F(AdbdClientFixture, SocketPlumbing)
         // start an AdbdClient that listens for PKRequests
         std::string pk;
         auto adbd_client = std::make_shared<GAdbdClient>(socket_path);
-        adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){
+        auto connection = adbd_client->on_pk_request().connect([&pk, main_thread, test](const AdbdClient::PKRequest& req){
             EXPECT_EQ(main_thread, g_thread_self());
             g_message("in on_pk_request with %s", req.public_key.c_str());
             pk = req.public_key;
@@ -82,12 +82,13 @@ TEST_F(AdbdClientFixture, SocketPlumbing)
 
         // start a mock AdbdServer with to fire test key requests and wait for a response
         auto adbd_server = std::make_shared<GAdbdServer>(socket_path, std::vector<std::string>{test.request});
-        wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 2000);
+        wait_for([adbd_server](){return !adbd_server->m_responses.empty();}, 5000);
         EXPECT_EQ(test.expected_pk, pk);
         ASSERT_EQ(1, adbd_server->m_responses.size());
         EXPECT_EQ(test.expected_response, adbd_server->m_responses.front());
    
         // cleanup
+        connection.disconnect();
         adbd_client.reset();
         adbd_server.reset();
         g_unlink(socket_path.c_str());
diff --git a/tests/unit/greeter-test.cpp b/tests/unit/greeter-test.cpp
new file mode 100644
index 0000000..bfa88e8
--- /dev/null
+++ b/tests/unit/greeter-test.cpp
@@ -0,0 +1,159 @@
+/*
+ * Copyright 2016 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 warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, 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/>.
+ *
+ * Authors:
+ *   Charles Kerr <charles.kerr@canonical.com>
+ */
+
+#include <tests/utils/qt-fixture.h>
+#include <tests/utils/gtest-print-helpers.h>
+
+#include <src/dbus-names.h>
+#include <src/greeter.h>
+
+#include <libqtdbustest/DBusTestRunner.h>
+#include <libqtdbustest/QProcessDBusService.h>
+#include <libqtdbusmock/DBusMock.h>
+
+class GreeterFixture: public QtFixture
+{
+private:
+
+    using super = QtFixture;
+
+public:
+
+    GreeterFixture() =default;
+    ~GreeterFixture() =default;
+
+protected:
+
+    std::shared_ptr<QtDBusTest::DBusTestRunner> m_dbus_runner;
+    std::shared_ptr<QtDBusMock::DBusMock> m_dbus_mock;
+    GDBusConnection* m_bus {};
+
+    void SetUp() override
+    {
+        super::SetUp();
+  
+        // use a fresh bus for each test run
+        m_dbus_runner.reset(new QtDBusTest::DBusTestRunner());
+        m_dbus_mock.reset(new QtDBusMock::DBusMock(*m_dbus_runner.get()));
+
+        GError* error {};
+        m_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, nullptr, &error);
+        g_assert_no_error(error);
+        g_dbus_connection_set_exit_on_close(m_bus, FALSE);
+    }
+
+    void TearDown() override
+    {
+        g_clear_object(&m_bus);
+
+        super::TearDown();
+    }
+
+    void start_greeter_service(bool is_active)
+    {
+        // set a watcher to look for our mock greeter to appear
+        bool owned {};
+        QDBusServiceWatcher watcher(
+            DBusNames::UnityGreeter::NAME,
+            m_dbus_runner->sessionConnection()
+        );
+        QObject::connect(
+            &watcher,
+            &QDBusServiceWatcher::serviceRegistered,
+            [&owned](const QString&){owned = true;}
+        );
+
+        // start the mock greeter
+        QVariantMap parameters;
+        parameters["IsActive"] = QVariant(is_active);
+        m_dbus_mock->registerTemplate(
+            DBusNames::UnityGreeter::NAME,
+            GREETER_TEMPLATE,
+            parameters,
+            QDBusConnection::SessionBus
+        );
+        m_dbus_runner->startServices();
+
+        // wait for the watcher
+        ASSERT_TRUE(wait_for([&owned]{return owned;}));
+    }
+};
+
+#define ASSERT_PROPERTY_EQ_EVENTUALLY(expected_in, property_in) \
+    do { \
+        const auto& e = expected_in; \
+        const auto& p = property_in; \
+        ASSERT_TRUE(wait_for([e, &p](){return e == p.get();})) \
+            << "expected " << e << " but got " << p.get(); \
+   } while(0)
+
+/**
+ * Test startup timing by looking at four different cases:
+ * [unity greeter shows up on bus (before, after) we start listening]
+ *   x [unity greeter is (active, inactive)]
+ */
+
+TEST_F(GreeterFixture, ActiveServiceStartsBeforeWatcher)
+{
+    constexpr bool is_active {true};
+    constexpr Greeter::State expected {Greeter::State::ACTIVE};
+
+    start_greeter_service(is_active);
+
+    UnityGreeter greeter;
+
+    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
+}
+
+TEST_F(GreeterFixture, WatcherStartsBeforeActiveService)
+{
+    constexpr bool is_active {true};
+    constexpr Greeter::State expected {Greeter::State::ACTIVE};
+
+    UnityGreeter greeter;
+
+    start_greeter_service(is_active);
+
+    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
+}
+
+TEST_F(GreeterFixture, InactiveServiceStartsBeforeWatcher)
+{
+    constexpr bool is_active {false};
+    constexpr Greeter::State expected {Greeter::State::INACTIVE};
+
+    start_greeter_service(is_active);
+
+    UnityGreeter greeter;
+
+    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
+}
+
+TEST_F(GreeterFixture, WatcherStartsBeforeInactiveService)
+{
+    constexpr bool is_active {false};
+    constexpr Greeter::State expected {Greeter::State::INACTIVE};
+
+    UnityGreeter greeter;
+
+    start_greeter_service(is_active);
+
+    ASSERT_PROPERTY_EQ_EVENTUALLY(expected, greeter.state());
+}
+
diff --git a/tests/unit/usb-snap-test.cpp b/tests/unit/usb-snap-test.cpp
index 3b778dd..2be4b27 100644
--- a/tests/unit/usb-snap-test.cpp
+++ b/tests/unit/usb-snap-test.cpp
@@ -88,7 +88,7 @@ TEST_F(UsbSnapFixture, TestRoundTrip)
         auto snap = std::make_shared<UsbSnap>(test.fingerprint);
         AdbdClient::PKResponse user_response {};
         bool user_response_set = false;
-        snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){
+        auto connection = snap->on_user_response().connect([&user_response,&user_response_set](AdbdClient::PKResponse response, bool /*remember*/){
             user_response = response;
             user_response_set = true;
         });
@@ -130,14 +130,9 @@ TEST_F(UsbSnapFixture, TestRoundTrip)
         EXPECT_TRUE(user_response_set);
         ASSERT_EQ(test.expected_response, user_response);
 
-        // confirm that the snap dtor cleans up the notification
+        // confirm that the snap dtor doesn't try to close
+        // the notification that's already been closed by user choice
         snap.reset();
-        wait_for_signals(notificationsSpy, 1);
-        {
-            QVariantList const& call(notificationsSpy.at(0));
-            EXPECT_EQ("CloseNotification", call.at(0));
-            QVariantList const& args(call.at(1).toList());
-            EXPECT_EQ(id, args.at(0));
-        }
+        EXPECT_FALSE(notificationsSpy.wait(1000));
     }
 }
diff --git a/tests/utils/adbd-server.h b/tests/utils/adbd-server.h
index b574622..585380f 100644
--- a/tests/utils/adbd-server.h
+++ b/tests/utils/adbd-server.h
@@ -38,7 +38,7 @@ public:
     GAdbdServer(const std::string& socket_path,
                 const std::vector<std::string>& requests):
         m_requests{requests},
-        m_socket_path{socket_path},
+        m_server_socket{create_server_socket(socket_path)},
         m_cancellable{g_cancellable_new()},
         m_worker_thread{&GAdbdServer::worker_func, this}
     {
@@ -50,6 +50,7 @@ public:
         g_cancellable_cancel(m_cancellable);
         m_worker_thread.join();
         g_clear_object(&m_cancellable);
+        g_clear_object(&m_server_socket);
     }
 
     const std::vector<std::string> m_requests;
@@ -59,18 +60,14 @@ private:
 
     void worker_func() // runs in worker thread
     {
-        auto server_socket = create_server_socket(m_socket_path);
         auto requests = m_requests;
 
-        GError* error {};
-        g_socket_listen (server_socket, &error);
-        g_assert_no_error (error);
-
         while (!g_cancellable_is_cancelled(m_cancellable) && !requests.empty())
         {
             // wait for a client connection
             g_message("GAdbdServer::Impl::worker_func() calling g_socket_accept()");
-            auto client_socket = g_socket_accept(server_socket, m_cancellable, &error);
+            GError* error {};
+            auto client_socket = g_socket_accept(m_server_socket, m_cancellable, &error);
             if (error != nullptr) {
                 if (!g_error_matches(error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
                     g_message("GAdbdServer: Error accepting socket connection: %s", error->message);
@@ -121,8 +118,6 @@ private:
             // cleanup
             g_clear_object(&client_socket);
         }
-
-        g_clear_object(&server_socket);
     }
 
     // bind to a local domain socket
@@ -139,11 +134,14 @@ private:
         g_assert_no_error (error);
         g_clear_object (&address);
 
+        g_socket_listen (socket, &error);
+        g_assert_no_error (error);
+
         return socket;
     }
 
-    const std::string m_socket_path;
-    GCancellable* m_cancellable = nullptr;
+    GSocket* m_server_socket {};
+    GCancellable* m_cancellable {};
     std::thread m_worker_thread;
 };
 
diff --git a/tests/utils/gtest-print-helpers.h b/tests/utils/gtest-print-helpers.h
new file mode 100644
index 0000000..60f42b4
--- /dev/null
+++ b/tests/utils/gtest-print-helpers.h
@@ -0,0 +1,18 @@
+
+#pragma once
+
+#include <src/greeter.h>
+
+inline void PrintTo(const Greeter::State& state, std::ostream* os) {
+    switch(state) {
+        case Greeter::State::ACTIVE:      *os << "Active"; break;
+        case Greeter::State::INACTIVE:    *os << "Inactive"; break;
+        case Greeter::State::UNAVAILABLE: *os << "Unavailable"; break;
+    }
+}
+
+inline std::ostream& operator<<(std::ostream& os, const Greeter::State& state) {
+    PrintTo(state, &os);
+    return os;
+}
+
diff --git a/tests/utils/mock-greeter.h b/tests/utils/mock-greeter.h
index 5ac85a0..5015087 100644
--- a/tests/utils/mock-greeter.h
+++ b/tests/utils/mock-greeter.h
@@ -26,7 +26,7 @@ class MockGreeter: public Greeter
 public:
     MockGreeter() =default;
     virtual ~MockGreeter() =default;
-    core::Property<bool>& is_active() override {return m_is_active;}
-    core::Property<bool> m_is_active {false};
+    core::Property<Greeter::State>& state() override {return m_state;}
+    core::Property<Greeter::State> m_state {State::INACTIVE};
 };
 
diff --git a/tests/utils/mock-unity-greeter.py b/tests/utils/mock-unity-greeter.py
new file mode 100644
index 0000000..70fb791
--- /dev/null
+++ b/tests/utils/mock-unity-greeter.py
@@ -0,0 +1,41 @@
+'''unity greeter mock template
+
+Very basic template that just mocks the greeter is-active flag
+'''
+
+# This program is free software; you can redistribute it and/or modify it under
+# the terms of the GNU Lesser General Public License as published by the Free
+# Software Foundation; either version 3 of the License, or (at your option) any
+# later version.  See http://www.gnu.org/copyleft/lgpl.html for the full text
+# of the license.
+
+__author__ = 'Charles Kerr'
+__email__ = 'charles.kerr@canonical.com'
+__copyright__ = '(c) 2016 Canonical Ltd.'
+__license__ = 'LGPL 3+'
+
+import dbus
+import os
+
+from dbusmock import MOCK_IFACE, mockobject
+
+BUS_NAME = 'com.canonical.UnityGreeter'
+MAIN_OBJ = '/'
+MAIN_IFACE = 'com.canonical.UnityGreeter'
+SYSTEM_BUS = False
+
+
+def load(mock, parameters):
+    mock.AddMethods(
+        MAIN_IFACE, [
+            ('HideGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", False)'),
+            ('ShowGreeter', '', '', 'self.Set("com.canonical.UnityGreeter", "IsActive", True)')
+        ]
+    )
+    mock.AddProperties(
+        MAIN_IFACE,
+        dbus.Dictionary({
+            'IsActive': parameters.get('IsActive', False),
+        }, signature='sv')
+    )
+