Merge lp:~mir-team/mir/enable-late-release-0.5-backport into lp:mir/0.5

[Alan Griffiths]

client: don't allow shared libraries to be unloaded before unity-mir calls connection_release()

client: don't allow shared libraries to be unloaded before unity-mir calls connection_release()

[lp:~mir-team/mir/enable-late-release-0.5-backport]

1789. By Alan Griffiths on 2014-07-17

merge -c 1776 lp:~mir-team/mir/enable-late-release

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1788
http://jenkins.qa.ubuntu.com/job/mir-0.5-ci/2/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-amd64-ci/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-armhf-ci/2/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/982
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/988
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/977/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3289/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1789
http://jenkins.qa.ubuntu.com/job/mir-0.5-ci/3/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-amd64-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-amd64-ci/3/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-armhf-ci/3
        deb: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-armhf-ci/3/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/985
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/991
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/980/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3297
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3297/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2107/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10012

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> I: stopping unity8
> Build timed out (after 60 minutes). Marking the build as failed.
> Build was aborted

Looks unrelated - triggering rebuild

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1789
http://jenkins.qa.ubuntu.com/job/mir-0.5-ci/4/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-amd64-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-amd64-ci/4/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-armhf-ci/4
        deb: http://jenkins.qa.ubuntu.com/job/mir-0.5-utopic-armhf-ci/4/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/991
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/997
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/986/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3333
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/3333/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2114/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/10052

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> > I: stopping unity8
> > Build timed out (after 60 minutes). Marking the build as failed.
> > Build was aborted
>
> Looks unrelated - triggering rebuild

Hmm. Happened twice on different makos.

Unmerged revisions

1789. By Alan Griffiths on 2014-07-17

merge -c 1776 lp:~mir-team/mir/enable-late-release

1788. By Alan Griffiths on 2014-07-17

merge -r1771..1775 lp:~mir-team/mir/enable-late-release

1787. By Alan Griffiths on 2014-07-17

merge -r1766..1770 lp:~mir-team/mir/enable-late-release

1786. By Alan Griffiths on 2014-07-17

merge lp:mir/0.5

1785. By Alan Griffiths on 2014-07-16

TODO notices

1784. By Alan Griffiths on 2014-07-16

Ensure default_display_container live a bit longer

1783. By Alan Griffiths on 2014-07-16

Ensure default_display_container live a bit longer

1782. By Alan Griffiths on 2014-07-16

Ensure loaded libraries live a bit longer

1781. By Alan Griffiths on 2014-07-16

avoid namespace scope objects

1780. By Daniel van Vugt on 2014-07-16

Mention another bug fix we forgot - LP: #1189775

=== modified file 'examples/basic.c'
--- examples/basic.c	2014-03-06 06:05:17 +0000
+++ examples/basic.c	2014-07-17 13:57:55 +0000
@@ -88,11 +88,25 @@
 }
 ///\internal [Callback_tag]
 
+MirDemoState mcd = {0, 0};
+
+static void close_connection()
+{
+    if (mcd.connection)
+    {
+        ///\internal [connection_release_tag]
+        mir_connection_release(mcd.connection);
+        puts("Connection released");
+        ///\internal [connection_release_tag]
+    }
+}
+
 void demo_client(const char* server, int buffer_swap_count)
 {
-    MirDemoState mcd;
     mcd.connection = 0;
     mcd.surface = 0;
+    // We should always release our connection
+    atexit(&close_connection);
 
     puts("Starting");
 
@@ -182,12 +196,6 @@
     mir_wait_for(mir_surface_release(mcd.surface, surface_release_callback, &mcd));
     puts("Surface released");
     ///\internal [surface_release_tag]
-
-    ///\internal [connection_release_tag]
-    // We should release our connection
-    mir_connection_release(mcd.connection);
-    puts("Connection released");
-    ///\internal [connection_release_tag]
 }
 
 // The main() function deals with parsing arguments and defaults

=== modified file 'src/client/android/android_native_display_container.cpp'
--- src/client/android/android_native_display_container.cpp	2014-07-16 16:28:55 +0000
+++ src/client/android/android_native_display_container.cpp	2014-07-17 13:57:55 +0000
@@ -20,16 +20,35 @@
 
 #include "mir_toolkit/mir_client_library.h"
 
+#include <mutex>
+
 namespace mcl = mir::client;
 namespace mcla = mcl::android;
 
-// TODO making this namespace scope extends its life slightly (and avoids a potential crash)
-// TODO but we should manage it and ensure it is only released after the last connection dies
-static mcla::AndroidNativeDisplayContainer default_display_container;
+namespace
+{
+// default_display_container needs to live until the library is unloaded
+std::mutex default_display_container_mutex;
+mcla::AndroidNativeDisplayContainer* default_display_container{nullptr};
+
+extern "C" int __attribute__((destructor)) destroy()
+{
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    delete default_display_container;
+
+    return 0;
+}
+}
 
 mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance()
 {
-    return default_display_container;
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    if (!default_display_container)
+        default_display_container = new mcla::AndroidNativeDisplayContainer;
+
+    return *default_display_container;
 }
 
 mcla::AndroidNativeDisplayContainer::AndroidNativeDisplayContainer()

=== modified file 'src/client/connection_configuration.h'
--- src/client/connection_configuration.h	2014-05-29 13:21:08 +0000
+++ src/client/connection_configuration.h	2014-07-17 13:57:55 +0000
@@ -39,8 +39,11 @@
 class Logger;
 }
 
+class SharedLibrary;
+
 namespace client
 {
+std::shared_ptr<SharedLibrary>& libraries_cache(std::string const& lib_name);
 
 class ConnectionSurfaceMap;
 class Logger;
@@ -70,7 +73,6 @@
     ConnectionConfiguration(ConnectionConfiguration const&) = delete;
     ConnectionConfiguration& operator=(ConnectionConfiguration const&) = delete;
 };
-
 }
 }
 

=== modified file 'src/client/default_connection_configuration.cpp'
--- src/client/default_connection_configuration.cpp	2014-07-16 16:28:55 +0000
+++ src/client/default_connection_configuration.cpp	2014-07-17 13:57:55 +0000
@@ -43,15 +43,10 @@
 std::string const lttng_opt_val{"lttng"};
 std::string const default_platform_lib{"libmirclientplatform.so"};
 
-// TODO making this namespace scope extends its life slightly (and avoids a potential crash)
-// TODO but we should manage it and ensure it is only released after the last connection dies
-static std::map<std::string, std::shared_ptr<mir::SharedLibrary>> libraries_cache;
-
 mir::SharedLibrary const* load_library(std::string const& libname)
 {
-    // There's no point in loading twice, and it isn't safe to unload...
 
-    if (auto& ptr = libraries_cache[libname])
+    if (auto& ptr = mcl::libraries_cache(libname))
     {
         return ptr.get();
     }

=== modified file 'src/client/mesa/mesa_native_display_container.cpp'
--- src/client/mesa/mesa_native_display_container.cpp	2014-07-16 16:28:55 +0000
+++ src/client/mesa/mesa_native_display_container.cpp	2014-07-17 13:57:55 +0000
@@ -46,15 +46,30 @@
 }
 
 }
-}
-
-// TODO making this namespace scope extends its life slightly (and avoids a potential crash)
-// TODO but we should manage it and ensure it is only released after the last connection dies
-static mclm::MesaNativeDisplayContainer default_display_container;
+
+// default_display_container needs to live until the library is unloaded
+std::mutex default_display_container_mutex;
+mclm::MesaNativeDisplayContainer* default_display_container{nullptr};
+
+extern "C" int __attribute__((destructor)) destroy()
+{
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    delete default_display_container;
+
+    return 0;
+}
+}
+
 
 mcl::EGLNativeDisplayContainer& mcl::EGLNativeDisplayContainer::instance()
 {
-    return default_display_container;
+    std::lock_guard<std::mutex> lock(default_display_container_mutex);
+
+    if (!default_display_container)
+        default_display_container = new mclm::MesaNativeDisplayContainer;
+
+    return *default_display_container;
 }
 
 mclm::MesaNativeDisplayContainer::MesaNativeDisplayContainer()

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2014-07-16 15:10:56 +0000
+++ src/client/mir_connection.cpp	2014-07-17 13:57:55 +0000
@@ -63,11 +63,48 @@
     MirDisplayConfiguration* const config;
 };
 
+using LibrariesCache = std::map<std::string, std::shared_ptr<mir::SharedLibrary>>;
+
 std::mutex connection_guard;
 MirConnection* valid_connections{nullptr};
+// There's no point in loading twice, and it isn't safe to
+// unload while there are valid connections
+std::map<std::string, std::shared_ptr<mir::SharedLibrary>>* libraries_cache_ptr{nullptr};
+}
+
+std::shared_ptr<mir::SharedLibrary>& mcl::libraries_cache(std::string const& libname)
+{
+    std::lock_guard<std::mutex> lock(connection_guard);
+
+    if (!libraries_cache_ptr)
+        libraries_cache_ptr = new LibrariesCache;
+
+    return (*libraries_cache_ptr)[libname];
+}
+
+MirConnection::Deregisterer::~Deregisterer()
+{
+    std::lock_guard<std::mutex> lock(connection_guard);
+
+    for (auto current = &valid_connections; *current; current = &(*current)->next_valid)
+    {
+        if (self == *current)
+        {
+            *current = self->next_valid;
+            break;
+        }
+    }
+
+    // When the last valid connection goes we can clear the libraries cache
+    if (!valid_connections)
+    {
+        delete libraries_cache_ptr;
+        libraries_cache_ptr = nullptr;
+    }
 }
 
 MirConnection::MirConnection(std::string const& error_message) :
+    deregisterer{this},
     channel(),
     server(0),
     error_message(error_message)
@@ -76,6 +113,7 @@
 
 MirConnection::MirConnection(
     mir::client::ConnectionConfiguration& conf) :
+        deregisterer{this},
         channel(conf.the_rpc_channel()),
         server(channel.get(), ::google::protobuf::Service::STUB_DOESNT_OWN_CHANNEL),
         logger(conf.the_logger()),
@@ -100,19 +138,6 @@
     // But, if after 500ms we don't get a call, assume it won't happen.
     connect_wait_handle.wait_for_pending(std::chrono::milliseconds(500));
 
-    {
-        std::lock_guard<std::mutex> lock(connection_guard);
-
-        for (auto current = &valid_connections; *current; current = &(*current)->next_valid)
-        {
-            if (this == *current)
-            {
-                *current = next_valid;
-                break;
-            }
-        }
-    }
-
     std::lock_guard<decltype(mutex)> lock(mutex);
     if (connect_result.has_platform())
     {

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2014-07-16 15:10:56 +0000
+++ src/client/mir_connection.h	2014-07-17 13:57:55 +0000
@@ -134,6 +134,10 @@
     mir::protobuf::DisplayServer& display_server();
 
 private:
+    // MUST be first data member so it is destroyed last.
+    struct Deregisterer
+    { MirConnection* const self; ~Deregisterer(); } deregisterer;
+
     std::mutex mutex; // Protects all members of *this (except release_wait_handles)
 
     std::shared_ptr<google::protobuf::RpcChannel> const channel;