Merge lp://qastaging/~afrantzis/mir/fix-1358191-connect-segv-spike into lp://qastaging/mir

PASSED: Continuous integration, rev:1861
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2495/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1449
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1455
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1434
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1017
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1017/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/371
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/371/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2515
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12048

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2495/rebuild

I've been pondering writing a Module class to properly reference-count all the things we load from libraries. That'd be significantly more invasive, though.

Of course, we could also just remove the dlclose() from ~SharedLibrary, and remove the SharedLibraryCache thing entirely. There's no reason to avoid dlopen()ing a library twice, and we never load an unnecessary one so the only thing dlclose() buys us is these bugs.

This seems like a reasonable approach, though.

> Of course, we could also just remove the dlclose() from ~SharedLibrary, and remove the
> SharedLibraryCache thing entirely. There's no reason to avoid dlopen()ing a library twice,
> and we never load an unnecessary one so the only thing dlclose() buys us is these bugs.

That's a fair point. If the only reason we want the cache is teardown cleanliness (i.e. explicitly unloading libraries that we loaded explicitly), then just keeping the libraries always loaded is also viable option (in that case a class name like PermanentSharedLibrary would be more descriptive).

Well, I do actually use the unloadability in https://code.launchpad.net/~raof/mir/privatise-all-the-things/+merge/228796 - we load everything to probe it, and then want to unload the platforms that didn't probe.

Hm. Having said that, just making the unload explicit would be fine - the only time we need to unload things is in the location that we open them, so reference-counted SharedLibraries aren't a big win.

> we load everything to probe it, and then want to unload the platforms that didn't probe.

Then perhaps we can keep SharedLibrary as it is and add a SharedLibrary::make_permanent() member that ensures that the library is not unloaded on object destruction (or we could have SharedLibrary and PermanentSharedLibrary, or ... many options here :))

I don't think we should be coupling SharedLibrary to a cache interface.

Indeed wouldn't placing a cache implementation directly in the configuration object be sufficient?

> Indeed wouldn't placing a cache implementation directly in the configuration
> object be sufficient?

After a bit of discussion on IRC I see this wouldn't work.

But that also leads to the conclusion that the condition in...

    // When the last valid connection goes we can clear the libraries cache
    if (!valid_connections)
    {
        delete libraries_cache_ptr;
        libraries_cache_ptr = nullptr;
    }

...is wrong - as the lack of valid connections isn't a sufficient criteria for releasing the cache. Maybe we can just fix that condition to also consider the configuration.

> I don't think we should be coupling SharedLibrary to a cache interfac

Oops meant to update the vote

Superseded by https://code.launchpad.net/~afrantzis/mir/fix-1358191-connect-segv/+merge/232218