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

[Daniel van Vugt]

Fix client memory leak that is causing sporadic CI test failures in and around
TestClientInput/DemoPrivateProtobuf. (LP: #1295231)

The leak was a simple mistake in the client library. It was triggered by
DemoPrivateProtobuf tests being strangely racy (not fixed here). However
valgrind doesn't detect that as a leak until the next multi-process test
completes. So it would fail typically during TestClientInput.

[Alan Griffiths (alan-griffiths)]

8 + // Use a unique_ptr to ensure connection gets deleted even if we have
9 + // thrown an exception (LP: #1295231)
10 + std::unique_ptr<MirConnection> ptr(connection);

This is the wrong place/way to fix the problem.

This code implements a C API and should not be propagating exceptions.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan,

We're not propagating exceptions, for that reason. See the catch() below that.

[Alan Griffiths (alan-griffiths)]

> Alan,
>
> We're not propagating exceptions, for that reason. See the catch() below that.

OK

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1499
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1150/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1350
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1348
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/930
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/882
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/882/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/887
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/887/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/931
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/931/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/870
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5111

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

How does the latest iteration fix a memory leak?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> How does the latest iteration fix a memory leak?

Sorry, that was a think-o.

While the test may expose raciness in the RPC code I don't think the test itself is the problem. AFAICS the race is between the client posting disconnect and the client detecting that the server has disconnected.

review: Approve

[Alan Griffiths (alan-griffiths)]

Rebuilding to see if CI is really happy with this merged.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1500
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1154/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1355
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1353
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/935
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/886
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/886/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/891
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/891/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/936
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/936/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/873
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5124

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1499
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/1156/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1357
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1355
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/937
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/888
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/888/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/893
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/893/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/938
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/938/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/875
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/5129

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

review: Approve (continuous-integration)

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2014-03-06 06:05:17 +0000
+++ src/client/mir_client_library.cpp	2014-03-24 09:43:22 +0000
@@ -120,8 +120,17 @@
 {
     if (!error_connections.contains(connection))
     {
-        auto wait_handle = connection->disconnect();
-        wait_handle->wait_for_all();
+        try
+        {
+            auto wait_handle = connection->disconnect();
+            wait_handle->wait_for_all();
+        }
+        catch (std::exception const&)
+        {
+            // We're implementing a C API so no exceptions are to be
+            // propagated. And that's OK because if disconnect() fails,
+            // we don't care why. We're finished with the connection anyway.
+        }
     }
     else
     {

=== modified file 'tests/acceptance-tests/test_protobuf.cpp'
--- tests/acceptance-tests/test_protobuf.cpp	2014-03-06 06:05:17 +0000
+++ tests/acceptance-tests/test_protobuf.cpp	2014-03-24 09:43:22 +0000
@@ -209,6 +209,11 @@
         &result,
         NewCallback(&callback, &called_back));
 
+    // FIXME - This test is somehow racy. If I add:
+    //    EXPECT_TRUE(false) << connection;
+    // then I can get mir_connection_release to generate an exception during
+    // disconnect() internally, sometimes. Although that exception is caught
+    // internally by the client library so we don't see it here.
     mir_connection_release(connection);
 
     EXPECT_TRUE(called_back);