Merge lp:~afrantzis/mir/fix-1401364-death-test-leaks into lp:mir

[Alexandros Frantzis]

tests: Fix memory leaks in death tests that cause subsequent tests to fail
(LP: #1402160)

tests: Fix memory leaks in death tests that cause subsequent tests to fail

To reproduce problem and test fixes do (courtesy of Alan):

valgrind --error-exitcode=1 --trace-children=yes --leak-check=full --show-leak-kinds=definite --errors-for-leak-kinds=definite bin/mir_acceptance_tests --gtest_filter=ClientLibraryErrorsDeathTest*:ClientLibraryThread*

The ClientLibraryThread tests should pass and not report any definite leaks. It's expected to get leaks from the DeathTests.

[Cemil Azizoglu (cemil-azizoglu)]

ok

review: Approve

[Robert Carr (robertcarr)]

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2147
http://jenkins.qa.ubuntu.com/job/mir-ci/2359/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/468/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/468
    ABORTED: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/442/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/356/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/442
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/442/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

That doesn't sound right to me:

ASSERT_FALSE(mir_connection_is_valid(connection));
...
mir_connection_release(connection);

review: Needs Information

[Daniel van Vugt (vanvugt)]

If it is right then we need to fix our API. Because invalid objects should not be releasable.

[Daniel van Vugt (vanvugt)]

Jenkins (above) shows the bug is not fixed:

8: [ FAILED ] ClientLibraryThread.handles_no_signals (732 ms)

review: Needs Fixing

[Chris Halse Rogers (raof)]

As discovered, this branch doesn't fix the problem (https://code.launchpad.net/~raof/mir/fix-all-the-CI/+merge/244526 does).

It's still correct to land, though.

review: Approve

[Daniel van Vugt (vanvugt)]

Also basic logic says no :)

ASSERT_FALSE(mir_connection_is_valid(connection));
// so connection is not valid, and then:
mir_connection_release(connection);

review: Needs Fixing

[Chris Halse Rogers (raof)]

We guarantee that mir_*_release (on something that we return) is always
a valid call, and always necessary.

[Daniel van Vugt (vanvugt)]

Hmm we need to fix that (another day). It's quite mind-bending to say:
    ASSERT_FALSE(mir_connection_is_valid(connection));
    mir_connection_release(connection);
Because it looks like an obvious programming error.

P.S. Bug 1401364 is now fixed by a different branch.

review: Abstain

[Alan Griffiths (alan-griffiths)]

> Jenkins (above) shows the bug is not fixed:
>
> 8: [ FAILED ] ClientLibraryThread.handles_no_signals (732 ms)

That's reassuring - it didn't work for me locally either.

[Alan Griffiths (alan-griffiths)]

Releasing an "error object" is expected use of the API, but the linked bug isn't fixed.

I'm not against landing this - provided the bug is unlinked.

review: Abstain

=== modified file 'tests/acceptance-tests/test_client_library_errors.cpp'
--- tests/acceptance-tests/test_client_library_errors.cpp	2014-12-11 02:43:01 +0000
+++ tests/acceptance-tests/test_client_library_errors.cpp	2014-12-11 12:02:43 +0000
@@ -245,6 +245,8 @@
     ASSERT_FALSE(mir_connection_is_valid(connection));
     EXPECT_DEATH(
         mtf::make_any_surface(connection), "");
+
+    mir_connection_release(connection);
 }
 
 
@@ -256,6 +258,8 @@
 
     ASSERT_FALSE(mir_connection_is_valid(connection));
     EXPECT_DEATH(mtf::make_any_surface(connection), "");
+
+    mir_connection_release(connection);
 }
 
 TEST_F(ClientLibraryErrorsDeathTest, creating_surface_synchronosly_on_invalid_connection_is_fatal)
@@ -266,4 +270,6 @@
 
     ASSERT_FALSE(mir_connection_is_valid(connection));
     EXPECT_DEATH(mtf::make_any_surface(connection), "");
+
+    mir_connection_release(connection);
 }