Merge lp:~mir-team/mir/fix-1376324-0.8 into lp:mir/0.8

[Daniel van Vugt]

Backport fix for USC crash LP: #1376324

Because if you look closely, it's still crashing for RTM devices (Mir 0.8.0) that lack the fix:
https://errors.ubuntu.com/problem/bfb855ebc60f251bf495a2ffb24f2924c50bdbf8

Look closer again and you'll see evidence of vivid (Mir 0.9 devices) crashing too. But I think that's actually a different bug with the same basic signature -> bug 1401488 that only exists in 0.9 and later.

So in theory, this fix is all that's needed for 0.8 and RTM.

[Daniel van Vugt (vanvugt)]

Yeah I know it's probably too late for 0.8.1 now we have a revision in QA already. This might be a good idea for 0.8.2 later though.

Discuss.

[Cemil Azizoglu (cemil-azizoglu)]

Daniel, thanks for being on top of the bugs and raising. We'll try to convince product owners to land this.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

(Let's try this again)

Daniel, thanks for being on top of the bugs and bringing attention to this. We'll try to convince product owners to land it.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Daniel, I just noticed Saviq's silo has landed. So we should probably merge back the bot's changelog changes also.

Product owners have approved it. I'll probably put it in a silo tomorrow.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Cemil: The bot's changelog is grossly incorrect as mentioned in email.. And it makes me suspicious of what actually landed.

[Daniel van Vugt (vanvugt)]

BTW, I don't know where the silo landed too -- I haven't yet found any branch having been updated yet.

[Cemil Azizoglu (cemil-azizoglu)]

Silo landed on RTM. As Saviq mentioned in an email, the train updates the changelog like that when the branch has never been used to land in RTM before. Up until Saviq's landing, it had been only used for ubuntu and then the source package was used to sync to rtm.

This will land in 0.8.2 (which means this branch needs to be updated with the correct PATCH version etc.).

[Robert Carr (robertcarr)]

looks ok to me. ABI looks unchanged.

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

All tests passed. In the QA queue now.

review: Approve

[Daniel van Vugt (vanvugt)]

Should we expect some script/robot/silo to land this or is manual intervention required?

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-01-12 04:17:53 +0000
+++ CMakeLists.txt	2015-01-15 03:19:37 +0000
@@ -28,7 +28,7 @@
 
 set(MIR_VERSION_MAJOR 0)
 set(MIR_VERSION_MINOR 8) # This should change at least with every MIRSERVER_ABI
-set(MIR_VERSION_PATCH 1)
+set(MIR_VERSION_PATCH 2)
 
 set(MIR_VERSION ${MIR_VERSION_MAJOR}.${MIR_VERSION_MINOR}.${MIR_VERSION_PATCH})
 
 
=== modified file 'debian/changelog'
--- debian/changelog	2015-01-15 03:13:20 +0000
+++ debian/changelog	2015-01-15 03:19:37 +0000
@@ -1,3 +1,13 @@
+mir (0.8.2-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Bug fix release 0.8.2 (https://launchpad.net/mir/+milestone/0.8.2)
+    - ABIs all unchanged. No downstream projects need rebuilding.
+    - Fixes bug: Crash in /usr/sbin/unity-system-compositor:*** Error in
+      `unity-system-compositor': free(): invalid pointer: ADDR ***
+      (LP: #1376324)
+
+ -- Daniel van Vugt <[e-mail]>  Thu, 15 Jan 2015 11:15:07 +0800
+
 mir (0.8.1+15.04.20150112.2~rtm-0ubuntu1) 14.09; urgency=medium
 
   [ Daniel van Vugt ]
 
=== 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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +0000
@@ -100,6 +100,7 @@
       frame_dropping_enabled{false},
       the_properties{props},
       force_new_compositor_buffer{false},
+      callbacks_allowed{true},
       gralloc{gralloc}
 {
     if (nbuffers < 1)
@@ -434,6 +435,9 @@
             [&]{ return !contains(buffer, pending_snapshots); });
     }
 
+    if (!callbacks_allowed)  // We're shutting down
+        return;
+
     buffers_owned_by_client.push_back(buffer);
 
     lock.unlock();
@@ -502,3 +506,10 @@
        release(buffer, std::move(lock));
     }
 }
+
+void mc::BufferQueue::drop_client_requests()
+{
+    std::unique_lock<std::mutex> lock(guard);
+    callbacks_allowed = false;
+    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	2015-01-15 03:19:37 +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,
@@ -89,6 +90,7 @@
     bool frame_dropping_enabled;
     graphics::BufferProperties the_properties;
     bool force_new_compositor_buffer;
+    bool callbacks_allowed;
 
     std::condition_variable snapshot_released;
     std::shared_ptr<graphics::GraphicBufferAllocator> gralloc;
 
=== 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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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-03 20:09:41 +0000
+++ tests/integration-tests/CMakeLists.txt	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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	2015-01-15 03:19:37 +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;
 
=== modified file 'tests/unit-tests/compositor/test_buffer_queue.cpp'
--- tests/unit-tests/compositor/test_buffer_queue.cpp	2014-09-30 06:11:33 +0000
+++ tests/unit-tests/compositor/test_buffer_queue.cpp	2015-01-15 03:19:37 +0000
@@ -695,6 +695,27 @@
     }
 }
 
+TEST_F(BufferQueueTest, callbacks_cant_happen_after_shutdown)
+{
+    mc::BufferQueue q(1, allocator, basic_properties, policy_factory);
+
+    q.drop_client_requests();
+    auto client = client_acquire_async(q);
+    ASSERT_FALSE(client->has_acquired_buffer());
+}
+
+TEST_F(BufferQueueTest, callbacks_cant_happen_after_shutdown_with_snapshots)
+{
+    mc::BufferQueue q(1, allocator, basic_properties, policy_factory);
+
+    auto snapshot = q.snapshot_acquire();
+    q.drop_client_requests();
+    q.snapshot_release(snapshot);
+
+    auto client = client_acquire_async(q);
+    ASSERT_FALSE(client->has_acquired_buffer());
+}
+
 TEST_F(BufferQueueTest, snapshot_acquire_never_blocks)
 {
     for (int nbuffers = 1; nbuffers <= max_nbuffers_to_test; ++nbuffers)