Merge lp:~marcustomlinson/unity-scopes-api/fix_ui_policy into lp:unity-scopes-api

[Marcus Tomlinson]

1. Depending on whether OnlineAccountClient is used by the shell or by a scope, set the SIGNON_SESSION_DATA_UI_POLICY accordingly.

2. Inform scopes of successful user authentications via a pub/sub network.

3. Removed inc_logins() and dec_logins() as only one session can be active per service (now using info->session != nullptr).

4. Added better thread and exception safety.

5. Run the internal event loop within its own context to avoid clashing with external main loops.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:263
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-ci/473/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-utopic-amd64-ci/100
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-utopic-armhf-ci/100
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-utopic-armhf-ci/100/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-utopic-i386-ci/100

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-ci/473/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

+1

review: Approve

[lp:~marcustomlinson/unity-scopes-api/fix_ui_policy]

264. By Marcus Tomlinson on 2014-10-08

Merged devel

Unmerged revisions

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-09-11 08:35:01 +0000
+++ CMakeLists.txt	2014-10-08 10:59:41 +0000
@@ -195,7 +195,7 @@
 # API version
 set(UNITY_SCOPES_MAJOR 0)
 set(UNITY_SCOPES_MINOR 6)
-set(UNITY_SCOPES_MICRO 6)
+set(UNITY_SCOPES_MICRO 7)
 set(UNITY_SCOPES_SOVERSION 3)
 
 # Version for testing, with all symbols visible

=== modified file 'HACKING'
--- HACKING	2014-09-08 22:15:40 +0000
+++ HACKING	2014-10-08 10:59:41 +0000
@@ -156,7 +156,28 @@
 This creates a diff of the symbols in /tmp/symbols.diff.
 (The demangled symbols from the debian build are in ./new_symbols.)
 
-Review any changes in /tmp/symbols.diff. If they are OK:
+NOTE: We currently have one architecture-specific symbol:
+
+(c++|arch=amd64)"unity::scopes::internal::RegistryObject::ScopeExecData::ScopeExecData(unity::scopes::internal::RegistryObject::ScopeExecData const&)@Base" 0.6.6+14.10.20140916
+
+We need this because the copy-constructor is inlined on i386 and armhf.
+
+The above script can't cope with the arch tag at the moment. Until we get
+around to fixing this, you need to delete the corresponding change from
+the diff output before applying the diff as a patch. The offending diff
+will look something like this:
+
+@@ -377,7 +378,7 @@
+  (c++)"unity::scopes::internal::RegistryObject::remove_desktop_file(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.6.6+14.10.20140916
+  (c++)"unity::scopes::internal::RegistryObject::remove_local_scope(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)@Base" 0.4.0+14.04.20140312.1
+  (c++)"unity::scopes::internal::RegistryObject::ScopeExecData::~ScopeExecData()@Base" 0.4.2+14.04.20140404.2
+- (c++|arch=amd64)"unity::scopes::internal::RegistryObject::ScopeExecData::ScopeExecData(unity::scopes::internal::RegistryObject::ScopeExecData const&)@Base" 0.6.6+14.10.20140916
++ (c++)"unity::scopes::internal::RegistryObject::ScopeExecData::ScopeExecData(unity::scopes::internal::RegistryObject::ScopeExecData const&)@Base" 0replaceme
+ (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::clear_handle_unlocked()@Base" 0.4.2+14.04.20140404.2
+ (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::exec(core::posix::ChildProcess::DeathObserver&, std::shared_ptr<unity::scopes::internal::Executor>)@Base" 0.4.3+14.10.20140428
+ (c++)"unity::scopes::internal::RegistryObject::ScopeProcess::expand_custom_exec()@Base" 0.5.0+14.10.20140619
+
+Review any other changes in /tmp/symbols.diff. If they are OK:
 
     $ cd -
     $ patch -p0 < /tmp/symbols.diff

=== modified file 'RELEASE_NOTES.md'
--- RELEASE_NOTES.md	2014-09-11 08:42:38 +0000
+++ RELEASE_NOTES.md	2014-10-08 10:59:41 +0000
@@ -1,6 +1,11 @@
 Release notes
 =============
 
+Changes in version 0.6.7
+========================
+  - Added new RunInExternalUiMainLoop option to OnlineAccountClient::MainLoopSelect.
+  - OnlineAccountClient signon UI policy determined by main loop used (shell: show / scope: hide).
+
 Changes in version 0.6.6
 ========================
   - Added support for online accounts (via new OnlineAccountClient class).

=== modified file 'debian/changelog'
--- debian/changelog	2014-10-06 12:03:20 +0000
+++ debian/changelog	2014-10-08 10:59:41 +0000
@@ -1,3 +1,10 @@
+unity-scopes-api (0.6.7-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Added new RunInExternalUiMainLoop option to OnlineAccountClient::MainLoopSelect.
+  * OnlineAccountClient signon UI policy determined by main loop used (shell: show / scope: hide).
+
+ -- Marcus Tomlinson <[e-mail]>  Wed, 01 Oct 2014 15:46:30 +0200
+
 unity-scopes-api (0.6.6+14.10.20141006-0ubuntu1) utopic; urgency=medium
 
   [ Pawel Stolowski ]

=== modified file 'include/unity/scopes/OnlineAccountClient.h'
--- include/unity/scopes/OnlineAccountClient.h	2014-09-15 05:10:17 +0000
+++ include/unity/scopes/OnlineAccountClient.h	2014-10-08 10:59:41 +0000
@@ -55,7 +55,7 @@
     */
     struct ServiceStatus
     {
-        uint account_id;            ///< A unique ID of the online account parenting this service.
+        int account_id;             ///< A unique ID of the online account parenting this service.
         bool service_enabled;       ///< True if this service is enabled.
         bool service_authenticated; ///< True if this service is authenticated.
         std::string client_id;      ///< "ConsumerKey" / "ClientId" OAuth (1 / 2) parameter.
@@ -68,16 +68,15 @@
     /**
     \brief Indicates whether an external main loop already exists, or one should be created internally.
 
-    A running main loop is essential in order to receive service update callbacks. When in doubt, set
-    RunInExternalMainLoop and just use refresh_service_statuses() and get_service_statuses() to obtain
-    service statuses.
+    A running main loop is essential in order to receive service updates from the online accounts
+    backend. When in doubt, set to CreateInternalMainLoop.
     */
     enum MainLoopSelect
     {
-        RunInExternalMainLoop,  ///< An external main loop already exists or the service update
-                                ///  callback is not required.
-        CreateInternalMainLoop  ///< An external main loop does not exist and the service update
-                                ///  callback is required.
+        RunInExternalMainLoop,    ///< An external main loop already exists and is running.
+        CreateInternalMainLoop,   ///< An external main loop does not exist.
+        RunInExternalUiMainLoop,  ///< An external UI main loop exists and is running (Intended for
+                                  ///  shell use only. A scope should not be running a UI main loop).
     };
 
     /**
@@ -91,7 +90,7 @@
     OnlineAccountClient(std::string const& service_name,
                         std::string const& service_type,
                         std::string const& provider_name,
-                        MainLoopSelect main_loop_select = RunInExternalMainLoop);
+                        MainLoopSelect main_loop_select = CreateInternalMainLoop);
 
     /// @cond
     ~OnlineAccountClient();

=== modified file 'include/unity/scopes/internal/OnlineAccountClientImpl.h'
--- include/unity/scopes/internal/OnlineAccountClientImpl.h	2014-09-11 11:29:29 +0000
+++ include/unity/scopes/internal/OnlineAccountClientImpl.h	2014-10-08 10:59:41 +0000
@@ -19,6 +19,7 @@
 #ifndef UNITY_SCOPES_INTERNAL_ONLINEACCOUNTCLIENTIMPL_H
 #define UNITY_SCOPES_INTERNAL_ONLINEACCOUNTCLIENTIMPL_H
 
+#include <unity/scopes/internal/MiddlewareBase.h>
 #include <unity/scopes/OnlineAccountClient.h>
 
 #include <libaccounts-glib/accounts-glib.h>
@@ -78,42 +79,51 @@
                                      OnlineAccountClient::PostLoginAction login_failed_action);
 
     // Methods used only by impl
-    void flush_pending_sessions();
+    void construct();
+    void tear_down();
+
+    void flush_pending_session(AgAccountId const& account_id, std::unique_lock<std::mutex>& lock);
 
     void main_loop_state_notify(bool is_running);
 
+    std::shared_ptr<AgManager> manager();
     std::string service_name();
-    void callback(OnlineAccountClient::ServiceStatus const& service_status);
+    OnlineAccountClient::MainLoopSelect main_loop_select();
+
+    void callback(AccountInfo const* info, std::string const& error = "");
 
     bool has_account(AgAccountId const& account_id);
     void add_account(AgAccountId const& account_id, std::shared_ptr<AccountInfo> account_info);
     void remove_account(AgAccountId const& account_id);
 
-    bool inc_logins();
-    void dec_logins();
-
 private:
-    std::string service_name_;
-    std::string service_type_;
-    std::string provider_name_;
+    std::string const service_name_;
+    std::string const service_type_;
+    std::string const provider_name_;
+    OnlineAccountClient::MainLoopSelect const main_loop_select_;
+
+    std::mutex callback_mutex_;
     OnlineAccountClient::ServiceUpdateCallback callback_;
-    bool use_external_main_loop_;
 
-    std::thread main_loop_thread_;
-    std::mutex callback_mutex_;
     std::mutex mutex_;
     std::condition_variable cond_;
+    std::thread main_loop_thread_;
     bool main_loop_is_running_;
-    bool client_stopping_;
-    int logins_busy_;
+    std::exception_ptr thread_exception_;
     gulong account_enabled_signal_id_;
     gulong account_deleted_signal_id_;
 
     std::shared_ptr<GMainLoop> main_loop_;
+    std::shared_ptr<GMainContext> main_loop_context_;
     std::shared_ptr<AgManager> manager_;
     std::map<AgAccountId, std::shared_ptr<AccountInfo>> accounts_;
 
+    MiddlewareBase::SPtr mw_;
+    MWPublisher::UPtr auth_publisher_;
+    MWSubscriber::UPtr auth_subscriber_;
+
     void main_loop_thread();
+    void auth_callback(std::string const& details_json);
 };
 
 } // namespace internal

=== modified file 'src/scopes/internal/OnlineAccountClientImpl.cpp'
--- src/scopes/internal/OnlineAccountClientImpl.cpp	2014-09-15 07:06:51 +0000
+++ src/scopes/internal/OnlineAccountClientImpl.cpp	2014-10-08 10:59:41 +0000
@@ -17,7 +17,12 @@
 */
 
 #include <unity/scopes/internal/OnlineAccountClientImpl.h>
-#include <unity/util/ResourcePtr.h>
+
+#include <unity/scopes/internal/JsonCppNode.h>
+#include <unity/scopes/internal/MiddlewareFactory.h>
+#include <unity/scopes/internal/RuntimeConfig.h>
+#include <unity/scopes/internal/RuntimeImpl.h>
+#include <unity/UnityExceptions.h>
 
 #include <iostream>
 
@@ -88,7 +93,41 @@
     return service_status;
 }
 
-static void clear_session_info(AccountInfo* info)
+static std::string details_to_json(OnlineAccountClient::ServiceStatus const& details)
+{
+    VariantMap vm;
+    vm["account_id"] = details.account_id;
+    vm["service_enabled"] = details.service_enabled;
+    vm["service_authenticated"] = details.service_authenticated;
+    vm["client_id"] = details.client_id;
+    vm["client_secret"] = details.client_secret;
+    vm["access_token"] = details.access_token;
+    vm["token_secret"] = details.token_secret;
+    vm["error"] = details.error;
+
+    Variant var(vm);
+    JsonCppNode node(var);
+    return node.to_json_string();
+}
+
+static OnlineAccountClient::ServiceStatus json_to_details(std::string const& json)
+{
+    OnlineAccountClient::ServiceStatus details;
+    JsonCppNode node(json);
+
+    details.account_id = node.get_node("account_id")->as_int();
+    details.service_enabled = node.get_node("service_enabled")->as_bool();
+    details.service_authenticated = node.get_node("service_authenticated")->as_bool();
+    details.client_id = node.get_node("client_id")->as_string();
+    details.client_secret = node.get_node("client_secret")->as_string();
+    details.access_token = node.get_node("access_token")->as_string();
+    details.token_secret = node.get_node("token_secret")->as_string();
+    details.error = node.get_node("error")->as_string();
+
+    return details;
+}
+
+static void clear_session(AccountInfo* info)
 {
     if (info->session)
     {
@@ -96,8 +135,6 @@
         signon_auth_session_cancel(info->session.get());
     }
     info->session = nullptr;
-    info->auth_params = nullptr;
-    info->session_data = nullptr;
 }
 
 static gboolean main_loop_is_running_cb(void* user_data)
@@ -107,6 +144,13 @@
     return G_SOURCE_REMOVE;
 }
 
+static gboolean main_loop_is_stopping_cb(void* user_data)
+{
+    OnlineAccountClientImpl* account_client = reinterpret_cast<OnlineAccountClientImpl*>(user_data);
+    account_client->main_loop_state_notify(false);
+    return G_SOURCE_REMOVE;
+}
+
 static gboolean wake_up_event_loop_cb(void* user_data)
 {
     GMainLoop* event_loop = reinterpret_cast<GMainLoop*>(user_data);
@@ -116,58 +160,75 @@
 
 static void service_login_cb(GObject* source, GAsyncResult* result, void* user_data)
 {
-    GError* error = nullptr;
-    util::ResourcePtr<GError*, decltype(&g_error_free)> error_cleanup(error, free_error);
-
     SignonAuthSession* session = reinterpret_cast<SignonAuthSession*>(source);
     AccountInfo* info = reinterpret_cast<AccountInfo*>(user_data);
 
     // Get session data then send a notification with the login result
+    GError* error = nullptr;
     info->session_data.reset(signon_auth_session_process_finish(session, result, &error), free_variant);
-    info->account_client->callback(info_to_details(info, error ? error->message : ""));
-    info->account_client->dec_logins();
+    std::shared_ptr<GError> error_cleanup(error, free_error);
+
+    info->account_client->callback(info, error ? error->message : "");
+
+    // Clear session info
+    clear_session(info);
 }
 
 static void service_update_cb(AgAccountService* account_service, gboolean enabled, AccountInfo* info)
 {
-    GError* error = nullptr;
-    util::ResourcePtr<GError*, decltype(&g_error_free)> error_cleanup(error, free_error);
-
     // Service state has updated, clear the old session data
     info->service_enabled = enabled;
-    clear_session_info(info);
+    clear_session(info);
 
     if (enabled)
     {
         // Get authorization data then create a new authorization session
         std::shared_ptr<AgAuthData> auth_data(ag_account_service_get_auth_data(account_service), ag_auth_data_unref);
+
+        GError* error = nullptr;
         info->session.reset(signon_auth_session_new(
             ag_auth_data_get_credentials_id(auth_data.get()), ag_auth_data_get_method(auth_data.get()), &error), g_object_unref);
+        std::shared_ptr<GError> error_cleanup(error, free_error);
+
         if (error)
         {
             // Send notification that the authorization session failed
-            info->account_client->callback(info_to_details(info, error->message));
+            info->account_client->callback(info, error->message);
             return;
         }
 
         // Get authorization parameters then attempt to signon
+        GVariantBuilder builder;
+        g_variant_builder_init(&builder, G_VARIANT_TYPE_VARDICT);
+
+        if (info->account_client->main_loop_select() == OnlineAccountClient::RunInExternalUiMainLoop)
+        {
+            g_variant_builder_add(&builder, "{sv}",
+                                  SIGNON_SESSION_DATA_UI_POLICY,
+                                  g_variant_new_int32(SIGNON_POLICY_DEFAULT));
+        }
+        else
+        {
+            g_variant_builder_add(&builder, "{sv}",
+                                  SIGNON_SESSION_DATA_UI_POLICY,
+                                  g_variant_new_int32(SIGNON_POLICY_NO_USER_INTERACTION));
+        }
+
         info->auth_params.reset(
-            g_variant_ref_sink(ag_auth_data_get_login_parameters(auth_data.get(), nullptr)), free_variant);
+            g_variant_ref_sink(ag_auth_data_get_login_parameters(auth_data.get(), g_variant_builder_end(&builder))), free_variant);
 
-        if (info->account_client->inc_logins())
-        {
-            signon_auth_session_process_async(info->session.get(),
-                                              info->auth_params.get(),
-                                              ag_auth_data_get_mechanism(auth_data.get()),
-                                              nullptr,
-                                              service_login_cb,
-                                              info);
-        }
+        // Start signon process
+        signon_auth_session_process_async(info->session.get(),
+                                          info->auth_params.get(),
+                                          ag_auth_data_get_mechanism(auth_data.get()),
+                                          nullptr,
+                                          service_login_cb,
+                                          info);
     }
     else
     {
         // Send notification that account has been disabled
-        info->account_client->callback(info_to_details(info));
+        info->account_client->callback(info);
     }
 }
 
@@ -193,7 +254,7 @@
         if (account_client->service_name() == ag_service_get_name(service))
         {
             std::shared_ptr<AgAccountService> account_service(ag_account_service_new(account.get(), service), g_object_unref);
-            std::shared_ptr<AccountInfo> info(new AccountInfo, [](AccountInfo* info){ info->account_client = nullptr; delete info; });
+            std::shared_ptr<AccountInfo> info(new AccountInfo);
             info->service_enabled = false;
             info->account_client = account_client;
             info->account_service.reset(reinterpret_cast<AgAccountService*>(g_object_ref(account_service.get())), g_object_unref);
@@ -226,51 +287,95 @@
     : service_name_(service_name)
     , service_type_(service_type)
     , provider_name_(provider_name)
-    , use_external_main_loop_(main_loop_select == OnlineAccountClient::RunInExternalMainLoop)
-    , main_loop_is_running_(false)
-    , client_stopping_(false)
-    , logins_busy_(0)
+    , main_loop_select_(main_loop_select)
+    , main_loop_is_running_(main_loop_select != OnlineAccountClient::CreateInternalMainLoop)
 {
+    // Set up authentication pub/sub
+    std::string oa_id = provider_name + ":" + service_type + ":" + service_name;
+    RuntimeImpl::UPtr rt = RuntimeImpl::create(oa_id);
+    MiddlewareFactory mw_factory(rt.get());
+    RuntimeConfig rt_config("");
+
+    mw_ = mw_factory.create(oa_id, rt_config.default_middleware(), rt_config.default_middleware_configfile());
+    if (main_loop_select == OnlineAccountClient::RunInExternalUiMainLoop)
+    {
+        // If this client was created by the shell, set up as the publisher
+        auth_publisher_ = mw_->create_publisher(oa_id);
+    }
+    else
+    {
+        // If this client was created by a scope, set up as a subscriber
+        auth_subscriber_ = mw_->create_subscriber(oa_id);
+        auth_subscriber_->message_received().connect(std::bind(&OnlineAccountClientImpl::auth_callback, this, std::placeholders::_1));
+    }
+
     // If we are responsible for the main loop
-    if (!use_external_main_loop_)
+    if (main_loop_select_ == OnlineAccountClient::CreateInternalMainLoop)
     {
         // Wait here until either the main loop begins running, or the thread exits
-        std::unique_lock<std::mutex> lock(mutex_);
-        main_loop_thread_ = std::thread(&OnlineAccountClientImpl::main_loop_thread, this);
-        cond_.wait(lock, [this] { return main_loop_is_running_; });
-    }
-
-    manager_.reset(ag_manager_new_for_service_type(service_type_.c_str()), g_object_unref);
-
-    // Watch for changes to accounts
-    account_enabled_signal_id_ = g_signal_connect(manager_.get(), "enabled-event", G_CALLBACK(account_enabled_cb), this);
-    account_deleted_signal_id_ = g_signal_connect(manager_.get(), "account-deleted", G_CALLBACK(account_deleted_cb), this);
-
-    // Now check initial state
-    refresh_service_statuses();
+        {
+            std::unique_lock<std::mutex> lock(mutex_);
+            main_loop_thread_ = std::thread(&OnlineAccountClientImpl::main_loop_thread, this);
+            cond_.wait_for(lock, std::chrono::seconds(5), [this] { return main_loop_is_running_; });
+        }
+        if (!main_loop_is_running_)
+        {
+            if (main_loop_)
+            {
+                // Quit the main loop, causing the thread to exit
+                g_main_loop_quit(main_loop_.get());
+                if (main_loop_thread_.joinable())
+                {
+                    main_loop_thread_.join();
+                }
+            }
+            if (thread_exception_)
+            {
+                std::rethrow_exception(thread_exception_);
+            }
+            else
+            {
+                throw unity::ResourceException("OnlineAccountClientImpl(): main_loop_thread failed to start.");
+            }
+        }
+    }
+    else
+    {
+        construct();
+    }
 }
 
 OnlineAccountClientImpl::~OnlineAccountClientImpl()
 {
-    // Disconnect all signal handlers
-    g_signal_handler_disconnect(manager_.get(), account_enabled_signal_id_);
-    g_signal_handler_disconnect(manager_.get(), account_deleted_signal_id_);
-
-    for (auto info : accounts_)
-    {
-        g_signal_handler_disconnect(info.second->account_service.get(), info.second->service_update_signal_id_);
-    }
-
-    // Wait until all currently running login sessions are done
-    {
-        std::unique_lock<std::mutex> lock(mutex_);
-        client_stopping_ = true;
-    }
-    flush_pending_sessions();
-
-    // If we are responsible for the main loop, quit it on destruction
-    if (main_loop_)
-    {
+    {
+        std::lock_guard<std::mutex> lock(mutex_);
+        if (thread_exception_)
+        {
+            try
+            {
+                std::rethrow_exception(thread_exception_);
+            }
+            catch (std::exception const& e)
+            {
+                std::cerr << "~OnlineAccountClientImpl(): main_loop_thread threw an exception: " << e.what() << std::endl;
+            }
+            catch (...)
+            {
+                std::cerr << "~OnlineAccountClientImpl(): main_loop_thread threw an unknown exception" << std::endl;
+            }
+        }
+    }
+
+    // If we are responsible for the main loop
+    if (main_loop_select_ == OnlineAccountClient::CreateInternalMainLoop)
+    {
+        // Invoke tear_down() from within the main loop
+        {
+            std::unique_lock<std::mutex> lock(mutex_);
+            g_main_context_invoke(main_loop_context_.get(), main_loop_is_stopping_cb, this);
+            cond_.wait(lock, [this] { return !main_loop_is_running_; });
+        }
+
         // Quit the main loop, causing the thread to exit
         g_main_loop_quit(main_loop_.get());
         if (main_loop_thread_.joinable())
@@ -278,16 +383,32 @@
             main_loop_thread_.join();
         }
     }
+    else
+    {
+        tear_down();
+    }
 }
 
 void OnlineAccountClientImpl::set_service_update_callback(OnlineAccountClient::ServiceUpdateCallback callback)
 {
-    std::lock_guard<std::mutex> lock(callback_mutex_);
+    std::lock_guard<std::mutex> lock(mutex_);
+    if (thread_exception_)
+    {
+        std::rethrow_exception(thread_exception_);
+    }
+
+    std::lock_guard<std::mutex> callback_lock(callback_mutex_);
     callback_ = callback;
 }
 
 void OnlineAccountClientImpl::refresh_service_statuses()
 {
+    std::unique_lock<std::mutex> lock(mutex_);
+    if (thread_exception_)
+    {
+        std::rethrow_exception(thread_exception_);
+    }
+
     std::shared_ptr<GList> enabled_accounts(ag_manager_list(manager_.get()), ag_manager_list_free);
     GList* it;
     for (it = enabled_accounts.get(); it; it = it->next)
@@ -297,19 +418,25 @@
         std::string provider_name = ag_account_get_provider_name(account.get());
         if (provider_name == provider_name_)
         {
+            lock.unlock();
             account_enabled_cb(manager_.get(), account_id, this);
+            lock.lock();
         }
+        flush_pending_session(account_id, lock);
     }
-    flush_pending_sessions();
 }
 
 std::vector<OnlineAccountClient::ServiceStatus> OnlineAccountClientImpl::get_service_statuses()
 {
     std::lock_guard<std::mutex> lock(mutex_);
+    if (thread_exception_)
+    {
+        std::rethrow_exception(thread_exception_);
+    }
 
     // Return all service statuses
     std::vector<OnlineAccountClient::ServiceStatus> service_statuses;
-    for (auto const info : accounts_)
+    for (auto const& info : accounts_)
     {
         service_statuses.push_back(info_to_details(info.second.get()));
     }
@@ -352,42 +479,112 @@
     widget.add_attribute_value("online_account_details", Variant(account_details_map));
 }
 
-void OnlineAccountClientImpl::flush_pending_sessions()
-{
+void OnlineAccountClientImpl::construct()
+{
+    manager_.reset(ag_manager_new_for_service_type(service_type_.c_str()), g_object_unref);
+
+    // Watch for changes to accounts
+    account_enabled_signal_id_ = g_signal_connect(manager_.get(), "enabled-event", G_CALLBACK(account_enabled_cb), this);
+    account_deleted_signal_id_ = g_signal_connect(manager_.get(), "account-deleted", G_CALLBACK(account_deleted_cb), this);
+
+    // Now check initial state
+    refresh_service_statuses();
+}
+
+void OnlineAccountClientImpl::tear_down()
+{
+    // Disconnect signal handlers
+    g_signal_handler_disconnect(manager_.get(), account_enabled_signal_id_);
+    g_signal_handler_disconnect(manager_.get(), account_deleted_signal_id_);
+
+    // Remove all accounts
+    {
+        std::unique_lock<std::mutex> lock(mutex_);
+        for (auto const& info : accounts_)
+        {
+            // Before we nuke the map, ensure that any pending sessions are done
+            g_signal_handler_disconnect(info.second->account_service.get(), info.second->service_update_signal_id_);
+            clear_session(info.second.get());
+            flush_pending_session(info.second->account_id, lock);
+        }
+        accounts_.clear();
+    }
+}
+
+void OnlineAccountClientImpl::flush_pending_session(AgAccountId const& account_id, std::unique_lock<std::mutex>& lock)
+{
+    // Get account info
+    auto info_it = accounts_.find(account_id);
+    if (info_it == accounts_.end())
+    {
+        return;
+    }
+    auto info = info_it->second;
+
     // Wait until all currently running login sessions are done
     // (ensures that accounts_ is up to date)
-    std::unique_lock<std::mutex> lock(mutex_);
+    std::shared_ptr<GMainLoop> event_loop;
+    event_loop.reset(g_main_loop_new(g_main_context_get_thread_default(), true), g_main_loop_unref);
+    while(info->session)
     {
-        std::shared_ptr<GMainLoop> event_loop;
-        event_loop.reset(g_main_loop_new(nullptr, true), g_main_loop_unref);
-        while(logins_busy_ > 0)
-        {
-            // We need to wait inside an event loop to allow for the main application loop to
-            // process its pending events
-            g_timeout_add(100, wake_up_event_loop_cb, event_loop.get());
-            lock.unlock();
-            g_main_loop_run(event_loop.get());
-            lock.lock();
-        }
+        // We need to wait inside an event loop to allow for the main application loop to
+        // process its pending events
+        std::shared_ptr<GSource> source;
+        source.reset(g_timeout_source_new(10), g_source_unref);
+        g_source_set_callback(source.get(), wake_up_event_loop_cb, event_loop.get(), NULL);
+        g_source_attach(source.get(), g_main_context_get_thread_default());
+
+        lock.unlock();
+        g_main_loop_run(event_loop.get());
+        lock.lock();
     }
 }
 
 void OnlineAccountClientImpl::main_loop_state_notify(bool is_running)
 {
+    bool was_running = false;
+    {
+        std::lock_guard<std::mutex> lock(mutex_);
+        was_running = main_loop_is_running_;
+    }
+    if (!was_running && is_running)
+    {
+        construct();
+    }
+    else if (was_running && !is_running)
+    {
+        tear_down();
+    }
+
     std::lock_guard<std::mutex> lock(mutex_);
     main_loop_is_running_ = is_running;
     cond_.notify_all();
 }
 
+std::shared_ptr<AgManager> OnlineAccountClientImpl::manager()
+{
+    return manager_;
+}
+
 std::string OnlineAccountClientImpl::service_name()
 {
     std::lock_guard<std::mutex> lock(mutex_);
     return service_name_;
 }
 
-void OnlineAccountClientImpl::callback(OnlineAccountClient::ServiceStatus const& service_status)
+OnlineAccountClient::MainLoopSelect OnlineAccountClientImpl::main_loop_select()
+{
+    return main_loop_select_;
+}
+
+void OnlineAccountClientImpl::callback(AccountInfo const* info, std::string const& error)
 {
     std::lock_guard<std::mutex> lock(callback_mutex_);
+    auto service_status = info_to_details(info, error);
+    if (service_status.service_authenticated && auth_publisher_)
+    {
+        auth_publisher_->send_message(details_to_json(service_status));
+    }
     if (callback_)
     {
         callback_(service_status);
@@ -408,42 +605,80 @@
 
 void OnlineAccountClientImpl::remove_account(AgAccountId const& account_id)
 {
-    std::lock_guard<std::mutex> lock(mutex_);
+    std::unique_lock<std::mutex> lock(mutex_);
+
+    // Get account info
+    auto info_it = accounts_.find(account_id);
+    if (info_it == accounts_.end())
+    {
+        return;
+    }
+    auto info = info_it->second;
+
+    // Before we nuke the pointer, ensure that any pending sessions are done
+    g_signal_handler_disconnect(info->account_service.get(), info->service_update_signal_id_);
+    clear_session(info.get());
+    flush_pending_session(account_id, lock);
+
+    // Remove account info from accounts_ map
     accounts_.erase(account_id);
 }
 
-bool OnlineAccountClientImpl::inc_logins()
-{
-    std::lock_guard<std::mutex> lock(mutex_);
-    if (!client_stopping_)
-    {
-        // Increment number of logins busy
-        ++logins_busy_;
-        return true;
-    }
-    // Client is stopping so don't increment
-    return false;
-}
-
-void OnlineAccountClientImpl::dec_logins()
-{
-    // Decrement number of logins busy
-    std::lock_guard<std::mutex> lock(mutex_);
-    --logins_busy_;
-}
-
 void OnlineAccountClientImpl::main_loop_thread()
 {
-    // If something goes wrong causing the main loop not to run, the destruction of this pointer
-    // will break the constructor from its wait
-    std::shared_ptr<void> thread_exit_notifier(nullptr, [this](void*){ main_loop_state_notify(true); });
-
-    // Stick a method call into the main loop to notify the constructor when the main loop begins running
-    g_idle_add(main_loop_is_running_cb, this);
-
-    // Run the main loop
-    main_loop_.reset(g_main_loop_new(nullptr, true), g_main_loop_unref);
-    g_main_loop_run(main_loop_.get());
+    // If something goes wrong causing the thread to abort, the destruction of this pointer update the
+    // main_loop_is_running_ state accordingly.
+    std::shared_ptr<void> thread_exit_notifier(nullptr, [this](void*){ main_loop_state_notify(false); });
+
+    try
+    {
+        main_loop_context_.reset(g_main_context_new(), g_main_context_unref);
+        g_main_context_push_thread_default(main_loop_context_.get());
+
+        // Stick a method call into the main loop to notify the constructor when the main loop begins running
+        g_main_context_invoke(main_loop_context_.get(), main_loop_is_running_cb, this);
+
+        // Run the main loop
+        main_loop_.reset(g_main_loop_new(main_loop_context_.get(), true), g_main_loop_unref);
+        g_main_loop_run(main_loop_.get());
+    }
+    catch (std::exception const& e)
+    {
+        std::cerr << "OnlineAccountClientImpl::main_loop_thread(): Thread aborted: " << e.what() << std::endl;
+        std::lock_guard<std::mutex> lock(mutex_);
+        thread_exception_ = std::current_exception();
+    }
+    catch (...)
+    {
+        std::cerr << "OnlineAccountClientImpl::main_loop_thread(): Thread aborted: unknown exception" << std::endl;
+        std::lock_guard<std::mutex> lock(mutex_);
+        thread_exception_ = std::current_exception();
+    }
+}
+
+void OnlineAccountClientImpl::auth_callback(std::string const& details_json)
+{
+    OnlineAccountClient::ServiceStatus details = json_to_details(details_json);
+
+    // Update account info
+    {
+        std::lock_guard<std::mutex> lock(mutex_);
+        auto info_it = accounts_.find(details.account_id);
+        if (info_it == accounts_.end())
+        {
+            return;
+        }
+        auto info = info_it->second;
+
+        GVariantDict dict;
+        g_variant_dict_init(&dict, nullptr);
+        g_variant_dict_insert(&dict, "AccessToken", "s", details.access_token.c_str());
+        g_variant_dict_insert(&dict, "TokenSecret", "s", details.token_secret.c_str());
+        info->session_data.reset(g_variant_ref_sink(g_variant_dict_end(&dict)), free_variant);
+    }
+
+    std::lock_guard<std::mutex> lock(callback_mutex_);
+    callback_(details);
 }
 
 }  // namespace internal

=== modified file 'src/scopes/internal/RegistryObject.cpp'
--- src/scopes/internal/RegistryObject.cpp	2014-09-15 18:33:08 +0000
+++ src/scopes/internal/RegistryObject.cpp	2014-10-08 10:59:41 +0000
@@ -33,6 +33,7 @@
 #include <cassert>
 #include <fstream>
 #include <wordexp.h>
+#include <iostream>
 
 using namespace std;
 
@@ -385,7 +386,16 @@
     desktop_file << "Type=Application" << std::endl;
     desktop_file << "NoDisplay=true" << std::endl;
     desktop_file << "Name=" << metadata.display_name() << std::endl;
-    desktop_file << "Icon=" << metadata.icon() << std::endl;
+    desktop_file << "Icon=";
+    try
+    {
+        desktop_file << metadata.icon();
+    }
+    catch (NotFoundException const&)
+    {
+        // Icon is optional.
+    }
+    desktop_file << endl;
     desktop_file.close();
 }
 

=== modified file 'test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp'
--- test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp	2014-09-10 08:57:42 +0000
+++ test/gtest/scopes/OnlineAccountClient/OnlineAccountClient_test.cpp	2014-10-08 10:59:41 +0000
@@ -25,6 +25,8 @@
 
 TEST(OnlineAccountClient, basic)
 {
+    setenv("XDG_RUNTIME_DIR", "/tmp", true);
+
     OnlineAccountClient oa_client("com.ubuntu.scopes.youtube_youtube", "sharing", "google");
     auto service_statuses = oa_client.get_service_statuses();
 }