Merge lp:~charlesk/indicator-display/lp-1572545-prompt-in-lockscreen into lp:indicator-display/15.10

[Charles Kerr]

Fix timing issue when talking to com.canonical.UnityGreeter on the bus.

Fix timing issue when talking to com.canonical.UnityGreeter on the bus.

Old behavior: watch for propertychanged events and ask it the value of the IsActive property.

New behavior: watch for propertychanged events and ask it the value of the IsActive property whenever we see the greeter appear on the bus. This way we'll pick up the property value even if the greeter appears on the bus after indicator-display running.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:26
http://jenkins.qa.ubuntu.com/job/indicator-display-ci/26/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-display-wily-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/indicator-display-wily-armhf-ci/9/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-display-ci/26/rebuild

review: Needs Fixing (continuous-integration)

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

review: Approve

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

[Antti Kaijanmäki (kaijanmaki)]

LGTM.

review: Approve

[lp:~charlesk/indicator-display/lp-1572545-prompt-in-lockscreen]

38. By Charles Kerr on 2016-04-27

increase timeout interval on usb manager integration test

[Charles Kerr (charlesk)]

review: Approve

[lp:~charlesk/indicator-display/lp-1572545-prompt-in-lockscreen]

39. By Charles Kerr on 2016-04-29

don't clear the snap decision when the greeter changes state

40. By Charles Kerr on 2016-05-03

always send a response to adb even in edge cases; e.g. send an auto-NO if the usb connection is broken while the prompt is up

41. By Charles Kerr on 2016-05-03

sync UsbManagerFixture::USBDisconnectedDuringPrompt to r40, to expect a 'no' each time there's a usb disconnect

Unmerged revisions

=== modified file 'src/greeter.cpp'
--- src/greeter.cpp	2016-03-22 13:15:38 +0000
+++ src/greeter.cpp	2016-05-04 00:04:00 +0000
@@ -26,117 +26,166 @@
 {
 public:
 
-    Impl():
-        m_cancellable{g_cancellable_new()}
-    {
-        g_bus_get(G_BUS_TYPE_SESSION, m_cancellable, on_bus_ready_static, 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);
-
-        g_clear_object(&m_bus);
-    }
-
-    core::Property<bool>& is_active()
-    {
-        return m_is_active;
+    Impl()
+    {
+        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() =default;
+
+    core::Property<State>& state()
+    {
+        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);
-    }
-
-    void on_bus_ready(GDBusConnection* bus)
-    {
-        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);
-    }
-
-    static void on_get_is_active_ready(GObject* source, GAsyncResult* res, gpointer gself)
+        }
+        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);
+    }
+
+    static void on_greeter_vanished(
+        GDBusConnection* /*bus*/,
+        const char* /*name*/,
+        gpointer gself)
+    {
+        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)
     {
         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);
+                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 +203,8 @@
 
 UnityGreeter::~UnityGreeter() =default;
 
-core::Property<bool>&
-UnityGreeter::is_active()
+core::Property<Greeter::State>&
+UnityGreeter::state()
 {
-    return impl->is_active();
+    return impl->state();
 }
 
=== modified file 'src/greeter.h'
--- src/greeter.h	2016-03-21 23:15:05 +0000
+++ src/greeter.h	2016-05-04 00:04:00 +0000
@@ -29,7 +29,13 @@
 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 @@
 public:
     UnityGreeter();
     virtual ~UnityGreeter();
-    core::Property<bool>& is_active() override;
+    core::Property<State>& state() override;
 
 protected:
     class Impl;
 
=== modified file 'src/usb-manager.cpp'
--- src/usb-manager.cpp	2016-03-23 19:44:23 +0000
+++ src/usb-manager.cpp	2016-05-04 00:04:00 +0000
@@ -46,72 +46,84 @@
         m_greeter{greeter}
     {
         m_usb_monitor->on_usb_disconnected().connect([this](const std::string& /*usb_name*/) {
-            restart();
-        });
-
-        m_greeter->is_active().changed().connect([this](bool /*is_active*/) {
-            maybe_snap();
-        });
-
-        restart();
+            m_req.reset();
+        });
+
+        m_greeter->state().changed().connect([this](const Greeter::State& state) {
+            if (state == Greeter::State::INACTIVE)
+                maybe_snap();
+            else
+                stop_snap();
+        });
+
+        // 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()
-    {
-        clear();
-
-        // 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();
-            }
-        );
     }
 
     void maybe_snap()
     {
-        // don't prompt in the greeter!
-        if (!m_req.public_key.empty() && !m_greeter->is_active().get())
-            snap();
+        // only prompt if there's something to prompt about
+        if (!m_req)
+            return;
+
+        // only prompt in an unlocked session
+        if (m_greeter->state().get() != Greeter::State::INACTIVE)
+            return;
+
+        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);
+                    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 +164,13 @@
     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;
 };
 
 /***
 
=== modified file 'src/usb-snap.cpp'
--- src/usb-snap.cpp	2016-03-23 17:16:06 +0000
+++ src/usb-snap.cpp	2016-05-04 00:04:00 +0000
@@ -208,6 +208,7 @@
         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)
 
=== modified file 'tests/integration/usb-manager-test.cpp'
--- tests/integration/usb-manager-test.cpp	2016-03-23 17:16:06 +0000
+++ tests/integration/usb-manager-test.cpp	2016-05-04 00:04:00 +0000
@@ -143,7 +143,7 @@
     );
 
     // 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 @@
     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 @@
         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 @@
     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 @@
     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();
 
=== modified file 'tests/unit/CMakeLists.txt'
--- tests/unit/CMakeLists.txt	2016-03-16 00:25:33 +0000
+++ tests/unit/CMakeLists.txt	2016-05-04 00:04:00 +0000
@@ -14,6 +14,10 @@
     ${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 @@
   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)
 
=== added file 'tests/unit/greeter-test.cpp'
--- tests/unit/greeter-test.cpp	1970-01-01 00:00:00 +0000
+++ tests/unit/greeter-test.cpp	2016-05-04 00:04:00 +0000
@@ -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 <[e-mail]>
+ */
+
+#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());
+}
+
 
=== modified file 'tests/unit/usb-snap-test.cpp'
--- tests/unit/usb-snap-test.cpp	2016-03-23 17:16:06 +0000
+++ tests/unit/usb-snap-test.cpp	2016-05-04 00:04:00 +0000
@@ -130,14 +130,9 @@
         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));
     }
 }
 
=== modified file 'tests/utils/adbd-server.h'
--- tests/utils/adbd-server.h	2016-03-24 16:01:16 +0000
+++ tests/utils/adbd-server.h	2016-05-04 00:04:00 +0000
@@ -38,7 +38,7 @@
     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 @@
         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 @@
 
     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 @@
             // cleanup
             g_clear_object(&client_socket);
         }
-
-        g_clear_object(&server_socket);
     }
 
     // bind to a local domain socket
@@ -139,11 +134,14 @@
         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;
 };
 
 
=== added file 'tests/utils/gtest-print-helpers.h'
--- tests/utils/gtest-print-helpers.h	1970-01-01 00:00:00 +0000
+++ tests/utils/gtest-print-helpers.h	2016-05-04 00:04:00 +0000
@@ -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;
+}
+
 
=== modified file 'tests/utils/mock-greeter.h'
--- tests/utils/mock-greeter.h	2016-03-22 20:39:36 +0000
+++ tests/utils/mock-greeter.h	2016-05-04 00:04:00 +0000
@@ -26,7 +26,7 @@
 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};
 };
 
 
=== added file 'tests/utils/mock-unity-greeter.py'
--- tests/utils/mock-unity-greeter.py	1970-01-01 00:00:00 +0000
+++ tests/utils/mock-unity-greeter.py	2016-05-04 00:04:00 +0000
@@ -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__ = '[e-mail]'
+__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')
+    )
+