Merge lp:~raof/mir/no-unbounded-multimaps into lp:mir

[Chris Halse Rogers]

mf::SessionMediator: Remove buffer associations when removing a BufferStream.

Oops. We should really erase the association between a BufferStream and its buffers when the BufferStream is freed, not just remove the buffers from the cache…

Otherwise the stream_associated_buffers multimap will grow with each allocated BufferStream but will not shrink when they a BufferStream is released.

Trivial fix for threaded-ipc branch.

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

PASSED: Continuous integration, rev:4231
https://mir-jenkins.ubuntu.com/job/mir-ci/3558/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/4876
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/5091
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=artful/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=zesty/5080
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=artful/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=mesa,release=zesty/4913/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4913
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/4913/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Don't some of the elements get deleted? By this:

    stream_associated_buffers.erase(current);

Which means by here:

+ stream_associated_buffers.erase(associated_range.first, associated_range.second);

associated_range.first is potentially an invalid iterator.

AFAICS All of this logic amounts to:

    stream_associated_buffers.erase(stream_id);

(while stream_id is in scope)

See lp:~alan-griffiths/mir/no-unbounded-multimaps/+merge/329192

review: Needs Fixing

[Chris Halse Rogers (raof)]

Ah, I think you've been mislead by the whitespace change above.

stream_associated_buffers.erase(current) is called in release_buffer(). The new stream_associated_buffers.erase(associated_range.first, associated_range.second) call is in release_buffer_stream.

This could indeed be replaced by a stream_associated_buffers.erase(stream_id).

[Chris Halse Rogers (raof)]

(Although there will nearly always be buffers associated with a bufferstream, so hitting the race where associated_range.first is invalid would be difficult…)

[Chris Halse Rogers (raof)]

And maybe a slightly better commit message.

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2017-08-16 06:40:10 +0000
+++ src/server/frontend/session_mediator.cpp	2017-08-17 23:06:43 +0000
@@ -581,7 +581,6 @@
             ++match;
             if (std::find(to_release.begin(), to_release.end(), current->second) != to_release.end())
             {
-
                 stream_associated_buffers.erase(current);
             }
         }
@@ -1039,6 +1038,7 @@
     {
         buffer_cache.erase(match->second);
     }
+    stream_associated_buffers.erase(id);
 
     done->Run();
 }