Merge lp:~vanvugt/mir/fix-1527449-Plugin into lp:mir

[Daniel van Vugt]

Avoid leaking client platform modules, and particularly ensure unused
modules are not kept resident while the client is running (LP: #1527449)

This also fixes crashing clients (LP: #1526658) due to Mesa's dlsym calls landing on the wrong symbol versions. However that fix only works for future Mir releases and not retrospectively because of the dlopen leak in client_platform_factory.cpp, which will forever exist in mesa.so.[23]

This is the Plugin version of the fix, which works reliably but is less popular.

Alternative:
https://code.launchpad.net/~vanvugt/mir/fix-1527449-make_module_ptr-2/+merge/283378

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

PASSED: Continuous integration, rev:3262
https://mir-jenkins.ubuntu.com/job/mir-ci/85/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/85/console

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

LGTM, nonblocking:

+ // Must never be inlined!
+ Plugin();

could __attribute__ (noinline) be used?

+ auto modules = mir::libraries_for_path(path,
+ *shared_library_prober_report);
could be one line.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3262
http://jenkins.qa.ubuntu.com/job/mir-ci/6056/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5588
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4495
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5544
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/298
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/381
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/381/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/381
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/381/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5541
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5541/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8000
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26785
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/294
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/294/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/150
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26795

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

review: Approve (continuous-integration)

[lp:~vanvugt/mir/fix-1527449-Plugin]

3263. By Daniel van Vugt on 2016-01-19

Merge latest trunk

3264. By Daniel van Vugt on 2016-01-20

Merge latest trunk and fix a conflict.

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

PASSED: Continuous integration, rev:3264
https://mir-jenkins.ubuntu.com/job/mir-ci/99/
Executed test runs:
    None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/99/console

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3264
http://jenkins.qa.ubuntu.com/job/mir-ci/6073/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5610
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4517
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5566
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/305
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/397
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/397/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/397
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/397/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5563
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5563/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/8013
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26833
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/301
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/301/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-xenial-touch/157
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/26844

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

We seem to be landing the alternate

Unmerged revisions

3264. By Daniel van Vugt on 2016-01-20

Merge latest trunk and fix a conflict.

3263. By Daniel van Vugt on 2016-01-19

Merge latest trunk

3262. By Daniel van Vugt on 2016-01-19

Remove redundant platform initialization added recently that was tripping
the destruction check.

3261. By Daniel van Vugt on 2016-01-19

Continue reverting back to Plugin

3260. By Daniel van Vugt on 2016-01-19

Start reverting back to the Plugin approach

3259. By Daniel van Vugt on 2016-01-19

Merge latest trunk

=== added file 'include/common/mir/plugin.h'
--- include/common/mir/plugin.h	1970-01-01 00:00:00 +0000
+++ include/common/mir/plugin.h	2016-01-20 16:21:34 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#ifndef MIR_PLUGIN_H_
+#define MIR_PLUGIN_H_
+
+#include <memory>
+
+namespace mir
+{
+
+/**
+ * Plugin is a base class designed to underpin any class whose implementation
+ * exists in a dynamically run-time loaded library.
+ * Plugin provides a foolproof check that you don't accidentally unload the
+ * library whose code you're still executing.
+ */
+class Plugin
+{
+public:
+    // Must never be inlined!
+    Plugin();
+    virtual ~Plugin() noexcept;
+
+    void keep_library_loaded(std::shared_ptr<void> const&);
+    std::shared_ptr<void> keep_library_loaded() const;
+private:
+    std::shared_ptr<void> library;
+};
+
+} // namespace mir
+
+#endif // MIR_PLUGIN_H_

=== modified file 'src/client/default_connection_configuration.cpp'
--- src/client/default_connection_configuration.cpp	2015-12-23 08:13:34 +0000
+++ src/client/default_connection_configuration.cpp	2016-01-20 16:21:34 +0000
@@ -34,11 +34,9 @@
 #include "lttng/shared_library_prober_report.h"
 #include "connection_surface_map.h"
 #include "lifecycle_control.h"
-#include "mir/shared_library.h"
 #include "mir/client_platform_factory.h"
 #include "probing_client_platform_factory.h"
 #include "mir_event_distributor.h"
-#include "mir/shared_library_prober.h"
 
 namespace mcl = mir::client;
 
@@ -47,10 +45,6 @@
 std::string const off_opt_val{"off"};
 std::string const log_opt_val{"log"};
 std::string const lttng_opt_val{"lttng"};
-
-// Shove this here until we properly manage the lifetime of our
-// loadable modules
-std::shared_ptr<mcl::ProbingClientPlatformFactory> the_platform_prober;
 }
 
 mcl::DefaultConnectionConfiguration::DefaultConnectionConfiguration(
@@ -95,22 +89,20 @@
     return client_platform_factory(
         [this]
         {
-            auto const platform_override = getenv("MIR_CLIENT_PLATFORM_LIB");
-            std::vector<std::shared_ptr<mir::SharedLibrary>> platform_plugins;
-            if (platform_override)
-            {
-                platform_plugins.push_back(std::make_shared<mir::SharedLibrary>(platform_override));
-            }
+            std::vector<std::string> libs, paths;
+            if (auto const lib = getenv("MIR_CLIENT_PLATFORM_LIB"))
+                libs.push_back(lib);
+
+            if (auto path = getenv("MIR_CLIENT_PLATFORM_PATH"))
+                paths.push_back(path);
             else
-            {
-                auto const platform_path_override = getenv("MIR_CLIENT_PLATFORM_PATH");
-                auto const platform_path = platform_path_override ? platform_path_override : MIR_CLIENT_PLATFORM_PATH;
-                platform_plugins = mir::libraries_for_path(platform_path, *the_shared_library_prober_report());
-            }
-
-            the_platform_prober = std::make_shared<mcl::ProbingClientPlatformFactory>(platform_plugins);
-
-            return the_platform_prober;
+                paths.push_back(MIR_CLIENT_PLATFORM_PATH);
+
+            return std::make_shared<mcl::ProbingClientPlatformFactory>(
+                                         the_shared_library_prober_report(),
+                                         libs,
+                                         paths
+                                         );
         });
 }
 

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2016-01-20 00:44:40 +0000
+++ src/client/mir_connection.cpp	2016-01-20 16:21:34 +0000
@@ -521,6 +521,7 @@
             };
 
         platform = client_platform_factory->create_client_platform(this);
+        platform_library = platform->keep_library_loaded();
         native_display = platform->create_egl_native_display();
         display_configuration->set_configuration(connect_result->display_configuration());
         lifecycle_control->set_callback(default_lifecycle_event_handler);

=== modified file 'src/client/mir_connection.h'
--- src/client/mir_connection.h	2016-01-18 15:54:40 +0000
+++ src/client/mir_connection.h	2016-01-20 16:21:34 +0000
@@ -216,6 +216,9 @@
 
     mutable std::mutex mutex; // Protects all members of *this (except release_wait_handles)
 
+    // Destruct this last as it will unmap the platform library from memory.
+    std::shared_ptr<void> platform_library;
+
     std::shared_ptr<mir::client::ConnectionSurfaceMap> surface_map;
     std::shared_ptr<mir::client::rpc::MirBasicRpcChannel> const channel;
     mir::client::rpc::DisplayServer server;

=== modified file 'src/client/probing_client_platform_factory.cpp'
--- src/client/probing_client_platform_factory.cpp	2016-01-05 10:23:12 +0000
+++ src/client/probing_client_platform_factory.cpp	2016-01-20 16:21:34 +0000
@@ -1,23 +1,60 @@
 #include "probing_client_platform_factory.h"
+#include "mir/client_platform.h"
+#include "mir/shared_library.h"
+#include "mir/shared_library_prober.h"
+#include "mir/shared_library_prober_report.h"
 
 #include <boost/exception/all.hpp>
 #include <stdexcept>
 
 namespace mcl = mir::client;
 
-mcl::ProbingClientPlatformFactory::ProbingClientPlatformFactory(std::vector<std::shared_ptr<mir::SharedLibrary>> const& modules)
-    : platform_modules{modules}
+mcl::ProbingClientPlatformFactory::ProbingClientPlatformFactory(
+    std::shared_ptr<mir::SharedLibraryProberReport> const& rep,
+    StringList const& force_libs,
+    StringList const& lib_paths)
+    : shared_library_prober_report{rep},
+      platform_overrides{force_libs},
+      platform_paths{lib_paths}
 {
-    if (modules.empty())
+    if (platform_overrides.empty() && platform_paths.empty())
     {
         BOOST_THROW_EXCEPTION(
-            std::runtime_error{"Attempted to create a ClientPlatformFactory with no platform modules"});
+            std::runtime_error{"Attempted to create a ClientPlatformFactory with no platform paths or overrides"});
     }
 }
 
 std::shared_ptr<mcl::ClientPlatform>
 mcl::ProbingClientPlatformFactory::create_client_platform(mcl::ClientContext* context)
 {
+    // Note we don't want to keep unused platform modules loaded any longer
+    // than it takes to choose the right one. So this list is local:
+    std::vector<std::shared_ptr<mir::SharedLibrary>> platform_modules;
+
+    if (!platform_overrides.empty())
+    {
+        // Even forcing a choice, platform is only loaded on demand. It's good
+        // to not need to hold the module open when there are no connections.
+        // Also, this allows you to swap driver binaries on disk at run-time,
+        // if you really wanted to.
+
+        for (auto const& platform : platform_overrides)
+            platform_modules.push_back(
+                std::make_shared<mir::SharedLibrary>(platform));
+    }
+    else
+    {
+        for (auto const& path : platform_paths)
+        {
+            // This is sub-optimal. We shouldn't need to have all drivers
+            // loaded simultaneously, but we do for now due to this API:
+            auto modules = mir::libraries_for_path(path,
+                                               *shared_library_prober_report);
+            for (auto const& module : modules)
+                 platform_modules.push_back(module);
+        }
+    }
+
     for (auto& module : platform_modules)
     {
         try
@@ -26,7 +63,9 @@
             if (probe(context))
             {
                 auto factory = module->load_function<mir::client::CreateClientPlatform>("create_client_platform", CLIENT_PLATFORM_VERSION);
-                return factory(context);
+                auto platform = factory(context);
+                platform->keep_library_loaded(module);
+                return platform;
             }
         }
         catch(std::runtime_error const&)

=== modified file 'src/client/probing_client_platform_factory.h'
--- src/client/probing_client_platform_factory.h	2015-01-21 07:34:50 +0000
+++ src/client/probing_client_platform_factory.h	2016-01-20 16:21:34 +0000
@@ -8,16 +8,25 @@
 
 namespace mir
 {
+class SharedLibraryProberReport;
+
 namespace client
 {
 class ProbingClientPlatformFactory : public ClientPlatformFactory
 {
 public:
-    ProbingClientPlatformFactory(std::vector<std::shared_ptr<SharedLibrary>> const& modules);
+    using StringList = std::vector<std::string>;
+    ProbingClientPlatformFactory(
+        std::shared_ptr<mir::SharedLibraryProberReport> const& rep,
+        StringList const& force_libs,
+        StringList const& lib_paths);
 
     std::shared_ptr<ClientPlatform> create_client_platform(ClientContext *context) override;
+
 private:
-    std::vector<std::shared_ptr<SharedLibrary>> platform_modules;
+    std::shared_ptr<mir::SharedLibraryProberReport> const shared_library_prober_report;
+    StringList const platform_overrides;
+    StringList const platform_paths;
 };
 
 }

=== modified file 'src/common/sharedlibrary/CMakeLists.txt'
--- src/common/sharedlibrary/CMakeLists.txt	2015-06-17 05:20:42 +0000
+++ src/common/sharedlibrary/CMakeLists.txt	2016-01-20 16:21:34 +0000
@@ -18,6 +18,7 @@
   module_deleter.cpp
   shared_library.cpp
   shared_library_prober.cpp
+  plugin.cpp
 )
 
 list(APPEND MIR_COMMON_SOURCES

=== added file 'src/common/sharedlibrary/plugin.cpp'
--- src/common/sharedlibrary/plugin.cpp	1970-01-01 00:00:00 +0000
+++ src/common/sharedlibrary/plugin.cpp	2016-01-20 16:21:34 +0000
@@ -0,0 +1,50 @@
+/*
+ * Copyright © 2015 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser 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 warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Daniel van Vugt <[e-mail]>
+ */
+
+#include "mir/plugin.h"
+#include "mir/fatal.h"
+
+namespace mir
+{
+
+Plugin::Plugin()
+{
+}
+
+Plugin::~Plugin() noexcept
+{
+    // This is not a recoverable error. If we don't exit now, we'll just
+    // end up with a worse crash where we have unloaded the library we're
+    // executing in, and so get no stack trace at all...
+    if (library.unique())
+        mir::fatal_error_abort("Attempted to unload a plugin library "
+                               "while it's still in use (~Plugin not "
+                               "completed yet)");
+}
+
+void Plugin::keep_library_loaded(std::shared_ptr<void> const& lib)
+{
+    library = lib;
+}
+
+std::shared_ptr<void> Plugin::keep_library_loaded() const
+{
+    return library;
+}
+
+} // namespace mir

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2016-01-08 09:50:51 +0000
+++ src/common/symbols.map	2016-01-20 16:21:34 +0000
@@ -208,6 +208,12 @@
       typeinfo?for?mir::time::SteadyClock;
       typeinfo?for?android::InputChannel;
       typeinfo?for?mir::logging::DumbConsoleLogger;
+
       mir::detail::libname_impl*;
+
+      typeinfo?for?mir::Plugin;
+      mir::Plugin::Plugin*;
+      mir::Plugin::?Plugin*;
+      mir::Plugin::keep_library_loaded*;
     };
 } MIR_COMMON_5.1;

=== modified file 'src/include/client/mir/client_platform.h'
--- src/include/client/mir/client_platform.h	2015-12-23 06:41:56 +0000
+++ src/include/client/mir/client_platform.h	2016-01-20 16:21:34 +0000
@@ -21,6 +21,7 @@
 #include "mir/graphics/native_buffer.h"
 #include "mir_toolkit/client_types.h"
 #include "mir_toolkit/mir_native_buffer.h"
+#include "mir/plugin.h"
 
 #include <EGL/eglplatform.h>
 #include <EGL/egl.h>  // for EGLConfig
@@ -38,7 +39,7 @@
  * Interface to client-side platform specific support for graphics operations.
  * \ingroup platform_enablement
  */
-class ClientPlatform
+class ClientPlatform : public mir::Plugin
 {
 public:
     ClientPlatform() = default;

=== modified file 'src/platforms/mesa/client/client_platform_factory.cpp'
--- src/platforms/mesa/client/client_platform_factory.cpp	2015-11-25 20:26:59 +0000
+++ src/platforms/mesa/client/client_platform_factory.cpp	2016-01-20 16:21:34 +0000
@@ -35,14 +35,18 @@
 
 namespace
 {
-// Hack around the way mesa loads mir: This hack makes the
-// necessary symbols global.
+// Re-export our own symbols from mesa.so.N globally so that Mesa itself can
+// find them with a simple dlsym(NULL,)
 void ensure_loaded_with_rtld_global_mesa_client()
 {
     Dl_info info;
 
     dladdr(reinterpret_cast<void*>(&ensure_loaded_with_rtld_global_mesa_client), &info);
-    dlopen(info.dli_fname,  RTLD_NOW | RTLD_NOLOAD | RTLD_GLOBAL);
+    void* reexport_self_global =
+        dlopen(info.dli_fname, RTLD_NOW | RTLD_NOLOAD | RTLD_GLOBAL);
+    // Yes, RTLD_NOLOAD does increase the ref count. So dlclose...
+    if (reexport_self_global)
+        dlclose(reexport_self_global);
 }
 
 struct RealBufferFileOps : public mclm::BufferFileOps

=== modified file 'tests/mir_test_framework/CMakeLists.txt'
--- tests/mir_test_framework/CMakeLists.txt	2016-01-05 19:14:36 +0000
+++ tests/mir_test_framework/CMakeLists.txt	2016-01-20 16:21:34 +0000
@@ -93,6 +93,7 @@
   mirclientplatformstub
 
   mir-test-framework-static
+  mircommon
   ${UMOCKDEV_LDFLAGS} ${UMOCKDEV_LIBRARIES}
 )
 

=== modified file 'tests/unit-tests/client/test_probing_client_platform_factory.cpp'
--- tests/unit-tests/client/test_probing_client_platform_factory.cpp	2015-09-30 19:34:21 +0000
+++ tests/unit-tests/client/test_probing_client_platform_factory.cpp	2016-01-20 16:21:34 +0000
@@ -18,6 +18,7 @@
 
 #include "mir/client_platform.h"
 #include "src/client/probing_client_platform_factory.h"
+#include "src/server/report/null_report_factory.h"
 
 #include "mir/test/doubles/mock_client_context.h"
 #include "mir_test_framework/executable_path.h"
@@ -25,30 +26,57 @@
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
+#include <dlfcn.h>
 
 namespace mtf = mir_test_framework;
 namespace mtd = mir::test::doubles;
 
 namespace
 {
-std::vector<std::shared_ptr<mir::SharedLibrary>>
+std::vector<std::string>
 all_available_modules()
 {
-    std::vector<std::shared_ptr<mir::SharedLibrary>> modules;
+    std::vector<std::string> modules;
 #if defined(MIR_BUILD_PLATFORM_MESA_KMS) || defined(MIR_BUILD_PLATFORM_MESA_X11)
-    modules.push_back(std::make_shared<mir::SharedLibrary>(mtf::client_platform("mesa")));
+    modules.push_back(mtf::client_platform("mesa"));
 #endif
 #ifdef MIR_BUILD_PLATFORM_ANDROID
-    modules.push_back(std::make_shared<mir::SharedLibrary>(mtf::client_platform("android")));
+    modules.push_back(mtf::client_platform("android"));
 #endif
     return modules;
 }
+
+bool loaded(std::string const& path)
+{
+    void* x = dlopen(path.c_str(), RTLD_LAZY | RTLD_NOLOAD);
+    if (x)
+        dlclose(x);
+    return !!x;
+}
+
+void populate_valid(MirPlatformPackage& pkg)
+{
+    memset(&pkg, 0, sizeof(MirPlatformPackage));
+    pkg.fd_items = 1;
+    pkg.fd[0] = 23;
+}
+
+void safely_unload(std::shared_ptr<mir::client::ClientPlatform>& platform)
+{
+    ASSERT_TRUE(platform.unique());
+    auto library = platform->keep_library_loaded();
+    platform.reset();
+    library.reset();
+}
+
 }
 
 TEST(ProbingClientPlatformFactory, ThrowsErrorWhenConstructedWithNoPlatforms)
 {
     std::vector<std::shared_ptr<mir::SharedLibrary>> empty_modules;
-    EXPECT_THROW(mir::client::ProbingClientPlatformFactory{empty_modules},
+    EXPECT_THROW(mir::client::ProbingClientPlatformFactory(
+                     mir::report::null_shared_library_prober_report(),
+                     {}, {}),
                  std::runtime_error);
 }
 
@@ -56,7 +84,10 @@
 {
     using namespace testing;
 
-    mir::client::ProbingClientPlatformFactory factory{all_available_modules()};
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        all_available_modules(),
+        {});
 
     mtd::MockClientContext context;
     ON_CALL(context, populate_server_package(_))
@@ -73,6 +104,98 @@
                  std::runtime_error);
 }
 
+TEST(ProbingClientPlatformFactory, DoesNotLeakTheUsedDriverModuleOnShutdown)
+{   // Regression test for LP: #1527449
+    using namespace testing;
+    auto const modules = all_available_modules();
+    ASSERT_FALSE(modules.empty());
+    std::string const preferred_module = modules.front();
+
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        {preferred_module},
+        {});
+
+    std::shared_ptr<mir::client::ClientPlatform> platform;
+    mtd::MockClientContext context;
+    ON_CALL(context, populate_server_package(_))
+            .WillByDefault(Invoke(populate_valid));
+
+    ASSERT_FALSE(loaded(preferred_module));
+    platform = factory.create_client_platform(&context);
+    ASSERT_TRUE(loaded(preferred_module));
+    safely_unload(platform);
+    EXPECT_FALSE(loaded(preferred_module));
+}
+
+TEST(ProbingClientPlatformFactory, DoesNotLeakUnusedDriverModulesOnStartup)
+{   // Regression test for LP: #1527449 and LP: #1526658
+    using namespace testing;
+    auto const modules = all_available_modules();
+    ASSERT_FALSE(modules.empty());
+
+    // Note: This test is only really effective with nmodules>1, which many of
+    //       our builds will have. But nmodules==1 is harmless.
+
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        modules,
+        {});
+
+    std::shared_ptr<mir::client::ClientPlatform> platform;
+    mtd::MockClientContext context;
+    ON_CALL(context, populate_server_package(_))
+            .WillByDefault(Invoke(populate_valid));
+
+    int nloaded = 0;
+    for (auto const& m : modules)
+        if (loaded(m)) ++nloaded;
+    ASSERT_EQ(0, nloaded);
+
+    platform = factory.create_client_platform(&context);
+
+    nloaded = 0;
+    for (auto const& m : modules)
+        if (loaded(m)) ++nloaded;
+    EXPECT_EQ(1, nloaded);  // expect not assert, because we need safely_unload
+
+    safely_unload(platform);
+
+    nloaded = 0;
+    for (auto const& m : modules)
+        if (loaded(m)) ++nloaded;
+    ASSERT_EQ(0, nloaded);
+}
+
+// Note "DeathTest" informs our scripts that we expect a leak from the
+// child process that died, and to ignore it.
+TEST(ProbingClientPlatformFactoryDeathTest, DiesOnUnsafeRelease)
+{
+    using namespace testing;
+    auto const modules = all_available_modules();
+    ASSERT_FALSE(modules.empty());
+    std::string const preferred_module = modules.front();
+
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        {preferred_module},
+        {});
+
+    std::shared_ptr<mir::client::ClientPlatform> platform;
+    mtd::MockClientContext context;
+    ON_CALL(context, populate_server_package(_))
+            .WillByDefault(Invoke(populate_valid));
+
+    platform = factory.create_client_platform(&context);
+
+    // Google Test creates a child process to verify this:
+    ASSERT_DEATH({platform.reset();}, ".*still in use.*");
+
+    // ... but here in the parent process we need to actively avoid the
+    // death happening, so that our test completes:
+    safely_unload(platform);
+}
+
 #if defined(MIR_BUILD_PLATFORM_MESA_KMS) || defined(MIR_BUILD_PLATFORM_MESA_X11)
 TEST(ProbingClientPlatformFactory, CreatesMesaPlatformWhenAppropriate)
 #else
@@ -81,7 +204,10 @@
 {
     using namespace testing;
 
-    mir::client::ProbingClientPlatformFactory factory{all_available_modules()};
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        all_available_modules(),
+        {});
 
     mtd::MockClientContext context;
     ON_CALL(context, populate_server_package(_))
@@ -95,6 +221,7 @@
                            }));
     auto platform = factory.create_client_platform(&context);
     EXPECT_EQ(mir_platform_type_gbm, platform->platform_type());
+    safely_unload(platform);
 }
 
 #ifdef MIR_BUILD_PLATFORM_ANDROID
@@ -105,7 +232,10 @@
 {
     using namespace testing;
 
-    mir::client::ProbingClientPlatformFactory factory{all_available_modules()};
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        all_available_modules(),
+        {});
 
     mtd::MockClientContext context;
     ON_CALL(context, populate_server_package(_))
@@ -118,6 +248,7 @@
 
     auto platform = factory.create_client_platform(&context);
     EXPECT_EQ(mir_platform_type_android, platform->platform_type());
+    safely_unload(platform);
 }
 
 TEST(ProbingClientPlatformFactory, IgnoresNonClientPlatformModules)
@@ -126,10 +257,13 @@
 
     auto modules = all_available_modules();
     // NOTE: For minimum fuss, load something that has minimal side-effects...
-    modules.push_back(std::make_shared<mir::SharedLibrary>("libc.so.6"));
-    modules.push_back(std::make_shared<mir::SharedLibrary>(mtf::client_platform("dummy.so")));
+    modules.push_back("libc.so.6");
+    modules.push_back(mtf::client_platform("dummy.so"));
 
-    mir::client::ProbingClientPlatformFactory factory{modules};
+    mir::client::ProbingClientPlatformFactory factory(
+        mir::report::null_shared_library_prober_report(),
+        modules,
+        {});
 
     mtd::MockClientContext context;
     ON_CALL(context, populate_server_package(_))
@@ -139,4 +273,5 @@
                            }));
 
     auto platform = factory.create_client_platform(&context);
+    safely_unload(platform);
 }