Merge lp:~kdub/mir/testable-surface-tracking into lp:mir

[Kevin DuBois]

frontend: SessionMediator: extract the session tracking of Surface and BufferId's into one place so session mediator is less concerned with it. This makes session mediator a bit more testable.

frontend: SessionMediator: extract the session tracking of Surface and BufferId's into one place so session mediator is less concerned with it. This makes session mediator a bit more testable.

This is a break-out of my tdd-iteration around the exchange_buffer rpc changes. Subsequent branches rely on associating BufferId's and SurfaceId's in a way that was difficult to test with the logic scattered between ClientBufferTracker and SessionMediator. This branch has no ABI change.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1859
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2401/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1332
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1338
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1317
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/923
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/923/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/924
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/924/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/254
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/254/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2402
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/11489

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2401/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

While it makes sense to separate out the tracker logic I'm not sure that it is useful to be able to mock SurfaceTracker.

I'd be inclined to hoist the implementation from SessionSurfaceTracker and avoiding the constructor parameter for SessionMediator by just having a data member.

But I won't block on that as a mockable SurfaceTracker may be what is needed by the follow-up.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1864
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2418/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1356
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1362
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1339
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/940
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/940/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/941
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/941/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/276
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/276/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2421
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/11573

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2418/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

(1) This is an ABI break. We probably don't need the ABI numbers bumped again since they have already for 0.7 but we do need the new header mentioned in *sha1sums:

+++ src/server/frontend/session_mediator.h 2014-08-15 17:55:26 +0000
82 +#include "surface_tracker.h"

Just run: tools/update-all-ABI-sha1sums.sh

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Whoops, that's a private file. I was wrong.

review: Abstain

=== modified file 'src/server/frontend/CMakeLists.txt'
--- src/server/frontend/CMakeLists.txt	2014-08-06 12:22:34 +0000
+++ src/server/frontend/CMakeLists.txt	2014-08-15 17:55:26 +0000
@@ -16,6 +16,7 @@
   socket_messenger.cpp
   event_sender.cpp
   surface.cpp
+  surface_tracker.cpp
   unauthorized_display_changer.cpp
   unauthorized_screencast.cpp
   session_credentials.cpp

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2014-08-14 08:18:45 +0000
+++ src/server/frontend/session_mediator.cpp	2014-08-15 17:55:26 +0000
@@ -45,6 +45,7 @@
 #include "mir/fd.h"
 
 #include "mir/geometry/rectangles.h"
+#include "surface_tracker.h"
 #include "client_buffer_tracker.h"
 #include "protobuf_buffer_packer.h"
 
@@ -83,7 +84,8 @@
     resource_cache(resource_cache),
     screencast(screencast),
     connection_context(connection_context),
-    cursor_images(cursor_images)
+    cursor_images(cursor_images),
+    surface_tracker{static_cast<size_t>(client_buffer_cache_size)}
 {
 }
 
@@ -142,24 +144,14 @@
     Surface& surface,
     std::function<void(graphics::Buffer*, graphics::BufferIpcMsgType)> complete)
 {
-    auto& tracker = client_buffer_tracker[surf_id];
-    if (!tracker) tracker = std::make_shared<ClientBufferTracker>(client_buffer_cache_size);
-
-    auto& client_buffer = client_buffer_resource[surf_id];
-    surface.swap_buffers(client_buffer,
-        [&tracker, &client_buffer, complete](mg::Buffer* new_buffer)
+    surface.swap_buffers( 
+        surface_tracker.last_buffer(surf_id),
+        [this, surf_id, complete](mg::Buffer* new_buffer)
         {
-
-            client_buffer = new_buffer;
-            auto id = client_buffer->id();
-            auto need_full_ipc = !tracker->client_has(id);
-            
-            tracker->add(id);
-
-            if (need_full_ipc)
-                complete(client_buffer, mg::BufferIpcMsgType::full_msg);
+            if (surface_tracker.track_buffer(surf_id, new_buffer))
+                complete(new_buffer, mg::BufferIpcMsgType::update_msg);
             else
-                complete(client_buffer, mg::BufferIpcMsgType::update_msg);
+                complete(new_buffer, mg::BufferIpcMsgType::full_msg);
         });
 }
 
@@ -275,7 +267,7 @@
         auto const id = SurfaceId(request->value());
 
         session->destroy_surface(id);
-        client_buffer_tracker.erase(id);
+        surface_tracker.remove_surface(id);
     }
 
     // TODO: We rely on this sending responses synchronously.

=== modified file 'src/server/frontend/session_mediator.h'
--- src/server/frontend/session_mediator.h	2014-07-30 15:25:54 +0000
+++ src/server/frontend/session_mediator.h	2014-08-15 17:55:26 +0000
@@ -24,9 +24,9 @@
 #include "mir/frontend/surface_id.h"
 #include "mir/graphics/platform.h"
 #include "mir_toolkit/common.h"
+#include "surface_tracker.h"
 
 #include <functional>
-#include <unordered_map>
 #include <memory>
 #include <mutex>
 #include <vector>
@@ -186,8 +186,7 @@
     ConnectionContext const connection_context;
     std::shared_ptr<input::CursorImages> const cursor_images;
 
-    std::unordered_map<SurfaceId,graphics::Buffer*> client_buffer_resource;
-    std::unordered_map<SurfaceId, std::shared_ptr<ClientBufferTracker>> client_buffer_tracker;
+    SurfaceTracker surface_tracker;
 
     std::mutex session_mutex;
     std::weak_ptr<Session> weak_session;

=== added file 'src/server/frontend/surface_tracker.cpp'
--- src/server/frontend/surface_tracker.cpp	1970-01-01 00:00:00 +0000
+++ src/server/frontend/surface_tracker.cpp	2014-08-15 17:55:26 +0000
@@ -0,0 +1,63 @@
+/*
+ * Copyright © 2013 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: Kevin DuBois <[e-mail]>
+ */
+
+#include "surface_tracker.h"
+#include "mir/graphics/buffer.h"
+#include "mir/graphics/buffer_id.h"
+
+namespace mf = mir::frontend;
+namespace mg = mir::graphics;
+
+mf::SurfaceTracker::SurfaceTracker(size_t client_cache_size) :
+    client_cache_size{client_cache_size}
+{
+}
+
+bool mf::SurfaceTracker::track_buffer(SurfaceId surface_id, mg::Buffer* buffer)
+{
+    auto& tracker = client_buffer_tracker[surface_id];
+    if (!tracker)
+        tracker = std::make_shared<ClientBufferTracker>(client_cache_size);
+
+    auto already_tracked = tracker->client_has(buffer->id());
+    tracker->add(buffer->id());
+
+    client_buffer_resource[surface_id] = buffer;
+    return already_tracked;
+}
+
+void mf::SurfaceTracker::remove_surface(SurfaceId surface_id)
+{
+    auto it = client_buffer_tracker.find(surface_id);
+    if (it != client_buffer_tracker.end())
+        client_buffer_tracker.erase(it);
+
+    auto last_buffer_it = client_buffer_resource.find(surface_id);
+    if (last_buffer_it != client_buffer_resource.end())
+        client_buffer_resource.erase(last_buffer_it);
+}
+
+mg::Buffer* mf::SurfaceTracker::last_buffer(SurfaceId surface_id) const
+{
+    auto it = client_buffer_resource.find(surface_id);
+    if (it != client_buffer_resource.end())
+        return it->second;
+    else
+        //should really throw, but that is difficult with the way the code currently works
+        return nullptr;
+}

=== added file 'src/server/frontend/surface_tracker.h'
--- src/server/frontend/surface_tracker.h	1970-01-01 00:00:00 +0000
+++ src/server/frontend/surface_tracker.h	2014-08-15 17:55:26 +0000
@@ -0,0 +1,64 @@
+/*
+ * Copyright © 2013 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: Kevin DuBois <[e-mail]>
+ */
+
+#ifndef MIR_FRONTEND_SURFACE_TRACKER_H_
+#define MIR_FRONTEND_SURFACE_TRACKER_H_
+
+#include "mir/frontend/surface_id.h"
+#include "client_buffer_tracker.h"
+#include <unordered_map>
+#include <memory>
+
+namespace mir
+{
+namespace graphics
+{
+class Buffer;
+}
+namespace frontend
+{
+
+class SurfaceTracker
+{
+public:
+    SurfaceTracker(size_t client_cache_size);
+    SurfaceTracker(SurfaceTracker const&) = delete;
+    SurfaceTracker& operator=(SurfaceTracker const&) = delete;
+
+    /* track a buffer as associated with a surface 
+     * \param surface_id id that the the buffer is associated with
+     * \param buffer     buffer to be tracked (TODO: should be a shared_ptr)
+     * \returns          true if the buffer is already tracked
+     *                   false if the buffer is not tracked
+     */
+    bool track_buffer(SurfaceId, graphics::Buffer*);
+    /* removes the surface id from all tracking */
+    void remove_surface(SurfaceId);
+    /* accesses the last buffer given to track_buffer() for the given SurfaceId */
+    graphics::Buffer* last_buffer(SurfaceId) const;
+private:
+    size_t const client_cache_size;
+
+    std::unordered_map<SurfaceId, graphics::Buffer*> client_buffer_resource;
+    std::unordered_map<SurfaceId, std::shared_ptr<ClientBufferTracker>> client_buffer_tracker;
+};
+
+}
+}
+
+#endif // MIR_FRONTEND_SURFACE_TRACKER_H_

=== modified file 'tests/unit-tests/frontend/test_client_buffer_tracker.cpp'
--- tests/unit-tests/frontend/test_client_buffer_tracker.cpp	2013-08-28 03:41:48 +0000
+++ tests/unit-tests/frontend/test_client_buffer_tracker.cpp	2014-08-15 17:55:26 +0000
@@ -16,11 +16,14 @@
  * Authored by: Christopher James Halse Rogers <[e-mail]>
  */
 
-#include "../../src/server/frontend/client_buffer_tracker.h"
+#include "src/server/frontend/client_buffer_tracker.h"
+#include "src/server/frontend/surface_tracker.h"
 #include "mir/graphics/buffer_id.h"
+#include "mir_test_doubles/stub_buffer.h"
 
 #include <gtest/gtest.h>
 
+namespace mtd = mir::test::doubles;
 namespace mf = mir::frontend;
 namespace mg = mir::graphics;
 
@@ -106,3 +109,99 @@
             EXPECT_TRUE(tracker.client_has(ids[i]));
     }
 }
+
+struct SurfaceTracker : public testing::Test
+{
+    mtd::StubBuffer stub_buffer0;
+    mtd::StubBuffer stub_buffer1;
+    mtd::StubBuffer stub_buffer2;
+    mtd::StubBuffer stub_buffer3;
+    mf::SurfaceId surf_id0{0};
+    mf::SurfaceId surf_id1{1};
+    size_t const client_cache_size{3};
+};
+
+TEST_F(SurfaceTracker, only_returns_true_if_buffer_already_tracked)
+{
+    mf::SurfaceTracker tracker{client_cache_size};
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer0));
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer1));
+    EXPECT_FALSE(tracker.track_buffer(surf_id1, &stub_buffer0));
+    EXPECT_FALSE(tracker.track_buffer(surf_id1, &stub_buffer1));
+}
+
+TEST_F(SurfaceTracker, removals_remove_buffer_instances)
+{
+    mf::SurfaceTracker tracker{client_cache_size};
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_FALSE(tracker.track_buffer(surf_id1, &stub_buffer0));
+
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_TRUE(tracker.track_buffer(surf_id1, &stub_buffer0));
+    EXPECT_EQ(&stub_buffer0, tracker.last_buffer(surf_id0));
+
+    tracker.remove_surface(surf_id0);
+    EXPECT_EQ(nullptr, tracker.last_buffer(surf_id0));
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_TRUE(tracker.track_buffer(surf_id1, &stub_buffer0));
+
+    tracker.remove_surface(mf::SurfaceId{33});
+}
+
+TEST_F(SurfaceTracker, last_client_buffer)
+{
+    mf::SurfaceTracker tracker{client_cache_size};
+
+    tracker.track_buffer(surf_id0, &stub_buffer0);
+    EXPECT_EQ(&stub_buffer0, tracker.last_buffer(surf_id0));
+    tracker.track_buffer(surf_id0, &stub_buffer1);
+    EXPECT_EQ(&stub_buffer1, tracker.last_buffer(surf_id0));
+
+    EXPECT_EQ(nullptr, tracker.last_buffer(surf_id1));
+
+    tracker.track_buffer(surf_id1, &stub_buffer0);
+    EXPECT_EQ(&stub_buffer0, tracker.last_buffer(surf_id1));
+}
+
+TEST_F(SurfaceTracker, buffers_expire_if_they_overrun_cache_size)
+{
+    mf::SurfaceTracker tracker{client_cache_size};
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer1));
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer2));
+    //stub_buffer0 forced out
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer3));
+    //tracker reports its never seen stub_buffer0
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer3));
+
+    EXPECT_EQ(&stub_buffer3, tracker.last_buffer(surf_id0));
+
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer3));
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_TRUE(tracker.track_buffer(surf_id0, &stub_buffer2));
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer1));
+}
+
+TEST_F(SurfaceTracker, buffers_only_affect_associated_surfaces)
+{
+    mf::SurfaceTracker tracker{client_cache_size};
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer0));
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer1));
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer2));
+    EXPECT_EQ(&stub_buffer2, tracker.last_buffer(surf_id0));
+    
+    EXPECT_FALSE(tracker.track_buffer(surf_id1, &stub_buffer0));
+    EXPECT_EQ(&stub_buffer2, tracker.last_buffer(surf_id0));
+    EXPECT_EQ(&stub_buffer0, tracker.last_buffer(surf_id1));
+
+    EXPECT_FALSE(tracker.track_buffer(surf_id0, &stub_buffer3));
+    EXPECT_EQ(&stub_buffer3, tracker.last_buffer(surf_id0));
+    EXPECT_EQ(&stub_buffer0, tracker.last_buffer(surf_id1));
+}

=== modified file 'tests/unit-tests/frontend/test_session_mediator.cpp'
--- tests/unit-tests/frontend/test_session_mediator.cpp	2014-08-15 17:55:26 +0000
+++ tests/unit-tests/frontend/test_session_mediator.cpp	2014-08-15 17:55:26 +0000
@@ -20,6 +20,7 @@
 #include "src/server/frontend/session_mediator.h"
 #include "src/server/report/null_report_factory.h"
 #include "src/server/frontend/resource_cache.h"
+#include "src/server/frontend/surface_tracker.h"
 #include "src/server/scene/application_session.h"
 #include "mir/graphics/display.h"
 #include "mir/graphics/display_configuration.h"
@@ -244,6 +245,16 @@
     mp::DRMMagic drm_request;
     mp::DRMAuthMagicStatus drm_response;
 
+    mtd::StubBuffer buffer1;
+    mtd::StubBuffer buffer2;
+
+    void toggle_buffers(mg::Buffer* b, std::function<void(mg::Buffer* new_buffer)> complete)
+    {
+        if ((!b) || (b == &buffer1))
+            complete(&buffer2);
+        if (b == &buffer2)
+            complete(&buffer1); 
+    }
 };
 }
 
@@ -405,34 +416,22 @@
 TEST_F(SessionMediator, session_only_sends_mininum_information_for_buffers)
 {
     using namespace testing;
-    mtd::StubBuffer buffer1;
-    mtd::StubBuffer buffer2;
-    mp::Buffer buffer_response[3];
-    auto surface = stubbed_session->mock_surface_at(mf::SurfaceId{0});
-    Sequence seq;
+    mf::SurfaceId surf_id{0};
+    auto surface = stubbed_session->mock_surface_at(surf_id);
+    ON_CALL(*surface, swap_buffers(_,_))
+        .WillByDefault(Invoke(this, &SessionMediator::toggle_buffers));
 
     //create
-    EXPECT_CALL(*surface, swap_buffers(_, _))
-        .InSequence(seq)
-        .WillOnce(InvokeArgument<1>(&buffer2));
+    Sequence seq;
     EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer2, mg::BufferIpcMsgType::full_msg))
         .InSequence(seq);
     //swap1
-    EXPECT_CALL(*surface, swap_buffers(&buffer2, _))
-        .InSequence(seq)
-        .WillOnce(InvokeArgument<1>(&buffer1));
     EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer1, mg::BufferIpcMsgType::full_msg))
         .InSequence(seq);
     //swap2
-    EXPECT_CALL(*surface, swap_buffers(&buffer1, _))
-        .InSequence(seq)
-        .WillOnce(InvokeArgument<1>(&buffer2));
     EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer2, mg::BufferIpcMsgType::update_msg))
         .InSequence(seq);
     //swap3
-    EXPECT_CALL(*surface, swap_buffers(&buffer2, _))
-        .InSequence(seq)
-        .WillOnce(InvokeArgument<1>(&buffer1));
     EXPECT_CALL(*graphics_platform, fill_buffer_package(_, &buffer1, mg::BufferIpcMsgType::update_msg))
         .InSequence(seq);
 
@@ -440,57 +439,64 @@
 
     mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
     surface_id_request = surface_response.id();
-    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response[0], null_callback.get());
-    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response[1], null_callback.get());
-    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response[2], null_callback.get());
+    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &surface_id_request, &buffer_response, null_callback.get());
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
 }
 
 TEST_F(SessionMediator, session_with_multiple_surfaces_only_sends_needed_buffers)
 {
     using namespace testing;
-    mtd::StubBuffer buffer[4];
-    mf::SurfaceId first_id{0};
-    mf::SurfaceId second_id{1};
-
+    auto surface0 = stubbed_session->mock_surface_at(mf::SurfaceId{0});
+    auto surface1 = stubbed_session->mock_surface_at(mf::SurfaceId{1});
+    ON_CALL(*surface0, swap_buffers(_,_))
+        .WillByDefault(Invoke(this, &SessionMediator::toggle_buffers));
+    ON_CALL(*surface1, swap_buffers(_,_))
+        .WillByDefault(Invoke(this, &SessionMediator::toggle_buffers));
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_,_,mg::BufferIpcMsgType::full_msg))
+        .Times(4);
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_,_,mg::BufferIpcMsgType::update_msg))
+        .Times(4);
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+
+    mp::Surface surface_response[2];
     mp::SurfaceId buffer_request[2];
-    buffer_request[0].set_value(first_id.as_value());
-    buffer_request[1].set_value(second_id.as_value());
-
-    auto surface1 = stubbed_session->mock_surface_at(first_id);
-    auto surface2 = stubbed_session->mock_surface_at(second_id);
-    EXPECT_CALL(*surface1, swap_buffers(_,_))
-        .WillOnce(InvokeArgument<1>(&buffer[0]))
-        .WillOnce(InvokeArgument<1>(&buffer[1]))
-        .WillOnce(InvokeArgument<1>(&buffer[0]))
-        .WillOnce(InvokeArgument<1>(&buffer[1]));
-    EXPECT_CALL(*surface2, swap_buffers(_,_))
-        .WillOnce(InvokeArgument<1>(&buffer[2]))
-        .WillOnce(InvokeArgument<1>(&buffer[3]))
-        .WillOnce(InvokeArgument<1>(&buffer[2]))
-        .WillOnce(InvokeArgument<1>(&buffer[3]));
-
-    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
-
-    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
-
-    mp::Surface surface_response[2];
-    mp::Buffer buffer_response[6];
-
-    EXPECT_CALL(*graphics_platform, fill_buffer_package(_,_,mg::BufferIpcMsgType::full_msg))
-        .Times(4);
-    EXPECT_CALL(*graphics_platform, fill_buffer_package(_,_,mg::BufferIpcMsgType::update_msg))
-        .Times(4);
-
     mediator.create_surface(nullptr, &surface_parameters, &surface_response[0], null_callback.get());
     mediator.create_surface(nullptr, &surface_parameters, &surface_response[1], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response[0], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response[1], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response[2], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response[3], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response[4], null_callback.get());
-    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response[5], null_callback.get());
-
+    buffer_request[0] = surface_response[0].id();
+    buffer_request[1] = surface_response[1].id();
+
+    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &buffer_request[0], &buffer_response, null_callback.get());
+    mediator.next_buffer(nullptr, &buffer_request[1], &buffer_response, null_callback.get());
+
+    mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
+}
+
+TEST_F(SessionMediator, destroys_tracker_associated_with_destroyed_surface)
+{
+    using namespace testing;
+    mf::SurfaceId first_id{0};
+    mp::Surface surface_response;
+
+    EXPECT_CALL(*graphics_platform, fill_buffer_package(_,_,mg::BufferIpcMsgType::full_msg))
+        .Times(2);
+
+    mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());
+    mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
+    surface_id_request.set_value(first_id.as_value());
+    mediator.release_surface(nullptr, &surface_id_request, nullptr, null_callback.get());
+
+    stubbed_session->last_surface_id = first_id.as_value();
+
+    mediator.create_surface(nullptr, &surface_parameters, &surface_response, null_callback.get());
+    surface_id_request.set_value(first_id.as_value());
+    mediator.release_surface(nullptr, &surface_id_request, nullptr, null_callback.get());
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
 }
 
@@ -503,8 +509,8 @@
     mp::Surface surface_response;
 
     auto surface1 = stubbed_session->mock_surface_at(mf::SurfaceId{0});
-    EXPECT_CALL(*surface1, swap_buffers(_, _))
-        .WillOnce(InvokeArgument<1>(&buffer));
+    ON_CALL(*surface1, swap_buffers(_,_))
+        .WillByDefault(InvokeArgument<1>(&buffer));
 
     mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
     mp::SurfaceId our_surface{surface_response.id()};
@@ -512,6 +518,7 @@
     /* Creating a new surface should not affect our surfaces' buffers */
     EXPECT_CALL(*surface1, swap_buffers(_, _)).Times(0);
     mediator.create_surface(nullptr, &surface_request, &surface_response, null_callback.get());
+    Mock::VerifyAndClearExpectations(surface1.get());
 
     mp::SurfaceId new_surface{surface_response.id()};
     mp::Buffer buffer_response;
@@ -520,7 +527,7 @@
     mediator.next_buffer(nullptr, &new_surface, &buffer_response, null_callback.get());
 
     /* Getting the next buffer of our surface should post the original */
-    EXPECT_CALL(*surface1, swap_buffers(Eq(&buffer), _)).Times(1);
+    EXPECT_CALL(*surface1, swap_buffers(Eq(&buffer),_)).Times(1);
 
     mediator.next_buffer(nullptr, &our_surface, &buffer_response, null_callback.get());
     mediator.disconnect(nullptr, nullptr, nullptr, null_callback.get());
@@ -559,8 +566,9 @@
     mf::SessionMediator mediator{
         shell, graphics_platform, mock_display_selector,
         surface_pixel_formats, report,
-        std::make_shared<mtd::NullEventSink>(),
-        resource_cache, stub_screencast, &connector, nullptr};
+        std::make_shared<mtd::NullEventSink>(), resource_cache,
+        std::make_shared<mtd::NullScreencast>(),
+         nullptr, nullptr};
 
     mediator.connect(nullptr, &connect_parameters, &connection, null_callback.get());