Merge lp:~alan-griffiths/mir/fix-1376324 into lp:mir

[Alan Griffiths]

scene, compositor: ensure a buffer requested by a surface is not delivered after the surface is deleted.

scene, compositor: ensure a buffer requested by a surface is not delivered after the surface is deleted.

I can find nothing in the Mir code that prevents a surface being rendered while the same surface is being deleted. If a pending buffer request from the client is honored after the deletion then we would have a stack trace like lp:1376324

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1957
http://jenkins.qa.ubuntu.com/job/mir-ci/1726/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2003
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2010/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1948/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/64/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/885/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1959
http://jenkins.qa.ubuntu.com/job/mir-ci/1728/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2006
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2013
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1951
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/66
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/66/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/888
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/888/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2963
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14060

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

LGTM.

review: Approve

[Daniel van Vugt (vanvugt)]

Theoretically there shouldn't be any use of deleted surfaces if we're maintaining ownership properly (shared_ptr). Is this a consequence of those raw Buffer pointers we still have in places?

review: Needs Information

[Daniel van Vugt (vanvugt)]

Oh I see. The real issue is that the callback object doesn't maintain a reference to the object (Surface) that owns the callback.

And it appears the only place where such an offending callback exists (twice) is in session_mediator.cpp; search for "swap_buffers". But even those callbacks that SessionMediator is passing to swap_buffers appear to be safe. We must be failing to check surface_id is valid then?...

I think there might be a race between a callback that's in-flight and the surface destruction. I can't see any synchronization that would prevent the crash from still happening if they both occur at basically the same time.

Why not just make the callbacks themselves uncrashable (since the SessionMediator has a longer lifetime it can check things)? That would also not be vulnerable to racing like this solution seems to be.

Also, where you have the fix right now in ~BasicSurface(), is the same place where the BufferStream will get destroyed, meaning the BufferQueue will be destroyed and no callbacks can happen after ~BasicSurface() right now anyway. So I'm not convinced this proposal is making anything better yet. Dead surfaces theoretically can never receive callbacks already, because a callback can't happen if the BufferQueue doesn't exist any more.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Daniel,

1. The pending callback is owned by BufferQueue which is (indirectly) owned by BasicSurface. If the callback were to own the SocketConnection (which indirectly owns the BasicSurface) then there would be an ownership loop.

2. If the BufferQueue were deleted (as you are assuming) when the BasicSurface is destroyed then there wouldn't be a problem. (And the new test would pass in -r 1955.)

HTH

[Alexandros Frantzis (afrantzis)]

If I understand the issue correctly: a Surface hands out Temporary*Buffers that use the BufferQueue (and keep a strong pointer to it). When a surface is deleted, the compositor may be holding a Temporary*Buffer and therefore the BufferQueue is kept alive. During composition, as buffers are released, pending client notifications may be invoked with invalid objects. This is particularly likely to occur with overlays, since for overlays we keep buffer references for longer (until we get the next frame).

In the error report, the crash happens during message serialization, which indicates that ProtobufResponder may have been deleted. So this likely occurs when an application has shut down or is shutting down.

The fix seems plausible, and lacking any way to reproduce the original problem, we have to wait and see if it eliminates the error reports. Perhaps there are other options for fixing this, but let's first ascertain the cause and we can move from there.

review: Approve

[Daniel van Vugt (vanvugt)]

Aha! Yes thanks Alexandros for pointing that out. The BufferQueue can indeed live longer than the Surface that owns it, because the compositor gets its own temporary reference to the BufferStream (hence BufferQueue) here:

class SurfaceSnapshot : public mg::Renderable
{
public:
    SurfaceSnapshot(
        std::shared_ptr<mc::BufferStream> const& stream,
        void const* compositor_id,
        geom::Rectangle const& position,
        glm::mat4 const& transform,
        bool visible,
        float alpha,
        bool shaped,
        mg::Renderable::ID id)
    : underlying_buffer_stream{stream},

This is a relatively new feature. If SurfaceSnapshot didn't exist then we wouldn't have a bug. Because then the BufferStream would be definitely destroyed with the BasicSurface.

review: Approve

[Daniel van Vugt (vanvugt)]

Hmm, actually SurfaceSnapshot doesn't really need the BufferStream at all. The SurfaceSnapshot is created at the instance when we want to composite. We should instead just pass it a buffer, and not let it hold a reference to the BufferStream. In theory that should guarantee that BasicSurface holds a unique pointer (which we should enforce as unique_ptr) to BufferStream and thus it would become impossible for the BufferQueue to live longer than the surface it calls back into.

I think that would be a nicer (and smaller?) fix, if it works.

review: Needs Information

[Alexandros Frantzis (afrantzis)]

> I think that would be a nicer (and smaller?) fix, if it works.

Just changing SurfaceSnapshot alone wouldn't help. We would still have the problem because we would have to keep references to Buffer objects, which are really instances of Temporary*Buffer and thus keep a strong reference to BufferQueue.

[Alan Griffiths (alan-griffiths)]

I accept that injecting "surface_buffer_stream->drop_client_requests()" in the destructor seems inelegant.

But the alternative of disentangling the ownership of the buffer from the lifetime of the BufferStream is far from trivial and may not be any clearer.

This is a pragmatic solution and allows us to determine if the crash reports are affected.

[Andreas Pokorny (andreas-pokorny)]

ok

review: Approve

=== modified file 'src/include/server/mir/compositor/buffer_stream.h'
--- src/include/server/mir/compositor/buffer_stream.h	2014-09-11 05:51:44 +0000
+++ src/include/server/mir/compositor/buffer_stream.h	2014-10-02 13:13:30 +0000
@@ -54,6 +54,7 @@
     virtual void force_requests_to_complete() = 0;
     virtual int buffers_ready_for_compositor() const = 0;
     virtual void drop_old_buffers() = 0;
+    virtual void drop_client_requests() = 0;
 };
 
 }

=== modified file 'src/server/compositor/buffer_bundle.h'
--- src/server/compositor/buffer_bundle.h	2014-09-11 05:51:44 +0000
+++ src/server/compositor/buffer_bundle.h	2014-10-02 13:13:30 +0000
@@ -68,6 +68,7 @@
      */
     virtual int buffers_free_for_client() const = 0;
     virtual void drop_old_buffers() = 0;
+    virtual void drop_client_requests() = 0;
 
 protected:
     BufferBundle() = default;

=== modified file 'src/server/compositor/buffer_queue.cpp'
--- src/server/compositor/buffer_queue.cpp	2014-09-30 06:11:33 +0000
+++ src/server/compositor/buffer_queue.cpp	2014-10-02 13:13:30 +0000
@@ -502,3 +502,9 @@
        release(buffer, std::move(lock));
     }
 }
+
+void mc::BufferQueue::drop_client_requests()
+{
+    std::unique_lock<std::mutex> lock(guard);
+    pending_client_notifications.clear();
+}

=== modified file 'src/server/compositor/buffer_queue.h'
--- src/server/compositor/buffer_queue.h	2014-09-30 06:11:33 +0000
+++ src/server/compositor/buffer_queue.h	2014-10-02 13:13:30 +0000
@@ -63,6 +63,7 @@
     bool framedropping_allowed() const;
     bool is_a_current_buffer_user(void const* user_id) const;
     void drop_old_buffers() override;
+    void drop_client_requests() override;
 
 private:
     void give_buffer_to_client(graphics::Buffer* buffer,

=== modified file 'src/server/compositor/buffer_stream_surfaces.cpp'
--- src/server/compositor/buffer_stream_surfaces.cpp	2014-09-11 05:51:44 +0000
+++ src/server/compositor/buffer_stream_surfaces.cpp	2014-10-02 13:13:30 +0000
@@ -94,3 +94,8 @@
 {
     buffer_bundle->drop_old_buffers();
 }
+
+void mc::BufferStreamSurfaces::drop_client_requests()
+{
+    buffer_bundle->drop_client_requests();
+}

=== modified file 'src/server/compositor/buffer_stream_surfaces.h'
--- src/server/compositor/buffer_stream_surfaces.h	2014-09-11 05:51:44 +0000
+++ src/server/compositor/buffer_stream_surfaces.h	2014-10-02 13:13:30 +0000
@@ -53,6 +53,7 @@
     void force_requests_to_complete() override;
     int buffers_ready_for_compositor() const override;
     void drop_old_buffers() override;
+    void drop_client_requests() override;
 
 protected:
     BufferStreamSurfaces(const BufferStreamSurfaces&) = delete;

=== modified file 'src/server/scene/basic_surface.cpp'
--- src/server/scene/basic_surface.cpp	2014-09-26 08:16:25 +0000
+++ src/server/scene/basic_surface.cpp	2014-10-02 13:13:30 +0000
@@ -151,6 +151,9 @@
 ms::BasicSurface::~BasicSurface() noexcept
 {
     report->surface_deleted(this, surface_name);
+
+    if (surface_buffer_stream) // some tests use null for surface_buffer_stream
+        surface_buffer_stream->drop_client_requests();
 }
 
 std::shared_ptr<mc::BufferStream> ms::BasicSurface::buffer_stream() const

=== modified file 'tests/include/mir_test_doubles/mock_buffer_bundle.h'
--- tests/include/mir_test_doubles/mock_buffer_bundle.h	2014-09-11 05:51:44 +0000
+++ tests/include/mir_test_doubles/mock_buffer_bundle.h	2014-10-02 13:13:30 +0000
@@ -51,6 +51,7 @@
     MOCK_METHOD0(drop_old_buffers, void());
     int buffers_ready_for_compositor() const override { return 1; }
     int buffers_free_for_client() const override { return 1; }
+    void drop_client_requests() override {}
 };
 
 }

=== modified file 'tests/include/mir_test_doubles/mock_buffer_stream.h'
--- tests/include/mir_test_doubles/mock_buffer_stream.h	2014-09-11 05:51:44 +0000
+++ tests/include/mir_test_doubles/mock_buffer_stream.h	2014-10-02 13:13:30 +0000
@@ -58,6 +58,7 @@
     MOCK_METHOD0(force_requests_to_complete, void());
     MOCK_CONST_METHOD0(buffers_ready_for_compositor, int());
     MOCK_METHOD0(drop_old_buffers, void());
+    MOCK_METHOD0(drop_client_requests, void());
 };
 }
 }

=== modified file 'tests/include/mir_test_doubles/stub_buffer_stream.h'
--- tests/include/mir_test_doubles/stub_buffer_stream.h	2014-09-11 05:51:44 +0000
+++ tests/include/mir_test_doubles/stub_buffer_stream.h	2014-10-02 13:13:30 +0000
@@ -82,6 +82,7 @@
     int buffers_ready_for_compositor() const override { return 1; }
 
     void drop_old_buffers() override {}
+    void drop_client_requests() override {}
 
     StubBuffer stub_client_buffer;
     std::shared_ptr<graphics::Buffer> stub_compositor_buffer;

=== modified file 'tests/integration-tests/CMakeLists.txt'
--- tests/integration-tests/CMakeLists.txt	2014-10-02 03:05:27 +0000
+++ tests/integration-tests/CMakeLists.txt	2014-10-02 13:13:30 +0000
@@ -24,6 +24,7 @@
   test_session_manager.cpp
   test_session.cpp
   session_management.cpp
+  surface_composition.cpp
 )
 
 add_subdirectory(client/)

=== added file 'tests/integration-tests/surface_composition.cpp'
--- tests/integration-tests/surface_composition.cpp	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/surface_composition.cpp	2014-10-02 13:13:30 +0000
@@ -0,0 +1,125 @@
+/*
+ * Copyright © 2014 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored By: Alan Griffiths <[e-mail]>
+ */
+
+#include "src/server/scene/basic_surface.h"
+#include "src/server/report/null_report_factory.h"
+#include "src/server/compositor/buffer_stream_surfaces.h"
+#include "src/server/compositor/buffer_queue.h"
+
+#include "mir_test_doubles/null_surface_configurator.h"
+#include "mir_test_doubles/stub_buffer_allocator.h"
+#include "mir_test_doubles/stub_frame_dropping_policy_factory.h"
+#include "mir_test_doubles/stub_input_sender.h"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+namespace geom = mir::geometry;
+namespace mc = mir::compositor;
+namespace mg = mir::graphics;
+namespace mi = mir::input;
+namespace mr = mir::report;
+namespace ms = mir::scene;
+namespace mtd = mir::test::doubles;
+
+using namespace testing;
+
+namespace
+{
+struct SurfaceComposition : Test
+{
+    auto create_surface() const
+    -> std::shared_ptr<ms::Surface>
+    {
+        return std::make_shared<ms::BasicSurface>(
+            std::string("SurfaceComposition"),
+            geom::Rectangle{{},{}},
+            false,
+            create_buffer_stream(),
+            create_input_channel(),
+            create_input_sender(),
+            create_surface_configurator(),
+            create_cursor_image(),
+            mr::null_scene_report());
+
+    }
+
+    int const number_of_buffers = 3;
+
+    auto create_buffer_stream() const
+    ->std::shared_ptr<mc::BufferStream>
+    {
+        return std::make_shared<mc::BufferStreamSurfaces>(create_buffer_bundle());
+    }
+
+    auto create_buffer_bundle() const
+    -> std::shared_ptr<mc::BufferBundle>
+    {
+        return std::make_shared<mc::BufferQueue>(
+            number_of_buffers,
+            allocator,
+            basic_properties,
+            policy_factory);
+    }
+
+    auto create_input_channel() const
+    -> std::shared_ptr<mi::InputChannel> { return {}; }
+
+    auto create_cursor_image() const
+    -> std::shared_ptr<mg::CursorImage>  { return {}; }
+
+    auto create_surface_configurator() const
+    -> std::shared_ptr<ms::SurfaceConfigurator>
+    { return std::make_shared<mtd::NullSurfaceConfigurator>(); }
+
+    auto create_input_sender() const
+    -> std::shared_ptr<mi::InputSender>
+    { return std::make_shared<mtd::StubInputSender>(); }
+
+    std::shared_ptr<mg::GraphicBufferAllocator> const allocator
+        {std::make_shared<mtd::StubBufferAllocator>()};
+
+    mg::BufferProperties const basic_properties
+        { geom::Size{3, 4}, mir_pixel_format_abgr_8888, mg::BufferUsage::hardware };
+
+    mtd::StubFrameDroppingPolicyFactory policy_factory;
+};
+}
+
+// Presumptive cause of lp:1376324
+TEST_F(SurfaceComposition, does_not_send_client_buffers_to_dead_surfaces)
+{
+    auto surface = create_surface();
+
+    mg::Buffer* old_buffer{nullptr};
+
+    auto const callback = [&] (mg::Buffer* new_buffer)
+        {
+            // If surface is dead then callback is not expected
+            EXPECT_THAT(surface.get(), NotNull());
+            old_buffer = new_buffer;
+        };
+
+    // Exhaust the buffers to ensure we have a pending swap to complete
+    for (auto i = 0; i != number_of_buffers; ++i)
+        surface->swap_buffers(old_buffer, callback);
+
+    auto const renderable = surface->compositor_snapshot(this);
+
+    surface.reset();
+}

=== modified file 'tests/integration-tests/test_exchange_buffer.cpp'
--- tests/integration-tests/test_exchange_buffer.cpp	2014-09-15 18:20:14 +0000
+++ tests/integration-tests/test_exchange_buffer.cpp	2014-10-02 13:13:30 +0000
@@ -78,6 +78,7 @@
     void force_requests_to_complete() {};
     int buffers_ready_for_compositor() const { return -5; }
     void drop_old_buffers() {}
+    void drop_client_requests() override {}
 
     std::vector<std::shared_ptr<mg::Buffer>> client_buffers;
     std::vector<mg::BufferID> const buffer_id_seq;