Merge lp:~vanvugt/mir/fix-1358191 into lp:mir

[Daniel van Vugt]

Avoid confusing weak/shared pointer ownership. In the case that
connect() fails and throws an exception, make sure the shared_ptr to your
ClientPlatformFactory (inside MirConnection) lives longer than the original
weak_ptr to it (inside conf). If you let the shared_ptr destruct first in
this case, the weak_ptr it came from somehow loses track and tries to
delete the object a second time, hence crashing (LP: #1358191)

Unfortunately testing this fix is hit and miss. For several hours today I could reproduce the bug reliably and verified this proposal does fix it. Other times (like all of Friday and now this evening) the bug stops happening so you can't be sure at all.

If this isn't a compiler bug then I can only assume there are compliant cases where weak_ptr stops communicating properly with shared_ptr's of the same object. I'm not aware of any way C++ allows that intentionally though.

Safe to say I'm still confused by this and hoping someone finds a simpler explanation before I wake up...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1862
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2492/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1445
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1451
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1430
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1014
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/1014/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/367
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/367/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2511
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12030

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

See https://code.launchpad.net/~afrantzis/mir/fix-1358191-connect-segv-spike/+merge/232108 for an MP that tries to address the core problem behind this bug.

[Alan Griffiths (alan-griffiths)]

> See https://code.launchpad.net/~afrantzis/mir/fix-1358191-connect-segv-
> spike/+merge/232108 for an MP that tries to address the core problem behind
> this bug.

That's a better approach

review: Disapprove

=== modified file 'src/client/mir_connection_api.cpp'
--- src/client/mir_connection_api.cpp	2014-08-11 21:56:45 +0000
+++ src/client/mir_connection_api.cpp	2014-08-25 11:11:24 +0000
@@ -58,6 +58,8 @@
         mir_connected_callback callback,
         void* context) override
     {
+        MirConnection* connection = nullptr;
+
         try
         {
             std::string sock;
@@ -74,13 +76,26 @@
 
             auto const conf = configuration(sock);
 
-            std::unique_ptr<MirConnection> connection{new MirConnection(*conf)};
+            // XXX Beware bug LP: #1358191
+            //     connection must not be a unique or shared pointer object!
+            //     If connect() throws then connection needs to live longer than
+            //     conf. Otherwise connection will delete its
+            //     client_platform_factory and conf's Cached/weak pointers to
+            //     the same factory will somehow remain valid, resulting in
+            //     a second spurious delete of the already deleted object.
+            //     This may be a compiler bug, or maybe the breakdown in
+            //     communication between shared and weak is correct in this
+            //     case. Not sure.
+            //     So long as you let conf destruct first, then everything
+            //     will be OK.
+            connection = new MirConnection(*conf);
+
             auto const result = connection->connect(name, callback, context);
-            connection.release();
             return result;
         }
         catch (std::exception const& x)
         {
+            delete connection;
             MirConnection* error_connection = new MirConnection(x.what());
             mcl::ErrorConnections::instance().insert(error_connection);
             callback(error_connection, context);