Merge lp:~albaguirre/mir/fix-1433330 into lp:mir

[Alberto Aguirre]

Fix calling dead objects from some protobuf closure objects.

Since the closure/callback object may be invoked by a different thread in some instances (e.g. SessionMediator::exchange_buffer), the closure object now gets a weak_ptr to the ProtobufMessageProcessor (instead of a raw pointer).

Fix calling dead objects from some protobuf closure objects.

Since the closure/callback object may be invoked by a different thread in some instances (e.g. SessionMediator::exchange_buffer), the closure object now gets a weak_ptr to the ProtobufMessageProcessor (instead of a raw pointer).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2403
http://jenkins.qa.ubuntu.com/job/mir-ci/3256/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1685
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1684
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1639
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1253
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1253/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1639
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1639/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4648
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18925

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

29 + callback();
30 + delete this;

We should ensure that 'this' is deleted even if callback() throws.

Looks good otherwise.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

20 +class SelfDeletingCallback : public google::protobuf::Closure {
21 +public:
22 +
23 + SelfDeletingCallback(std::function<void()> const& callback)
24 + : callback(callback)
25 + {
26 + }
27 +
28 + void Run() {
29 + callback();
30 + delete this;
31 + }
32 +
33 + private:
34 + SelfDeletingCallback(SelfDeletingCallback&) = delete;
35 + void operator=(const SelfDeletingCallback&) = delete;
36 + std::function<void()> callback;
37 +};

Why is this needed? google::protobuf::NewCallback<> deletes itself. (NewPermanentCallback<> is the version that doesn't).

And as Alexandros says, callbacks could throw.

review: Needs Fixing

[Alberto Aguirre (albaguirre)]

> 20 +class SelfDeletingCallback : public google::protobuf::Closure {
> 21 +public:
> 22 +
> 23 + SelfDeletingCallback(std::function<void()> const& callback)
> 24 + : callback(callback)
> 25 + {
> 26 + }
> 27 +
> 28 + void Run() {
> 29 + callback();
> 30 + delete this;
> 31 + }
> 32 +
> 33 + private:
> 34 + SelfDeletingCallback(SelfDeletingCallback&) = delete;
> 35 + void operator=(const SelfDeletingCallback&) = delete;
> 36 + std::function<void()> callback;
> 37 +};
>
> Why is this needed? google::protobuf::NewCallback<> deletes itself.
> (NewPermanentCallback<> is the version that doesn't).

The code in question used to use google::protobuf::NewCallback<>. I need to be able to promote a weak_ptr within Run() so I can't use it as is. I don't want to change any Protobuf related code hence SelfDeletingCallback.

> And as Alexandros says, callbacks could throw.
OK.

[Alberto Aguirre (albaguirre)]

> 29 + callback();
> 30 + delete this;
>
> We should ensure that 'this' is deleted even if callback() throws.
>
> Looks good otherwise.

Will fix.
Note all ::google::protobuf::Closure implementations have the same issue.

[Kevin DuBois (kdub)]

20 +class SelfDeletingCallback : public google::protobuf::Closure {
indentation

28 + void Run()
override?

also, something like this would unintuitively double-free:
{
  auto c = std::make_unique<SelfDeletingCallback>();
  c->Run();
}
maybe make ~SelfDeletingCallback() private to avoid.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

OK

review: Approve

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Kevin DuBois (kdub)]

lgtm too

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2404
http://jenkins.qa.ubuntu.com/job/mir-ci/3264/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1694
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1693/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1648
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1261
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1261/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1648
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1648/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4653
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18946

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

review: Needs Fixing (continuous-integration)

[Alberto Aguirre (albaguirre)]

^---Ummm, can't replicate locally and can't tell if it is related to this MP. Let's try re-triggering a build.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2404
http://jenkins.qa.ubuntu.com/job/mir-ci/3267/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1698
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1697
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1652
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1264
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1264/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1652
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1652/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4657
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18957

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2405
http://jenkins.qa.ubuntu.com/job/mir-ci/3270/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1702
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1701
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1656
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1267
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1267/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1656
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1656/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4660
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18963

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

review: Approve (continuous-integration)

=== modified file 'cmake/MirCommon.cmake'
--- cmake/MirCommon.cmake	2015-03-13 05:22:41 +0000
+++ cmake/MirCommon.cmake	2015-03-18 16:01:57 +0000
@@ -98,7 +98,6 @@
         set(EXCLUDED_TESTS "UnresponsiveClient.does_not_hang_server:DemoInProcessServerWithStubClientPlatform.surface_creation_does_not_leak_fds:StreamTransportTest/0.SendsFullMessagesWhenInterrupted")
     endif()
 
-    message("What is this: " "${EXECUTABLE_OUTPUT_PATH}/${EXECUTABLE} --gtest_list_tests ${EXCLUDED_TESTS}")
     add_custom_target(
       ${TEST_DISCOVERY_TARGET_NAME} ALL
       ${EXECUTABLE_OUTPUT_PATH}/${EXECUTABLE} --gtest_list_tests | ${CMAKE_BINARY_DIR}/mir_gtest/mir_discover_gtest_tests --executable=${EXECUTABLE_OUTPUT_PATH}/${EXECUTABLE} --exclusions=${EXCLUDED_TESTS} ${DISCOVER_FLAGS}

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2015-02-18 05:27:28 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2015-03-18 16:01:57 +0000
@@ -74,9 +74,35 @@
     return request;
 }
 
+class SelfDeletingCallback : public google::protobuf::Closure
+{
+public:
+    SelfDeletingCallback(std::function<void()> const& callback)
+    : callback(callback)
+    {
+    }
+
+    void Run() override
+    {
+        struct Deleter
+        {
+          ~Deleter() { delete obj; }
+          SelfDeletingCallback* obj;
+        };
+        Deleter deleter{this};
+        callback();
+    }
+
+ private:
+  ~SelfDeletingCallback() = default;
+  SelfDeletingCallback(SelfDeletingCallback&) = delete;
+  void operator=(const SelfDeletingCallback&) = delete;
+  std::function<void()> callback;
+};
+
 template<typename RequestType, typename ResponseType>
 void invoke(
-    ProtobufMessageProcessor* self,
+    std::shared_ptr<ProtobufMessageProcessor> const& mp,
     DisplayServer* server,
     void (mir::protobuf::DisplayServer::*function)(
         ::google::protobuf::RpcController* controller,
@@ -88,15 +114,17 @@
 {
     auto const result_message = std::make_shared<ResponseType>();
 
-    auto const callback =
-        google::protobuf::NewCallback<
-            ProtobufMessageProcessor,
-            ::google::protobuf::uint32,
-             std::shared_ptr<ResponseType>>(
-                self,
-                &ProtobufMessageProcessor::send_response,
-                invocation_id,
-                result_message);
+    std::weak_ptr<ProtobufMessageProcessor> weak_mp = mp;
+    auto const response_callback = [weak_mp, invocation_id, result_message]
+    {
+        auto message_processor = weak_mp.lock();
+        if (message_processor)
+        {
+            message_processor->send_response(invocation_id, result_message);
+        }
+    };
+
+    auto callback = new SelfDeletingCallback{response_callback};
 
     try
     {
@@ -108,9 +136,8 @@
     }
     catch (std::exception const& x)
     {
-        delete callback;
         result_message->set_error(boost::diagnostic_information(x));
-        self->send_response(invocation_id, result_message);
+        callback->Run();
     }
 }
 
@@ -179,7 +206,7 @@
         else if ("next_buffer" == invocation.method_name())
         {
             auto request = parse_parameter<mir::protobuf::SurfaceId>(invocation);
-            invoke(this, display_server.get(), &DisplayServer::next_buffer, invocation.id(), &request);
+            invoke(shared_from_this(), display_server.get(), &DisplayServer::next_buffer, invocation.id(), &request);
         }
         else if ("exchange_buffer" == invocation.method_name())
         {
@@ -187,7 +214,7 @@
             request.mutable_buffer()->clear_fd();
             for (auto& fd : side_channel_fds)
                 request.mutable_buffer()->add_fd(fd);
-            invoke(this, display_server.get(), &DisplayServer::exchange_buffer, invocation.id(), &request);
+            invoke(shared_from_this(), display_server.get(), &DisplayServer::exchange_buffer, invocation.id(), &request);
         }
         else if ("release_surface" == invocation.method_name())
         {
@@ -205,7 +232,7 @@
             for (auto& fd : side_channel_fds)
                 request.add_fd(fd);
 
-            invoke(this, display_server.get(), &DisplayServer::platform_operation,
+            invoke(shared_from_this(), display_server.get(), &DisplayServer::platform_operation,
                    invocation.id(), &request);
         }
         else if ("configure_display" == invocation.method_name())

=== modified file 'src/server/frontend/protobuf_message_processor.h'
--- src/server/frontend/protobuf_message_processor.h	2015-01-21 07:34:50 +0000
+++ src/server/frontend/protobuf_message_processor.h	2015-03-18 16:01:57 +0000
@@ -37,7 +37,8 @@
 class DisplayServer;
 class ProtobufMessageSender;
 
-class ProtobufMessageProcessor : public MessageProcessor
+class ProtobufMessageProcessor : public MessageProcessor,
+                                 public std::enable_shared_from_this<ProtobufMessageProcessor>
 {
 public:
     ProtobufMessageProcessor(

=== modified file 'tests/unit-tests/frontend/test_protobuf_message_processor.cpp'
--- tests/unit-tests/frontend/test_protobuf_message_processor.cpp	2015-02-18 05:27:28 +0000
+++ tests/unit-tests/frontend/test_protobuf_message_processor.cpp	2015-03-18 16:01:57 +0000
@@ -87,6 +87,7 @@
         mt::fake_shared(stub_msg_sender),
         mt::fake_shared(stub_display_server),
         mt::fake_shared(stub_report));
+    std::shared_ptr<mfd::MessageProcessor> mp = mt::fake_shared(pb_message_processor);
 
     mpw::Invocation raw_invocation;
     mp::BufferRequest buffer_request;
@@ -97,7 +98,6 @@
     mfd::Invocation invocation(raw_invocation);
 
     std::vector<mir::Fd> fds;
-    mfd::MessageProcessor* mp = &pb_message_processor;
     mp->dispatch(invocation, fds);
 
     ASSERT_THAT(stub_display_server.exchange_buffer_response, testing::Ne(nullptr));