Merge lp:~vanvugt/mir/simplify-single-object-accesses into lp:mir

[Daniel van Vugt]

Simplify SurfaceMap's single object functions.

Also make them safer and more powerful by replacing raw pointers with
shared pointers. That's the reason I need this - to get a shared
pointer.

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

FAILED: Continuous integration, rev:3874
https://mir-jenkins.ubuntu.com/job/mir-ci/2347/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3061/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3127
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3119
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3119
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3119
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3090
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3090/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/3090
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3090/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3090
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3090/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/3090
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3090/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/3090
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3090/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3090/console

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

review: Needs Fixing (continuous-integration)

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

PASSED: Continuous integration, rev:3875
https://mir-jenkins.ubuntu.com/job/mir-ci/2350/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/3065
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3131
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3123
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3123
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3123
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3094/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/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3094/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3094/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/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3094/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/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3094/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/3094
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3094/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm none to familiar with this code, but the change in behaviour looks rather drastic.

Before: a ConnectionSurfaceMap mutex is locked using "execute around method" and ownership isn't shared. Any actions on the MirSurface are under lock.

After: ownership is being shared meaning the MirSurface can be accessed without the lock.

Far from being "safer and more powerful" it will require the reviewer to invest significant thought to ensure that the changed behaviour doesn't violate consistency assumptions made elsewhere in the code.

Why is this necessary?

review: Needs Information

[Daniel van Vugt (vanvugt)]

The assertion that actions were previously under lock is incorrect. See:

21 - lk.unlock();
22 - exec(surface.get());

So this proposal does not change the locking behaviour at all.

I need this because I'm in a situation where I want each MirBufferStream to hold a weak reference to the MirSurface(s) that use it. This is required to implement client-side vsync where the clock to sync the stream's swapping to is dictated by which output the window (MirSurface) is on.

So MirBufferStream (or anything in future) wants to hold a weak_ptr to its MirSurface(s). Presently we can only store raw pointers, which is unsafe because there's no guarantee the toolkit will always destroy its streams before its surfaces. Using a shared_ptr (or weak_ptr in my case) avoids such risk. And as mentioned above the locking semantics are unchanged.

Admittedly I can work around all this and move forward in my other work but it would be a hack. What I propose here is the cleanest solution and a generally nice to have simplification.

It should also be noted this is not a new idea. It's already used in:

174 virtual std::shared_ptr<MirBuffer> buffer(int buffer_id) const = 0;

[Alan Griffiths (alan-griffiths)]

> The assertion that actions were previously under lock is incorrect. See:
>
> 21 - lk.unlock();
> 22 - exec(surface.get());
>
> So this proposal does not change the locking behaviour at all.

Yikes! The existing code is confusing: The *point* of execute-around is to run paired operations *around* the invoked code (e.g. start transaction/end transaction).

It is far too easy to make the mistake I did and assume the lock is being held. If we're not holding the lock then either /1/ there's no need for this pattern; or, /2/ releasing the lock before exec() is an existing error.

Either way, this MP doesn't break things.

review: Approve

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

FAILED: Autolanding.
More details in the following jenkins job:
https://mir-jenkins.ubuntu.com/job/mir-autolanding/855/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/3082/console
    None: https://mir-jenkins.ubuntu.com/job/generic-land-mp/903/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/3149
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/3141
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/3141
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/3141
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3111
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=yakkety/3111/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/3111
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial+overlay/3111/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3111
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=yakkety/3111/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/3111
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/3111/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/3111
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/3111/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial+overlay/3111/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

^^^
Bug 1628794

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

review: Approve (continuous-integration)

=== modified file 'src/client/connection_surface_map.cpp'
--- src/client/connection_surface_map.cpp	2016-12-02 05:05:06 +0000
+++ src/client/connection_surface_map.cpp	2016-12-08 10:07:47 +0000
@@ -26,23 +26,15 @@
 namespace mcl=mir::client;
 namespace mf=mir::frontend;
 
-void mcl::ConnectionSurfaceMap::with_surface_do(
-    mf::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const
+using mir::client::ConnectionSurfaceMap;
+using mir::frontend::SurfaceId;
+using mir::frontend::BufferStreamId;
+
+std::shared_ptr<MirSurface> ConnectionSurfaceMap::surface(SurfaceId id) const
 {
     std::shared_lock<decltype(guard)> lk(guard);
-    auto const it = surfaces.find(surface_id);
-    if (it != surfaces.end())
-    {
-        auto const surface = it->second;
-        lk.unlock();
-        exec(surface.get());
-    }
-    else
-    {
-        std::stringstream ss;
-        ss << __PRETTY_FUNCTION__ << "executed with non-existent surface ID " << surface_id;
-        BOOST_THROW_EXCEPTION(std::runtime_error(ss.str()));
-    }
+    auto const found = surfaces.find(id);
+    return found != surfaces.end() ? found->second : nullptr;
 }
 
 void mcl::ConnectionSurfaceMap::insert(mf::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface)
@@ -57,23 +49,11 @@
     surfaces.erase(surface_id);
 }
 
-void mcl::ConnectionSurfaceMap::with_stream_do(
-    mf::BufferStreamId stream_id, std::function<void(MirBufferStream*)> const& exec) const
+std::shared_ptr<MirBufferStream> ConnectionSurfaceMap::stream(BufferStreamId id) const
 {
     std::shared_lock<decltype(stream_guard)> lk(stream_guard);
-    auto const it = streams.find(stream_id);
-    if (it != streams.end())
-    {
-        auto const stream = it->second;
-        lk.unlock();
-        exec(stream.get());
-    }
-    else
-    {
-        std::stringstream ss;
-        ss << __PRETTY_FUNCTION__ << "executed with non-existent stream ID " << stream_id;
-        BOOST_THROW_EXCEPTION(std::runtime_error(ss.str()));
-    }
+    auto const found = streams.find(id);
+    return found != streams.end() ? found->second : nullptr;
 }
 
 void mcl::ConnectionSurfaceMap::with_all_streams_do(std::function<void(MirBufferStream*)> const& fn) const

=== modified file 'src/client/connection_surface_map.h'
--- src/client/connection_surface_map.h	2016-12-02 05:05:06 +0000
+++ src/client/connection_surface_map.h	2016-12-08 10:07:47 +0000
@@ -34,11 +34,11 @@
 class ConnectionSurfaceMap : public SurfaceMap
 {
 public:
-    void with_surface_do(frontend::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const override;
+    virtual std::shared_ptr<MirSurface> surface(frontend::SurfaceId) const override;
     void insert(frontend::SurfaceId surface_id, std::shared_ptr<MirSurface> const& surface);
     void erase(frontend::SurfaceId surface_id);
 
-    void with_stream_do(frontend::BufferStreamId stream_id, std::function<void(MirBufferStream*)> const& exec) const override;
+    virtual std::shared_ptr<MirBufferStream> stream(frontend::BufferStreamId) const override;
     void with_all_streams_do(std::function<void(MirBufferStream*)> const&) const override;
 
     void insert(frontend::BufferStreamId stream_id, std::shared_ptr<MirBufferStream> const& chain);

=== modified file 'src/client/rpc/mir_protobuf_rpc_channel.cpp'
--- src/client/rpc/mir_protobuf_rpc_channel.cpp	2016-12-02 05:05:06 +0000
+++ src/client/rpc/mir_protobuf_rpc_channel.cpp	2016-12-08 10:07:47 +0000
@@ -312,10 +312,9 @@
             {
                 if (seq.buffer_request().has_id())
                 {
-                    map->with_stream_do(mf::BufferStreamId(seq.buffer_request().id().value()),
-                    [&] (MirBufferStream* receiver) {
+                    mf::BufferStreamId stream_id(seq.buffer_request().id().value());
+                    if (auto receiver = map->stream(stream_id))
                         receiver->buffer_available(seq.buffer_request().buffer());
-                    });
                 }
                 
                 else if (seq.buffer_request().has_operation())
@@ -372,46 +371,41 @@
                 {
                     rpc_report->event_parsing_succeeded(*e);
 
-                    auto const send_e = [&e](MirSurface* surface)
-                        { surface->handle_event(*e); };
+                    int surface_id = 0;
+                    bool is_surface_event = true;
 
                     switch (e->type())
                     {
                     case mir_event_type_surface:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_surface()->id()), send_e);
+                        surface_id = e->to_surface()->id();
                         break;
-
                     case mir_event_type_resize:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_resize()->surface_id()), send_e);
+                        surface_id = e->to_resize()->surface_id();
                         break;
-
                     case mir_event_type_orientation:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_orientation()->surface_id()), send_e);
+                        surface_id = e->to_orientation()->surface_id();
                         break;
-
                     case mir_event_type_close_surface:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_close_surface()->surface_id()), send_e);
+                        surface_id = e->to_close_surface()->surface_id();
                         break;
                     case mir_event_type_keymap:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_keymap()->surface_id()), send_e);
+                        surface_id = e->to_keymap()->surface_id();
                         break;
                     case mir_event_type_surface_output:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_surface_output()->surface_id()), send_e);
+                        surface_id = e->to_surface_output()->surface_id();
                         break;
                     case mir_event_type_surface_placement:
-                        if (auto map = surface_map.lock())
-                            map->with_surface_do(mf::SurfaceId(e->to_surface_placement()->id()), send_e);
+                        surface_id = e->to_surface_placement()->id();
                         break;
-
                     default:
+                        is_surface_event = false;
                         event_sink->handle_event(*e);
                     }
+
+                    if (is_surface_event)
+                        if (auto map = surface_map.lock())
+                            if (auto surf = map->surface(mf::SurfaceId(surface_id)))
+                                surf->handle_event(*e);
                 }
             }
             catch(...)

=== modified file 'src/client/surface_map.h'
--- src/client/surface_map.h	2016-12-02 05:05:06 +0000
+++ src/client/surface_map.h	2016-12-08 10:07:47 +0000
@@ -40,10 +40,8 @@
 class SurfaceMap
 {
 public:
-    virtual void with_surface_do(
-        frontend::SurfaceId surface_id, std::function<void(MirSurface*)> const& exec) const = 0;
-    virtual void with_stream_do(
-        frontend::BufferStreamId stream_id, std::function<void(MirBufferStream*)> const& exec) const = 0;
+    virtual std::shared_ptr<MirSurface> surface(frontend::SurfaceId) const = 0;
+    virtual std::shared_ptr<MirBufferStream> stream(frontend::BufferStreamId) const = 0;
     virtual void with_all_streams_do(std::function<void(MirBufferStream*)> const&) const = 0;
     virtual std::shared_ptr<MirBuffer> buffer(int buffer_id) const = 0;
     virtual void insert(int buffer_id, std::shared_ptr<MirBuffer> const& buffer) = 0;

=== modified file 'tests/unit-tests/client/test_connection_resource_map.cpp'
--- tests/unit-tests/client/test_connection_resource_map.cpp	2016-12-02 05:05:06 +0000
+++ tests/unit-tests/client/test_connection_resource_map.cpp	2016-12-08 10:07:47 +0000
@@ -56,15 +56,9 @@
 TEST_F(ConnectionResourceMap, maps_surface_when_surface_inserted)
 {
     using namespace testing;
-    auto surface_called = false;
     mcl::ConnectionSurfaceMap map;
     map.insert(surface_id, surface);
-    map.with_surface_do(surface_id, [&](MirSurface* surf) {
-        EXPECT_THAT(surf, Eq(surface.get()));
-        surface_called = true;
-    });
-
-    EXPECT_TRUE(surface_called);
+    EXPECT_EQ(surface, map.surface(surface_id));
 }
 
 TEST_F(ConnectionResourceMap, removes_surface_when_surface_removed)
@@ -73,22 +67,15 @@
     mcl::ConnectionSurfaceMap map;
     map.insert(surface_id, surface);
     map.erase(surface_id);
-    EXPECT_THROW({
-        map.with_stream_do(stream_id, [](MirBufferStream*){});
-    }, std::runtime_error);
+    EXPECT_FALSE(map.stream(stream_id));
 }
 
 TEST_F(ConnectionResourceMap, maps_streams)
 {
     using namespace testing;
-    auto stream_called = false;
     mcl::ConnectionSurfaceMap map;
     map.insert(stream_id, stream);
-    map.with_stream_do(stream_id, [&](MirBufferStream* str) {
-        EXPECT_THAT(str, Eq(stream.get()));
-        stream_called = true;
-    });
-    EXPECT_TRUE(stream_called);
+    EXPECT_EQ(stream, map.stream(stream_id));
     map.erase(stream_id);
 }
 
@@ -126,11 +113,7 @@
     mcl::ConnectionSurfaceMap map;
     map.insert(buffer_id, buffer);
     map.insert(surface_id, surface);
-
-    map.with_surface_do(surface_id,
-        [this, &map](auto) {
-            EXPECT_THAT(map.buffer(buffer_id), Eq(buffer));
-        });
+    EXPECT_EQ(buffer, map.buffer(buffer_id));
 }
 
 TEST_F(ConnectionResourceMap, can_insert_retrieve_erase_render_surface)

=== modified file 'tests/unit-tests/client/test_protobuf_rpc_channel.cpp'
--- tests/unit-tests/client/test_protobuf_rpc_channel.cpp	2016-12-02 05:05:06 +0000
+++ tests/unit-tests/client/test_protobuf_rpc_channel.cpp	2016-12-08 10:07:47 +0000
@@ -59,10 +59,8 @@
 
 struct MockSurfaceMap : mcl::SurfaceMap
 {
-    MOCK_CONST_METHOD2(with_surface_do,
-        void(mir::frontend::SurfaceId, std::function<void(MirSurface*)> const&));
-    MOCK_CONST_METHOD2(with_stream_do,
-        void(mir::frontend::BufferStreamId, std::function<void(MirBufferStream*)> const&));
+    MOCK_CONST_METHOD1(surface, std::shared_ptr<MirSurface>(mir::frontend::SurfaceId));
+    MOCK_CONST_METHOD1(stream, std::shared_ptr<MirBufferStream>(mir::frontend::BufferStreamId));
     MOCK_CONST_METHOD1(with_all_streams_do,
         void(std::function<void(MirBufferStream*)> const&));
     MOCK_CONST_METHOD1(buffer, std::shared_ptr<mcl::MirBuffer>(int));
@@ -74,13 +72,13 @@
 class StubSurfaceMap : public mcl::SurfaceMap
 {
 public:
-    void with_surface_do(
-        mir::frontend::SurfaceId, std::function<void(MirSurface*)> const&) const override
+    std::shared_ptr<MirSurface> surface(mir::frontend::SurfaceId) const override
     {
+        return {};
     }
-    void with_stream_do(
-        mir::frontend::BufferStreamId, std::function<void(MirBufferStream*)> const&) const override
+    std::shared_ptr<MirBufferStream> stream(mir::frontend::BufferStreamId) const override
     {
+        return {};
     }
     void with_all_streams_do(std::function<void(MirBufferStream*)> const&) const override
     {
@@ -796,7 +794,7 @@
     auto mock_buffer_factory = std::make_shared<MockBufferFactory>();
     auto mock_client_buffer = std::make_shared<mtd::MockClientBuffer>();
     auto buf = std::make_shared<mcl::Buffer>(buffer_cb, nullptr, buffer_id, mock_client_buffer, nullptr, mir_buffer_usage_software);
-    EXPECT_CALL(*stream_map, with_stream_do(mir::frontend::BufferStreamId{stream_id},_))
+    EXPECT_CALL(*stream_map, stream(mir::frontend::BufferStreamId{stream_id}))
         .Times(1);
 
     auto transport = std::make_unique<NiceMock<MockStreamTransport>>();