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

[Daniel van Vugt]

Fix memory leak LP: #1628794, I think.

Also remove an unused (masked) member variable.

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

FAILED: Continuous integration, rev:3823
https://mir-jenkins.ubuntu.com/job/mir-ci/2159/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/2797/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2862
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2854
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2826/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2826/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2826/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2826
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2826/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Fix in the leak is one thing, but why are we doing a memory allocation at all?

It looks as though the unused member variable was intended for use in these call sites and using it would avoid allocating memory on each RPC.

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

This seems unnecessary complex. Do we still have to put mirprotobuf types on heap?

If not a stack variable would do the job.

If this is still the case I think you can get away with a make_unique<mp::Void>() and an empty callback .. like google::protobuf::NewCallback(google::protobuf::DoNothig) (bleh I wish this would be less verbose) so ignore_response can be removed too.

review: Needs Fixing

[Andreas Pokorny (andreas-pokorny)]

PS: the unique_ptr<mp::Void> member that contains the ignored non existing response \o/ could be the yet unused class member..

[Daniel van Vugt (vanvugt)]

All good questions but those are issues I have already considered...

* Why do a memory allocation? Because each call completes asynchronously in the background and must point to a valid Void object and not NULL, or it will crash.

* Stack variables: Not an option. The call completes in the background so the result lives longer than the stack memory that initiated it.

* google::protobuf::DoNothing: Also not an option. The Void* must point to a valid Void object which exists long enough for protobuf to populate it and make the callback. The callback must reference the Void object and be the last reference to it so it gets freed.

* Void is not really void. It contains useful error messages and codes, which we should pay more attention to.

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

PASSED: Continuous integration, rev:3824
https://mir-jenkins.ubuntu.com/job/mir-ci/2167/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/2808
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/2873
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2865
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/2837/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2837
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/2837/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> All good questions but those are issues I have already considered...
>
> * Why do a memory allocation? Because each call completes asynchronously in
> the background and must point to a valid Void object and not NULL, or it will
> crash.

That doesn't answer the question: why not use the Void data member whose lifetime surrounds any RPC calls?

I suspect the answer is:

> * Void is not really void. It contains useful error messages and codes, which
> we should pay more attention to.

If we were to use a single object, then using these error codes /could/ be racy.

Some years ago I went through the IPC stack tuning it for performance - a big part of that was enabling reuse of protobuf objects to avoid memory allocations. I no longer understand this code well enough to know whether there is a race here. Let's just land the fix and worry about performance if it comes around again.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Just got bit by this on my branch.

LGTM.

review: Approve

[Andreas Pokorny (andreas-pokorny)]

ah understood this thing lives inside the callback...

review: Approve

=== modified file 'src/client/buffer_stream.cpp'
--- src/client/buffer_stream.cpp	2016-11-09 02:30:14 +0000
+++ src/client/buffer_stream.cpp	2016-11-15 02:24:53 +0000
@@ -72,9 +72,8 @@
         buf_params->set_pixel_format(format);
         buf_params->set_buffer_usage(usage);
 
-        //note, NewCallback will trigger on exception, deleting this object there
-        auto protobuf_void = new mp::Void;
-        server.allocate_buffers(&request,  protobuf_void,
+        auto protobuf_void = std::make_shared<mp::Void>();
+        server.allocate_buffers(&request, protobuf_void.get(),
             google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
     }
 
@@ -84,9 +83,8 @@
         request.mutable_id()->set_value(stream_id);
         request.add_buffers()->set_buffer_id(buffer_id);
 
-        //note, NewCallback will trigger on exception, deleting this object there
-        auto protobuf_void = new mp::Void;
-        server.release_buffers(&request, protobuf_void,
+        auto protobuf_void = std::make_shared<mp::Void>();
+        server.release_buffers(&request, protobuf_void.get(),
             google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
     }
 
@@ -96,20 +94,17 @@
         request.mutable_id()->set_value(stream_id);
         request.mutable_buffer()->set_buffer_id(buffer.rpc_id());
 
-        //note, NewCallback will trigger on exception, deleting this object there
-        auto protobuf_void = new mp::Void;
-        server.submit_buffer(&request, protobuf_void,
+        auto protobuf_void = std::make_shared<mp::Void>();
+        server.submit_buffer(&request, protobuf_void.get(),
             google::protobuf::NewCallback(Requests::ignore_response, protobuf_void));
     }
 
-    static void ignore_response(mp::Void* void_response)
+    static void ignore_response(std::shared_ptr<mp::Void>)
     {
-        delete void_response;
     }
 
 private:
     mclr::DisplayServer& server;
-    mp::Void protobuf_void;
     int stream_id;
 };